Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

I had written something like the following,

public class ClassName {
//private instance variables
  public ClassName() {
    //initialize instance variables
    this.someFunction();
  }
  public void someFunction() {
    //do something 
  }
}

The person who reviews my code asked me to move the call to someFunction() method outside of the constructor. And I had to do something like this wherever I created an object for the class,

ClassName object = new ClassName();
object.someFunction();

Calling a method from constructor is a bad idea? Why the second way is preferred?

share|improve this question
4  
I think the answer is it depends on what method is doing and if it calls any overridable methods etc This link has some good discussions stackoverflow.com/questions/5764689/… – dreza Nov 18 '12 at 5:49
Can you share more details about the exact nature of your code? You could go the "dependency injection" route and require that the constructor accepts an already constructed, meaningful object, so that your code is more Lego-like. If it is a one-off hack, then you must make the helper method private and preferably static. – Leonid Nov 23 '12 at 18:54
add comment (requires an account with 50 reputation)

3 Answers

up vote 10 down vote accepted

It is bad practice to call overridable methods in c-tor. Read What's wrong with overridable method calls in constructors? It is error prone.

To avoid this bad practice in future (even advanced developers some time do this mistake) please use special static code analysis tools: Checkstyle+extension, PMD, FindBug.

share|improve this answer
thanks! got it, if the method is overridden it will cause problems. – Senthil Kumar Nov 19 '12 at 3:39
add comment (requires an account with 50 reputation)
up vote 7 down vote

In addition to what Roman wrote, you should ask yourself:

Is this method call necessary to construct a valid object?

If the answer is yes, you absolutely should leave it in the constructor (and the method should probably be private). Otherwise, you'll end up with uninitialized objects when someone forgets to call the method after calling the constructor.

If the answer is no, then it shouldn't be in the constructor.

Could the method potentially fail?

I usually avoid using constructors for error-prone tasks such as opening files or even (gasp) prompting for user input. Again, the problem is that a failed method might leave your object in a half-baked state, which means subsequent method calls might fail.

If you need to do something risky to create an instance, you could use a static factory method to clarify your intent, such as XmlParser.tryLoadFile(path).

Does the method take a long time to run?

Constructors are expected to return quickly. If the constructor does a lot of long-running work, you probably need to fix your design and move that work out of the constructor.

share|improve this answer
thanks! it is helpful. – Senthil Kumar Nov 19 '12 at 3:40
If you do not need the call for a valid object but you foresee that you will frequently call it right after the constructor, think about a static factory method doing both. – scarfridge Nov 19 '12 at 8:18
add comment (requires an account with 50 reputation)
up vote 1 down vote

In general, you should try to avoid doing anything which could be unexpected for some other developers in the constructor. One expects the initialization of fields. So this is the upper bound then.
Some of the specific reasons are already pointed out, I just wanted to show this more general view.

You could use a static factory method. Than you save the additional line in the normal code and you will not forget it somewhere:

public class ClassName {
//private instance variables
  private ClassName() {
    //initialize instance variables
  }

  private void someFunction() {
    //do something 
  }

  public static ClassName create/make/getPreparedInstance()
  {
      ClassName className = new ClassName();
      className.someFunction();
      return className;
  }
}
share|improve this answer
add comment (requires an account with 50 reputation)

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.