18

I have a class that looks similar to this, and findbugz is complaining about the 'write to the static field from the instance method' (initialize(), and killStaticfield()). I can't set the static field in the ctor.

  • What is the best fix for this issue?

  • Would putting staticField in an AtomicReference suffice? 

     public class Something
 {
  private static SomeClass staticField = null;
  private AnotherClass aClass;
  public Something()
  {

  }

  public void initialize()
  {
    //must be ctor'd in initialize
    aClass = new AnotherClass();
    staticField = new SomeClass( aClass );
  }

  public void killStaticField()
  {
   staticField = null;
  }

  public static void getStaticField()
  {
    return staticField;
  }
}
Akhil Jain
  • 13,872
  • 15
  • 57
  • 93
darrickc
  • 1,872
  • 6
  • 27
  • 38
  • To answer your question, this field is static because the get method needs to be static so other objects can access the staticField without having a reference to a Something object. – darrickc Sep 02 '10 at 20:07
  • 1
    Basically, my question is what's the best way to fix the 'write to static field from instance method' findbugz warning; I just made up the code to represent the warning. Is it better to wrap the static object in an AtomicReference object, or to synchronize? – darrickc Sep 02 '10 at 20:19

4 Answers4

17

Staying as close as possible to your original design...

public class Something {
  private static volatile SomeClass staticField = null;

  public Something() {
  }

  public static SomeClass getStaticField() {
    if(Something.staticField == null)
      Something.staticField = new SomeClass();;
    return Something.staticField;
  }
}

Refer to your static variable via the class name, that will remove the findbugz warning. Mark your static variable as volatile, which will make the reference safer in a multithreaded environment.

Even better would be:

public class Something {
  private static final SomeClass staticField = new SomeClass();

  public Something() {
  }

  public static SomeClass getStaticField() {
    return Something.staticField;
  }
}
Vikas Pal
  • 83
  • 1
  • 1
  • 10
romacafe
  • 3,098
  • 2
  • 23
  • 27
  • But a design without synchronized would also mean no killStaticField. – extraneon Sep 03 '10 at 13:34
  • You know... Looking at the first solution, its still not thread-safe, even though the variable is volatile. You'd have to put a synchronized{} block around the if-null-then-new section... I'd definitely prefer the second option. – romacafe Sep 03 '10 at 15:59
  • I think (unfortunately) the first solution is the right answer in my case. Mainly because I can't initialize staticField at the top, it has to be initialized after initialize() is called (I'll edit the question to reflect that). – darrickc Sep 03 '10 at 18:17
  • If we went with the 1st code block we'd have to make all access to staticField synchronize around SomeClass.class. – darrickc Sep 03 '10 at 18:19
  • I changed its reference by using ClassName.staticField, however findbugs still complain the same thing. – Tiina Mar 09 '18 at 11:37
5

The question is what you want to do with the static field. If it changes for every class you create it might not be a good idea to have it static at all. If it gets initialized only once you should just lazily initialize it as a singleton.

public class Something
{
    private static SomeClass staticField = null;

    public Something()
    {

    }

    public static SomeClass getStaticField()
    {
        if(staticField == null)
            staticField = new SomeClass();;
        return staticField;
    }
}
Vikas Pal
  • 83
  • 1
  • 1
  • 10
Daff
  • 43,734
  • 9
  • 106
  • 120
  • Don't forget to make that `getStaticField` method `synchronized`! – Richard Fearn Sep 02 '10 at 19:39
  • And note, you can't just put synchronized on the static method definition, as static methods use the Something.class to synchronize against! – darrickc Sep 02 '10 at 20:08
  • 1
    I think the use of static variables should be clear before discussing any advanced threading issues. And sorry, but your question doesn't really sound as if you understood when to use static variables and when not. Maybe you can clarify your question a little more. – Daff Sep 02 '10 at 20:11
  • @darrickc What is wrong with syncing on the class? If your class is only a singleton holder (as your example) that is a decent sync solution. – extraneon Sep 03 '10 at 13:37
4

Remove static from staticField if it should not be static.

Make kill and getStaticField static themselves. And you usually reference static by the class name, not by an (implicit) this, to make very clear that it is static and may cause unexpected consequences in other thReads.

When in doubt, don't use statics for non-constant fields.

extraneon
  • 23,575
  • 2
  • 47
  • 51
0

The best way is not to do it, try to find a better design patern. If really necessary this will work and make findbugs/spotbugs not complain.

public class Something
{
    private static SomeClass staticField = null;

    public Something()
    {
    }

    private void setStaticField(SomeClass value){
        staticField=value;
    } 

    public static SomeClass getStaticField()
    {
        if(staticField == null)
            setStaticField(new SomeClass());
        return staticField;
    }
}
The CTO
  • 71
  • 3