What's Wrong With Overridable Method Calls in Constructors

What's wrong with overridable method calls in constructors?

On invoking overridable method from constructors

Simply put, this is wrong because it unnecessarily opens up possibilities to MANY bugs. When the @Override is invoked, the state of the object may be inconsistent and/or incomplete.

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.

On object construction with many parameters

Constructors with many parameters can lead to poor readability, and better alternatives exist.

Here's a quote from Effective Java 2nd Edition, Item 2: Consider a builder pattern when faced with many constructor parameters:

Traditionally, programmers have used the telescoping constructor pattern, in which you provide a constructor with only the required parameters, another with a single optional parameters, a third with two optional parameters, and so on...

The telescoping constructor pattern is essentially something like this:

public class Telescope {
    final String name;
    final int levels;
    final boolean isAdjustable;

    public Telescope(String name) {
        this(name, 5);
    }
    public Telescope(String name, int levels) {
        this(name, levels, false);
    }
    public Telescope(String name, int levels, boolean isAdjustable) {       
        this.name = name;
        this.levels = levels;
        this.isAdjustable = isAdjustable;
    }
}

And now you can do any of the following:

new Telescope("X/1999");
new Telescope("X/1999", 13);
new Telescope("X/1999", 13, true);

You can't, however, currently set only the name and isAdjustable, and leaving levels at default. You can provide more constructor overloads, but obviously the number would explode as the number of parameters grow, and you may even have multiple boolean and int arguments, which would really make a mess out of things.

As you can see, this isn't a pleasant pattern to write, and even less pleasant to use (What does "true" mean here? What's 13?).

Bloch recommends using a builder pattern, which would allow you to write something like this instead:

Telescope telly = new Telescope.Builder("X/1999").setAdjustable(true).build();

Note that now the parameters are named, and you can set them in any order you want, and you can skip the ones that you want to keep at default values. This is certainly much better than telescoping constructors, especially when there's a huge number of parameters that belong to many of the same types.

Calling an overridable method in constructor is bad. Are there exceptions?

I would recommend you to use a factory method in this case.

Write a static method called something like

public static IdentifiedConnection openConnection(String serverAddress, int port)
        throws IOException {
    ...
}

This allows for better control of the creation and avoids the potential problems with leaking references to uninitialized objects.

Another approach would be to take a Supplier<UUID> as argument to the constructor, as follows:

abstract class IdentifiedConnection extends Connection {

    private UUID uuid;

    public IdentifiedConnection(String serverAddress,
                                int port,
                                Supplier<UUID> uuidSupplier) throws IOException {
        super(serverAddress, port);
        this.uuid = uuidSupplier.get();
    }
}

and in a subclass use it as

class SomeConnection extends IdentifiedConnection {

    public SomeConnection(String serverAddress, int port) throws IOException {
        super(serverAddress, port, SomeConnection::createUUID);
    }

    public static UUID createUUID() {
        return ...;
    }

}

the issues of calling overridable methods from constructor

You have to first make a distinction between instantiation and initialization. Instantiation is the process of creating an instance of a type (allocating the space for it and getting a reference to that space). Initialization is the process of setting the state of the instance to its initial value.

Take the following type hierarchy:

class Foo { 
    public Foo() {}
}
class Bar extends Foo {
    public Bar() {super();}
}

New instance creation expressions

new Bar();

cause instantiation and initialization. Instantiation happens first. Java creates an instance of concrete type Bar.

Then initialization needs to take place. In an inheritance hierarchy, the initialization follows the same hierarchy, but top to bottom.

Object
   |
  Foo 
   |
  Bar

The constructor for Object runs first to initialize the state that is defined as part of Object, then the constructor for Foo is run to initialize the state that is defined as part of Foo and finally the constructor for Bar is run to initialize the state defined in Bar. Your instance is still of type Bar. So polymorphism still applies. If you invoke an instance method and that method is overriden somewhere lower in the hierarchy, that implementation will be invoked.

That's what that quote is referring to. It's dangerous. Read more here:

What's wrong with overridable method calls in constructors?

Overridable method call in constructor - what is the proper way to call the method?

The compiler warning notification somewhat says what can happen. If a subclass overrides the doSomething method, it can change the behavior when creating the class instance, which may be dangerous or not, depending on your design. Note that it's a warning, not a compiler exception.

To prove this, just extended your code to test it:

class SomeClass {

    public SomeClass() {

        doSomething();//Warning - Overridable method call in constructor
        SomeClass.this.doSomething();//Seems OK, but is not
    }

    public void doSomething() {
        System.out.println("parent");
    }
}

public class SomeOtherClass extends SomeClass {
    @Override
    public void doSomething() {
        System.out.println("child");
    }

    public static void main(String[] args) {
        SomeClass a = new SomeClass();
        SomeOtherClass b = new SomeOtherClass();
    }
}

Prints:

parent
parent
child
child

If you just want that no other class can override doSomething method, mark it as final:

class SomeClass {

    public SomeClass() {

        doSomething();//Warning - Overridable method call in constructor
        SomeClass.this.doSomething();//Seems OK
    }

    public final void doSomething() {
        System.out.println("parent");
    }
}

