10

I have something like this:

    public abstract class Menu {
     public Menu() {
      init();
     }

     protected abstract void init();

     protected void addMenuItem(MenuItem menuItem) {
      // some code...
     }
    }

    public class ConcreteMenu extends Menu {
     protected void init() {
      addMenuItem(new MenuItem("ITEM1"));
      addMenuItem(new MenuItem("ITEM2"));
      // ....
     }
    }

//Somewhere in code
Menu menu1 = new ConcreteMenu();

As you can see superclass's init method is abstract and is called by constructor automatically after object is created.

I'm curious if i can run into some sort of problems with code like this, when i need to create some object of this kind whose structure wont't be changed in time.

Would be any approach better? It works in Java, but will it work in C++ and possibly ActionScript?

Thank you for answer.

Jarek
  • 7,425
  • 15
  • 62
  • 89

3 Answers3

12

DO NOT INVOKE OVERRIDEABLE METHODS FROM THE CONSTRUCTOR.

A quote from Effective Java 2nd Edition, Item 17: Design and document for inheritance, or else prohibit it:

There are a few more restrictions that a class must obey to allow inheritance. Constructors must not invoke overridable methods, directly or indirectly. If you violate this rule, program failure will result. The superclass constructor runs before the subclass constructor, so the overriding method in the subclass will be invoked before the subclass constructor has run. If the overriding method depends on any initialization performed by the subclass constructor, the method will not behave as expected.

Here's an example to illustrate:

public class ConstructorCallsOverride {
    public static void main(String[] args) {
        abstract class Base {
            Base() { overrideMe(); }
            abstract void overrideMe(); 
        }
        class Child extends Base {
            final int x;
            Child(int x) { this.x = x; }
            @Override void overrideMe() {
                System.out.println(x);
            }
        }
        new Child(42); // prints "0"
    }
}

Here, when Base constructor calls overrideMe, Child has not finished initializing the final int x, and the method gets the wrong value. This will almost certainly lead to bugs and errors.

Related questions

See also

Community
  • 1
  • 1
polygenelubricants
  • 376,812
  • 128
  • 561
  • 623
  • 1
    Related Q: http://stackoverflow.com/questions/1511822/java-call-overridden-method-implicitly/ ; http://stackoverflow.com/questions/3288601/problem-in-instance-variable-initialization – polygenelubricants Jul 27 '10 at 11:09
  • Related post of mine on Java constructors and template method pattern: http://novyden.blogspot.com/2011/08/java-anti-pattern-constructors-and.html – topchef Aug 08 '11 at 08:00
  • I understand that a constructor must not call any virtual methods *whose contract does not specify that they must be callable from the constructor*. Is there any problem with having constructor call a virtual method whose contract specifies that it must not rely upon any fields which the subclass constructor would set? – supercat May 21 '13 at 19:12
  • One of the first things you learn in Object Oriented Programming class is the order of execution (the constructor invokes the constructor of its superclass and then executes the rest of the constructor body). And indeed in your example you don't take this into account. However in the code supplied by jarek this is not the case. He uses it as an extention to the constructor of the superclass. The question is more: as long as you don't use subclass constructor parameters in the init function, is there anything against using this? – Rik Schaaf Mar 01 '15 at 00:18
  • @polygenelubricants @ jarek Quote: "If the overriding method depends on any initialization performed by the subclass constructor, the method will not behave as expected." So only if the overriding method depends on initialization performed by the subclass constructor, it is a problem. The provided code doesn't depend on this and therefore it is ok to use. It is still a bad coding habit though: if the code is only used by yourself it is not a problem, because only you will override the parent class yourself. If you are creating an API however, people using it might get into trouble. – Rik Schaaf Apr 27 '15 at 16:26
2

You are right in that it might cause problems with a derived class whose instance variables are initialised in the constructor or when the instance is created. If you had this:

public class ConcreteMenu extends Menu {
 String firstItem = "Item1";

 protected void init() {
  addMenuItem(new MenuItem(firstItem));
  // ....
 }
}

Then the MenuItem would have null as it's constructor argument!

Calling non-final methods in constructors is a risky practise.

A simple solution could be to separate the construction and the initialisation, like so:

Menu menu = new ConcreteMenu();
menu.init();
andrewmu
  • 14,276
  • 4
  • 39
  • 37
0

As others mentioned, calling an overridable method from the constructor is entering a world of pain ...

Have you considered doing the initialization in the constructor itself?

public abstract class Menu { 
    public Menu() { 
        ....
    } 

    protected void addMenuItem(MenuItem menuItem) { 
        // some code... 
    } 
} 

public class ConcreteMenu extends Menu { 
    public ConcreteMenu() { 
        super();
        addMenuItem(new MenuItem("ITEM1")); 
        addMenuItem(new MenuItem("ITEM2")); 
        // .... 
    } 
} 
Eyal Schneider
  • 22,166
  • 5
  • 47
  • 78
  • Yeah, I just wanted to clearly tell that you have to create those menu items in every subclass by overriding init() method, well after responses i will probably create builder class that will construct object first and then initialize it... – Jarek Jul 27 '10 at 11:50