4

say I had a class that creates objects and keeps track of the number of objects with a static variable. Something like this:

public class Apple {
    private static int count = 0;

    public Apple () {
        count++;
    } 

    public void removeApple() {
        count--;
    }
}

When I check this code with FindBugs I get the warning Write to static field from instance method, which is obvious of course.

How can I work around this problem and make it more safe and even get rid of that FindBugs Warning??

user2426316
  • 7,131
  • 20
  • 52
  • 83
  • similar discussion at http://stackoverflow.com/questions/13388829/write-to-static-field-is-findbugs-wrong-in-this-case – Karthik T Jul 02 '13 at 10:08

4 Answers4

9

1. General programming advice

This message is there to warn you about a potential programming mistake because it is a common pitfall for beginner programmers, who are not aware of the differences in the scope of static and instance variables.

However, if you can declare your real version of the removeApple method static without causing any compiler errors, then most probably you should. This would both take care of the warning and make it clear that this method has nothing to do with any specific instance of your class.

2. Concerns related to concurrency

Another aspect of this warning concerns thread safety. If you write to a static field from an instance, that opens the possibility that you'll be doing concurrent updates from different threads, even with no sharing of the class instances between threads.

If you don't need thread safety for your code (which is perfectly fine in general), then you don't need to do anything. If you do need it, then synchronize all updates of the field, or use an AtomicInteger wrapper.

Personally, I'd opt for AtomicInteger because it's the safest option: other options would need you to chase all field updates around the class and make sure they are synchronized. Using AtomicInteger is very simple:

private static final AtomicInteger count = new AtomicInteger();

and then you use count.getAndIncrement() instead of count++.

Community
  • 1
  • 1
Marko Topolnik
  • 195,646
  • 29
  • 319
  • 436
3

Use AtomicInteger instead of int primitive.

You might synchronize the method.

duffymo
  • 305,152
  • 44
  • 369
  • 561
1

You have two choices:

  1. Use AtomicInteger as mentioned by duffmyo

An AtomicInteger is used in applications such as atomically thread-safe incremented counters.

or

2 . Control the access of the variable by synchronized block

And just from the Findbug error removal perspective:

Logically, we expect instance methods to affect that instance's data. We expect static methods to affect static data.

Making the counter private and providing the public getter and setter methods will take away that findbug error.

Juned Ahsan
  • 67,789
  • 12
  • 98
  • 136
1

By all probability FindBugs would prefer that removeApple() were static

Evgeniy Dorofeev
  • 133,369
  • 30
  • 199
  • 275