Writing to a static variable in an instance method, why is this a bad practice?

Question

I am a little confused here with this findbugs warning in eclipse.

public class MyClass {
    public static String myString;
}


public class AnotherClass {
   public void doSomething() {
       MyClass.myString = "something";
   }
}

This gives me a findbugs warning "write to static field from instance method", however this does not give me a warning:

public class MyClass {
    public static String myString;
}


public class AnotherClass {
   public void doSomething() {
       doAnotherThing();
   }
   public static doAnotherThing() {
       MyClass.myString = "something";
   }
}

How is this any different?, and why is writing to a static variable from an instance method a bad practice?, I assume it has to do with synchronization, but it is still not clear to me.

I know this looks like the variable should be final, but I am loading the value from a properties file.

Answer 1

Its a form of aliasing, which may be counter-intuitive. Counter-intuitive code hampers ease of maintenance.

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

Let's rename doSomething to initialize:

...
a.initialize();
...
b.initialize();
...

The reader of this code may not immediately realize that the instances of a and b are actually affecting the same data. This may be a bug since we're initializing the same memory twice, but its non-obvious since it seems reasonable that we may need to call initialize on each instance.

However, the the code were:

...
MyClass.initialize();
...
MyClass.initialize();
...

In this case, its more intuitive that we're likely affecting the same static data and this is likely a bug.

This is similar to the common version of aliasing where two variables in the same scope point to the same instance.

---

For your last example,

• an instance calls a static method

  The fact that an instance method is calling a static method isn't expected to raise flags. The examples were this is useful far outweigh where its likely a problem.

• a static method of one class affects another class' static data

  In one sense, it should generate a different, but similar warning: that one class is messing with the data of another class. However, by making the static variable public is a way of tacitly approving of this, so such a warning isn't necessary.

Keep in mind that FindBugs is simply trying to flag potential likely problems, not every possible problem, in your code. Your first example is likely a potential maintenance issue that you need to examine whether its a real problem. Your second example is likely not a problem or it is a real problem that is too similar to use cases where it is not a problem.