Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I had written something like the following,

public class ClassName {
//private instance variables
  public ClassName() {
    //initialize instance variables
  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();

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

share|improve this question

closed as off-topic by 200_success Dec 8 '15 at 7:47

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

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… – 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
up vote 11 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

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

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();
      return className;
share|improve this answer

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