Is it a good or bad practice to call instance methods from a java constructor?

Question

There are several different ways I can initialize complex objects (with injected dependencies and required set-up of injected members), are all seem reasonable, but have various advantages and disadvantages. I'll give a concrete example:

final class MyClass {
  private final Dependency dependency;
  @Inject public MyClass(Dependency dependency) {
    this.dependency = dependency;
    dependency.addHandler(new Handler() {
      @Override void handle(int foo) { MyClass.this.doSomething(foo); }
    });
    doSomething(0);
  }
  private void doSomething(int foo) { dependency.doSomethingElse(foo+1); }
}

As you can see, the constructor does 3 things, including calling an instance method. I've been told that calling instance methods from a constructor is unsafe because it circumvents the compiler's checks for uninitialized members. I.e. I could have called doSomething(0) before setting this.dependency, which would have compiled but not worked. What is the best way to refactor this?

1. Make doSomething static and pass in the dependency explicitly? In my actual case I have three instance methods and three member fields that all depend on one another, so this seems like a lot of extra boilerplate to make all three of these static.

2. Move the addHandler and doSomething into an @Inject public void init() method. While use with Guice will be transparent, it requires any manual construction to be sure to call init() or else the object won't be fully-functional if someone forgets. Also, this exposes more of the API, both of which seem like bad ideas.

3. Wrap a nested class to keep the dependency to make sure it behaves properly without exposing additional API:

   class DependencyManager {
     private final Dependency dependency;
     public DependecyManager(Dependency dependency) { ... }
     public doSomething(int foo) { ... }
   }
   @Inject public MyClass(Dependency dependency) {
     DependencyManager manager = new DependencyManager(dependency);
     manager.doSomething(0);
   }

   This pulls instance methods out of all constructors, but generates an extra layer of classes, and when I already had inner and anonymous classes (e.g. that handler) it can become confusing - when I tried this I was told to move the DependencyManager to a separate file, which is also distasteful because it's now multiple files to do a single thing.

So what is the preferred way to deal with this sort of situation?

Answer 1

Yeah, it's actually illegal, It really shouldn't even compile (but I believe it does)

Consider the builder pattern instead (and lean towards immutable which in builder pattern terms means that you can't call any setter twice and can't call any setter after the object has been "used"--calling a setter at that point should probably throw a runtime exception).

You can find the slides by Joshua Bloch on the (new) Builder pattern in a slide presentation called "Effective Java Reloaded: This Time It's for Real", for example here:

http://docs.huihoo.com/javaone/2007/java-se/

Answer 2

Josh Bloch in Effective Java recommends using a static factory method, although I can't find any argument for cases like this. There is, however, a similar case in Java Concurrency in Practice, specifically meant to prevent leaking out a reference to this from the constructor. Applied to this case, it would look like:

final class MyClass {
  private final Dependency dependency;

  private MyClass(Dependency dependency) {
    this.dependency = dependency;
  }

  public static createInstance(Dependency dependency) {
    MyClass instance = new MyClass(dependency);
    dependency.addHandler(new Handler() {
      @Override void handle(int foo) { instance.doSomething(foo); }
    });
    instance.doSomething(0);
    return instance;
  }
  ...
}

However, this may not work well with the DI annotation you use.

Answer 3

You could use a static method that takes the dependency and constructs and return a new instance, and mark the constructor Friend. I'm not sure Friend exists in java though (is it package protected.) This might not be the best way though. You could also use another class that is a Factory for creating MyClass.

Edit: Wow another posted just suggested this same exact thing. Looks like you can make constructors private in Java. You can't do that in VB.NET (not sure about C#)... very cool...

Answer 4

It also messes badly with inheritance. If your constructor is being called in the chain to instantiate a subclass of your class, you may call a method which is overridden in the subclass and relies on an invariant that is not established until the subclass constructor has been run.

Answer 5

You'd want to be careful about using instance methods from within the constructor, as the class has not been fully constructed yet. If a called method uses a member that has not yet been initialized, well, bad things will happen.