tags:

views:

104

answers:

7

I have a class MyClass:

public class MyClass {
  private MyComplexType member1;
}

I have to do some pretty intense initialization on member1. Enough that it easily warrants its own method, called from the MyClass constructor.

My question is, which of the following formats is best for this method?

private MyComplexType initMyComplexType() { 
  MyComplexType result = new MyComplexType();  
  // extensive initialization on result...
  return result;
}

called like this:

public MyClass() {
  member1 = initMember1();
}

OR

private void initMember1() {
  member1 = new MyComplexType();
  // extensive initialization on member1...
}

called like this:

public MyClass() {
  initMember1();
}

Which is the better style for a private member? Why?

+5  A: 

I would choose the first option as it more clearly expresses the purpose of the init method and shows the data flow explicitly.

Not to mention that it makes the code in the init method potentially reusable. Should you need to initialize another variable later, you can just call the method again without worrying about side effects. Furthermore, if that other variable is in another class, you can easily move the method to somewhere accessible to both places.

Along this line, I would also consider remaining the init method to something like doExtensiveComplexCalculation to decouple it from your actual member variable.

Péter Török
Whoops, good point, that should be named something different. Editing...
Tom Tresansky
+3  A: 

Go for option 1. Apart from reasons mentioned by Peter, it is a better practice because this way you have a computation-intensive but side-effect-free function init(), and lighter but state-modifying constructor. It's recognized as good practice to separate these two features.

Also, using template/factory method is open for extension. It's easier to to override it (or its part if you use template method) in subclasses. Again, that's thanks to separation of computations from state modifications.

Edit: As others have stated, also consider renaming initComplexMember() to buildContextMember().

Konrad Garus
Agreed, but: I would not name it `initMember1()` because it does not only initialize it but also creates the object. A better name would be, depending on exact circumstances, `createMyComplexType()` or `createMember1()`.
Moritz Both
@Moritz Both: Yes, I agree.
Konrad Garus
"using template/factory method is open for extension. It's easier to to override it (or its part if you use template method) in subclasses." However, it is not a good idea to (directly or indirectly) call virtual methods from a constructor.
Péter Török
+3  A: 

Only the first allows you to assign the result to a final member, and that's reason enough for me.

Mark Peters
+4  A: 

Another drawback of the second approach is that the field member1 potentially exposes a partially initialized MyComplexType to another thread.

Example of overridden protected static method in reply to Jörn Horstmann:

public class StaticOverrideParent {

 protected static void doSomething() {
  System.out.println("Parent doing something");
 }
}

public class StaticNoOverride extends StaticOverrideParent {

 public static void main(String[] args) {
  doSomething();
 }
}

public class StaticOverride extends StaticOverrideParent {

 protected static void doSomething() {
  System.out.println("Doing something");
 }

 public static void main(String[] args) {
  doSomething();
 }
}

Running StaticNoOverride prints "Parent doing something". Running StaticOverride prints "Doing something".

Adriaan Koster
Didn't even think of that. Shouldn't be a concern in this particular case, but still: good point!
Tom Tresansky
This is not a 'override', as far as the jvm is concerned these are two completey independent functions. See http://java.sun.com/docs/books/jls/second_edition/html/classes.doc.html#227928 for details.
Jörn Horstmann
+1  A: 

Very good reasons have already been given (final member assignment, multi-threading issue, improved readability), the technical ones being very strong and enough for me. I'll just add a little extract from the Java Tutorials:

Initializing Instance Members

Normally, you would put code to initialize an instance variable in a constructor. There are two alternatives to using a constructor to initialize instance variables: initializer blocks and final methods.

Initializer blocks for instance variables look just like static initializer blocks, but without the static keyword:

{

    // whatever code is needed for initialization goes here
}

The Java compiler copies initializer blocks into every constructor. Therefore, this approach can be used to share a block of code between multiple constructors.

A final method cannot be overridden in a subclass. This is discussed in the lesson on interfaces and inheritance. Here is an example of using a final method for initializing an instance variable:

class Whatever {
    private varType myVar = initializeInstanceVariable();

    protected final varType initializeInstanceVariable() {

        //initialization code goes here
    }
}

This is especially useful if subclasses might want to reuse the initialization method. The method is final because calling non-final methods during instance initialization can cause problems. Joshua Bloch describes this in more detail in Effective Java.

I tend to favor the above style (unless I don't want the initialization to occur in all constructors but this is unusual).

Pascal Thivent
+1  A: 

I'd prefer the Spring IoC container to the kind of complex hard-coded initialization you've described. It leads to better separation of concerns and is a much better environment for unit testing.

Alan Krueger
Maybe you can give an example? I don't think complex initialization can always by done declaratively in the Spring applicationContext.
Adriaan Koster
For objects that are complicated enough to not fit directly into the declarative app context, you can provide a FactoryBean to adapt your object to the IoC container.
Alan Krueger
+1  A: 

Good reasons have been given by the other commentators to use a helper function to initialise the variable. I just want to add that I actually prefer to use a private or protected static function for this. This makes it really clear that this is just an initialization helper that can in no way modify other state of the object, and it also can not get overridden by a subclass.

Jörn Horstmann
A protected static method can be overriden by a subclass, see the example in my previous answer
Adriaan Koster