FCCD_FIND_CLASS_CIRCULAR_DEPENDENCY False Positive

[mttjj]

We've been getting some code flagged with the FCCD_FIND_CLASS_CIRCULAR_DEPENDENCY detector that we believe to be a false positive. Here's an example of the code that causes the flag:

public abstract class A
{
    public static A getInstance()
    {
        return new AImpl();
    }

    public abstract void doSomething();
}

// (In a different file)
public class AImpl extends A
{
    @Override
    public void doSomething()
    {
        System.out.println("In AImpl do something."); //$NON-NLS-1$
    }
}

Now, we don't deny that this is a circular dependency, however we feel that static methods should be exempt from this check. Uplifting this code to use Java 8 idioms we have come up with this solution:

public interface A
{
    static A getInstance()
    {
        return new AImpl();
    }

    void doSomething();
}

// (in another file)
public class AImpl implements A
{
    @Override
    public void doSomething()
    {
        System.out.println("In AImpl do something."); //$NON-NLS-1$
    }
}

Using this Java 8 code, the code is NOT flagged by this circular dependency check. We've proven out that updating the code like this would be both source and binary incompatible so we're not comfortable making the non-passive change. To us, the static method on the abstract class (which is flagged) is logically equivalent to the static method on the interface (which is not flagged) which is why we feel that static methods should be exempt from this check.

We would love to hear your thoughts on this. Thank you.

[mebigfatguy]

Well, this detector isn't about building circular object graphs, for instance, with a concern for building data islands that the gc would have trouble with. The JVM handles those effectively. This is more about design and composability of classes and ease of refactoring. So the consequence of 'static' isn't really applicable with regard to whether or not i flag the issue. With regard to not flagging static methods in interfaces, that is just an oversight, something that i don't often see, so it hasn't come up before.

[ThrawnCA]

Basically, it's generally not a good idea for the abstract parent to know about the subclass. If the singleton can be expected to always be of the subclass type, why is there an abstract parent at all?

[mttjj]

Thanks for the responses. I guess our argument is that since the factory method (getInstance) is static then you could technically move that method to any other class, right? So something like this would avoid the issue I think but we're just trying to make things more convenient by putting the static factory method on the abstract class itself.

public class AFactory {
    public static A getInstance() {
        return new AImpl();
    }
}

public interface A {
    void doSomething();
}

public class AImpl implements A {
    // and so forth...
}

In response to ThrawnCA's comments, this class is abstract simply because we started using this pattern/convention before Java 8 (which as shown in my first comment is what we would like to ultimately end up with). The abstract class never has any implementation code and is essentially an interface for all intents and purposes. Because of that and because the getInstance method is static I would argue that A still doesn't 'know about its subclass'.

[ddesna]

I had a problem also with this rule. Here is code that fb-contrib considers as circular dependency:

public class Customer {
  private Date regDate;

public void setRegDate(Date regDate) {
  this.regDate = Utils.checkDate(regDate);
}

  public getRegDate() {
    return regDate;
  }
}

public class Utils {
  public static Date checkDate(Date date) {
    // Do some stuff to validate date ...
    return date;
  }

  public static void validateCustomer(Customer customer) {
    // Some stuff validating Customer object
    customer.getRegDate();
  }
}

So, fb-contrib says that Customer class have circular dependency with Utils class. It happens due Utils.validateCustomer method that accepts Customer object as parameter. If I remove this method this circular dependency error disappears.

[ThrawnCA]

@ddesna That appears to be unrelated to the original issue here, and it also appears to be a straightforward case where there is indeed a circular dependency. If Utils is a generic helper class, then it probably should not have any specific knowledge about the Customer class. You should either move the validation logic into Customer, or have a third CustomerValidator class.

[ddesna]

Thanks for you suggestions @ThrawnCA, this si what I did, move Customer validation to another helper class. Sorry for offtopic.