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

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 clea开发者_如何学运维r to me.

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

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.

There aren't many use cases for why you would want to change a static field. Remember that if you set this field to a new value that this value has changed for all instances of this class. This might get you into trouble in a multi-threaded environment, where more than one thread is calling doSomething(). Proper synchronisation is required.

In 99% of all cases, you want your instance methods to change the non-static fields only, which is why findbugs warns you.

And findbugs isn't clever enough to find out about your instance method indirectly changing the field in your second example :)

This is what FindBugs has to say about this: http://findbugs.sourceforge.net/bugDescriptions.html#ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD

This is my take, so take it with a grain of salt. You mentioned synchronization issues, which are a major reason for this warning, but more importantly, the two cases are fundamentally operating on different conceptual "levels" of data. Instance methods are "owned" by objects and modify data that describes individual instances. Class methods are generic operations and state that, while related to the class, are not related to individual objects. Thus, modifying that state from within each instance would probably (but not necessarily) be a poor design decision.

Because changing a static field changes it for all instances, causing untold problems if not properly synchronised.

If you're reading in a properties file to set shared fields, then do it in a static method. Alternatively, refactor the fields into a separate singleton instance that the other class can only read from. If you're only going to have one instance, then use a singleton pattern and make the fields non-static.

Static methods should only affect static data, and instance methods should only affect instance data.

I don't think synchronization (mentioned in several answers) has any bearing on this. After all, static methods can be called from multiple threads just as easily as can instance methods.

The reason for the warning (not very well explained by the FindBugs documentation) is, I think, hinted at by a couple of answers: it's suspicious and possibly a mistake. Like Jochen Bedersdorfer said, there aren't all that many use cases where you want to assign to a static variable in one class from an instance method in another. Just like

while (x = y) {
    // ...
}

isn't technically an error (and actually legal Java if x and y are boolean), it's almost always a mistake. Similarly, the authors of FindBug felt the same about the subject case.