Is it a good or bad practice to call instance methods from a java constructor?
- by Steve
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?
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.
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.
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?
