Write to static field - is FindBugs wrong in this case?

Question

I have a Java class like this:

public class Foo {

    public static int counter = 0;

    public void bar(int counter) {
        Foo.counter = counter;
    }
}

FindBugs warns me about writing to the static field counter via the instance method bar. However, if I change the code to:

public class Foo {

    public static int counter = 0;

    public static void setCounter(int counter) {
        Foo.counter = counter;
    }

    public void bar(int counter) {
        setCounter(counter);
    }
}

Then FindBugs won't complain. Isn't that wrong? I'm still writing to a static field from an instance method, just via a static method, am I not?

Answer 1

Suppose that at some point in the future, you decide this setter method needs to be thread safe and you want to make it synchronized.

This code will work fine:

public synchronized static void setCounter(int counter) {
    Foo.counter = counter;
}

public void bar(int counter) {
    setCounter(counter);
}

This code is wrong and will have incorrect behavior:

public synchronized void bar(int counter) {
    Foo.counter = counter;
}

This might not seem like a significant difference in this contrived example, especially since counter can usually just be marked volatile. However, in a real world example where the setter method has more complicated logic and is being called from many different places (not just from one instance method), the latter pattern will be easier to refactor.

As an aside, in my opinion Google's CodePro Analytix plugin is a much faster and more comprehensive tool than FindBugs.

Related:

• Synchronized vs. Volatile in Java
• Synchronized Getters and Setters

Answer 2

From the FindBugs list of bug descriptions:

ST: Write to static field from instance method (ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD)

This instance method writes to a static field. This is tricky to get correct if multiple instances are being manipulated, and generally bad practice.

There is no similar bug description for access to a static field via a static method called from an instance method.

You may want to discuss the rationale behind this decision on the FindBugs mailing list