Then if any subclass tries to override it, the compiler will throw an error:

public class SomeOtherClass extends SomeClass {
    @Override
    public void doSomething() { //compiler error
        System.out.println("child");
    }
    //...
}

Calling overridable methods like Swing's add() in constructor

To avoid wiring Swing components together in the constructor, you could simply give the responsibility of the wiring to another object. For instance, you could give wiring duties to a Factory:

public class MyPanelFactory {
    public MyBasePanel myBasePanel() {
        MyBasePanel myBasePanel = new MyBasePanel();
        initMyBasePanel(myBasePanel);
        return myBasePanel;
    }

    public MyDerivedPanel myDerivedPanel() {
        MyDerivedPanel myDerivedPanel = new MyDerivedPanel();
        initMyBasePanel(myDerivedPanel);
        return myDerivedPanel;
    }

    private void initMyBasePanel(MyBasePanel myBasePanel) {
        myBasePanel.add(new JLabel("My label"), BorderLayout.CENTER);
    }
}

Or you could go all out and instantiate all your Swing components with a dependency injection container and have the container trigger the wiring. Here's an example with Dagger:

@Module
public class MyPanelModule {
    static class MyBasePanel extends JPanel {
        private final JLabel myLabel;

        MyBasePanel(JLabel myLabel) {
            this.myLabel = myLabel;
        }

        void initComponents() {
            this.add(myLabel, BorderLayout.CENTER);
        }
    }

    static class MyDerivedPanel extends MyBasePanel {
        private final List<JLabel> addedLabels = new ArrayList<>();

        MyDerivedPanel(JLabel myLabel) {
            super(myLabel);
        }

        @Override
        public void add(Component comp, Object constraints) {
            super.add(comp);
            if (comp instanceof JLabel) {
                JLabel label = (JLabel) comp;
                addedLabels.add(label);
            }
        }
    }

    @Provides MyBasePanel myBasePanel(@Named("myLabel") JLabel myLabel) {
        MyBasePanel myBasePanel = new MyBasePanel(myLabel);
        myBasePanel.initComponents();
        return myBasePanel;
    }

    @Provides MyDerivedPanel myDerivedPanel(@Named("myLabel") JLabel myLabel) {
        MyDerivedPanel myDerivedPanel = new MyDerivedPanel(myLabel);
        myDerivedPanel.initComponents();
        return myDerivedPanel;
    }

    @Provides @Named("myLabel") JLabel myLabel() {
        return new JLabel("My label");
    }
}

Why do not call overridable methods in constructors?

As T.S. said (thank you T.S.), the base constructor will always be called, when instantiating the derived type. If you do as I did in the question, where the derivative constructor does not explicitly specify which base constructor to use, then the parameterless base constructor is used.

public HttpUrl()           // : base() is implied.

And

public HttpUrl(string Url) // : base() is still implied.  
                           // Parameterless. Not :base(Url)

So the complete answer to this question is: The abstract or virtual methods declared in the base class are only overridable in the base class, if you sealed the derivative class. So to make clean code, you don't run this line in the base constructor:

this.Url = Url;   // Don't do this in the base constructor

You instead, should "sealed" the derivative class (or method). As follows:

public abstract class SimpleUrl
{
    protected string _url;
    public abstract string Url { get; set; }
    // Optionally declare base constructors that do *not* call Url.set
    // Not sure why these constructors are optional in the base, but
    // required in the derivative classes.  But they are.
    public SimpleUrl()
    { }
}

public sealed class HttpUrl : SimpleUrl
{
    public HttpUrl()   // Not sure why this is required, but it is.
    { }
    public HttpUrl(string Url)
    {
        // Since HttpUrl is sealed, the Url set accessor is no longer
        // overridable, which makes the following line safe.
        this.Url = Url;
    }
    public override string Url
    {
        get
        {
            return this._url;
        }
        set
        {
            if (value.StartsWith("http://"))
                this._url = value;
            else
                throw new ArgumentException();
        }
    }
}

Alternatively, you don't need to seal the entire derivative class, if you just seal the overridable method (making it no longer overridable by any further derivative classes)

public class HttpUrl : SimpleUrl
{
    // ...
    public override sealed string Url
    // ...

Calling an overridable method in constructor

//do some parsing on string and set up the BoardImpl properties

The method should be responsible to convertFromString only.

1) Make the method final

public class BoardImpl implements Board{

  public void final convertFromString(String formattedString)
  {
        //do some parsing on string and set up the BoardImpl properties
  }

}

2) Solution make an abstract class and call in superClass constructor so you don't have to call in each subclass BUT don't use properties from subclass cause they aren't intilized.

public abstract class AbstractBoard implements Board{

     public AbstractBoard(String s){
       convertFromString(s);
     }

}

3) And My preferred one make something with composition

public class Client {

 private Board board;

  public Client(String s){
   board.convertFromString(s);
  }

 public void setBoard(Board board){
      this.board = board;
 } 

}

Then in the board you can delegate responsability of deciding wich Board you should use to a factory or if it has no state a FlyweightFactory

What's Wrong With Overridable Method Calls in Constructors