views:

435

answers:

4

I see this idiom of initializing instance variables quite a bit

public class Test{
    private Long l = 1l;
    private MyClass mc = new MyClass();

    public Test(){}
    ...
 }

But I would prefer

public class Test{
    private Long l;
    private MyClass mc;

    public Test(){
        l = 1l;
        mc = new MyClass();
    }
    ...
}

Considering that these are non-final variables, are the 2 approaches equivalent or is one "more" correct than the other in terms of thread safety?

+1  A: 

since your variables are instance variables, not class variables, you don't have a thread safety issue during initialization using either method. I'm sure others will chime in if there's a Java-standard-recommended best practice.

atk
@atk: Are you saying that if those were "static" variables there would be a thread safety issue, by my understanding static initialization is guaranteed thread-safe according to the JMM.
non sequitor
And, of course, we wouldn't be initializing the "static" variables in the constructor either, so it would either be initialized like above or in a static block which is also thread safe
non sequitor
@non sequitor: Yes, there would be a thread safety risk with static variables. Static variables are class-wide instead of instance-specific. Initializing static variables in a constructor may be non-threadsafe if no additional locking is performed. "static initialization" is a different thing than "initializing a static variable". "static initialization" is the OP's first example (if it were with static variables) or within a "static {}" block
atk
@atk: my second comment ruled out initializing static variables in the constructor, I've never even tried that -- it shouldn't even be legal to initialize a static variable in a constructor, goes against the spirit of the concepts
non sequitor
@non sequitor: Apologies. I didn't realize that you considered the issue ruled out - I thought your initial response was still standing. However, under certain circumstances, it can be appropriate to use or initialize statics in constructors. I realize we haven't been talking about use, but it's probably worth covering. A simple example of use would be counting the number of instances of the class.
atk
Another possibility would be a static instance for which the application required full initialization of the application before a static could be initialized. For example, imagine a client that caches state, but that state is stored in a database. The application would require connection to the database prior to initializing the static variable.
atk
A: 

In terms of thread safety, they are equivalent. Both will need to execute the same instructions, and if you prefer the second (which I agree with you in your preference) then I would use that. If you want thread safety around a constructor, you would need a synchronized call around the constructor call.

aperkins
as atk noted, I am not sure that there even ARE thread safety issues here, unless the object is stored in a globally accessible location.
aperkins
If you make the field `final` (which ever equivalent piece of code you use) and don't let `this` escape by the end of the constructor, then the instance will be safe from unsafe publication. In reality, unsafe publication is relatively rare (or at least it is rarely done on purpose).
Tom Hawtin - tackline
I am aware that unsafe publication is rarely done on purpose, I was simply pointing it out in case they made that mistake - it is a difficult one to track down sometimes :)
aperkins
+4  A: 

Thread safety isn't an issue because this happens at construction phase, and two threads cannot be constructing the same object. Well, if you let this escape from the constructor, it might be possible for another thread to access the object during construction, but you really shouldn't do that. Functionality-wise, the two options are the same, so even if there were thread-safety issues, they would affect both the same way.

The first option, of initializing the fields at their declaration, is not always possible if you need to do some computation that cannot be done in an initializer (even then, you can keep the initialization out of the constructor if you do it in an initializer block, though). But if either way is possible, then it's purely a style issue, and I don't think there is a clear preference among Java programmers, so go with whichever seems better to you.

jk
+1 for mentioning the one way that instance variables can get manipulated by another thread while the constructor is executing. However, even making this possible seems like an abuse of a constructor. If you have to do that much initialization, I would put it in a separate method anyway.
Daniel Pryden
I just want to mention, that you can do a lot of computations in the `instance initializer` block ( `{}` blocks inside the class body ). However I would not recommend using them for general case described in the question ( they are invaluable when doing Java version of closures ). If you need computation, do it in the constructor, or static factory method.
Alexander Pogrebnyak
@Daniel: It is unfortunately very common that people let "this" escape from the constructor while creating and starting threads (if this implements Runnable), adding themselves as listeners to others etc. FindBugs will catch it but I see it too often to say people understand why it is dangerous.
Fredrik
This answer is, unfortunately, wrong. Making the fields final would be the best solution if the object is immutable. http://java.sun.com/docs/books/jls/third_edition/html/memory.html#66562 has more on the importance of final fields in immutable objects that need to be thread safe.
MB
+1  A: 

I think it's the matter of personal preference and your project coding standards.

Just make sure you only initialize variables in one place ( either constructor, or inline ).

Having initialization work done in the constructor gives you a better place for exception handling.

Alexander Pogrebnyak