What's wrong with overridable method calls in constructors?
Asked Answered
M

8

405

I have a Wicket page class that sets the page title depending on the result of an abstract method.

public abstract class BasicPage extends WebPage {

    public BasicPage() {
        add(new Label("title", getTitle()));
    }

    protected abstract String getTitle();

}

NetBeans warns me with the message "Overridable method call in constructor", but what should be wrong with it? The only alternative I can imagine is to pass the results of otherwise abstract methods to the super constructor in subclasses. But that could be hard to read with many parameters.

Morrell answered 4/8, 2010 at 9:41 Comment(3)
I'm a .NET developer, but saw this one and was interested as to why it would warn against this because I do a similar thing at times in C#. This article seems to give an indication regarding why it is a warning: javapractices.com/topic/TopicAction.do?Id=215 So it's all to do with when and the order in which object hierarchy is inialised.Chiao
In C# we have the same problem: msdn.microsoft.com/en-us/library/ms182331.aspxGeographical
This reminds me to check if IntelliJ gives this warning...Tophet
B
513

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.

Related questions

See also


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.

See also

Related questions

Bushwhack answered 4/8, 2010 at 9:51 Comment(11)
+1. Interesting. I wonder whether object initializers in C# render both telescoping constructors and the Builder pattern unnecessary.Veterinary
@Johannes: In Java, instance initializer is executed at step 4, after superclass constructor at step 3, upon creation of new instances java.sun.com/docs/books/jls/third_edition/html/… ; I'm not sure if that addresses your comment though.Bushwhack
I'm wondering if there is a special handling for enums as this seem to work fine????Frasch
For the telescope constructor, I would not hard-code the defaults for every constructor. Store those in static constants at least, or in a file...Chlorpromazine
That said, it's too bad Java doesn't do a 2 phase initialization: 1st pass for method definitions, 2nd pass for execution of constructor. Now I gotta write more code for some factory-factory pattern or other. Bleh. All I wanted was to set some default data from a pure function that could be swapped in a subclass, or updated between construct and use.Barmen
Android engineers be aware: android view's overridable method invalidate() is sometimes called in view's constructor.Disassemble
FYI: the quoted sentence "If you violate this rule, program failure will result." is an outright lie. It is, however, somewhat more likely to result in the future.Guesswork
@immibis Sometimes the truth won't get the message across. I imagine a 'gentle warning' could result in more people thinking they know what they're doing (when they don't) and just do it.Moslemism
Stepping through the Java <init> code with a debugger used to be a surreal experience. My simple view is that (all) daughter-class(-es) are NOT initialised at the time-of parent constructor execution. However there are work-arounds. My favourite is to have a private static method which you may call from the constuctor, that can then call your @Override method because the <init> is complete at that time.Dworman
Playing devil's advocate: How is builder better compared to calling constructor and then set of setters?Flasher
Not allowing to invoke overridable methods from a constructor hinders the implementation of a template-like factory pattern, which is described here Factory Pattern in Wikipedia .Santosantonica
T
63

Here's an example which helps to understand this:

public class Main {
    static abstract class A {
        abstract void foo();
        A() {
            System.out.println("Constructing A");
            foo();
        }
    }

    static class C extends A {
        C() { 
            System.out.println("Constructing C");
        }
        void foo() { 
            System.out.println("Using C"); 
        }
    }

    public static void main(String[] args) {
        C c = new C(); 
    }
}

If you run this code, you get the following output:

Constructing A
Using C
Constructing C

You see? foo() makes use of C before C's constructor has been run. If foo() requires C to have a defined state (i.e. the constructor has finished), then it will encounter an undefined state in C and things might break. And since you can't know in A what the overwritten foo() expects, you get a warning.

Toggery answered 4/8, 2010 at 10:0 Comment(0)
B
12

Invoking an overridable method in the constructor allows subclasses to subvert the code, so you can't guarantee that it works anymore. That's why you get a warning.

In your example, what happens if a subclass overrides getTitle() and returns null ?

To "fix" this, you can use a factory method instead of a constructor, it's a common pattern of objects instanciation.

Basement answered 4/8, 2010 at 9:49 Comment(2)
returning null is a general problem breaking many interfaces.Morrell
returning null is a special problem when it happens in overridden methods which get called by the super constructor.Zygophyte
F
9

Here is an example that reveals the logical problems that can occur when calling an overridable method in the super constructor.

class A {

    protected int minWeeklySalary;
    protected int maxWeeklySalary;

    protected static final int MIN = 1000;
    protected static final int MAX = 2000;

    public A() {
        setSalaryRange();
    }

    protected void setSalaryRange() {
        throw new RuntimeException("not implemented");
    }

    public void pr() {
        System.out.println("minWeeklySalary: " + minWeeklySalary);
        System.out.println("maxWeeklySalary: " + maxWeeklySalary);
    }
}

class B extends A {

    private int factor = 1;

    public B(int _factor) {
        this.factor = _factor;
    }

    @Override
    protected void setSalaryRange() {
        this.minWeeklySalary = MIN * this.factor;
        this.maxWeeklySalary = MAX * this.factor;
    }
}

public static void main(String[] args) {
    B b = new B(2);
    b.pr();
}

The result would actually be:

minWeeklySalary: 0

maxWeeklySalary: 0

This is because the constructor of class B first calls the constructor of class A, where the overridable method inside B gets executed. But inside the method we are using the instance variable factor which has not yet been initialized (because the constructor of A has not yet finished), thus factor is 0 and not 1 and definitely not 2 (the thing that the programmer might think it will be). Imagine how hard would be to track an error if the calculation logic was ten times more twisted.

I hope that would help someone.

Foetus answered 22/1, 2015 at 16:28 Comment(0)
R
4

If you call methods in your constructor that subclasses override, it means you are less likely to be referencing variables that don’t exist yet if you divide your initialization logically between the constructor and the method.

Have a look on this sample link http://www.javapractices.com/topic/TopicAction.do?Id=215

Roybn answered 4/8, 2010 at 9:44 Comment(0)
G
3

I certainly agree that there are cases where it is better not to call some methods from a constructor.

Making them private takes away all doubt: "You shall not pass".

However, what if you DO want to keep things open.

It's not just the access modifier that is the real problem, as I tried to explain here. To be completely honest, private is a clear showstopper where protected usually will still allow a (harmful) workaround.

A more general advice:

  • don't start threads from your constructor
  • don't read files from your constructor
  • don't call APIs or services from your constructor
  • don't load data from a database from your constructor
  • don't parse json or xml documents from your constructor

Don't do so (in)directly from your constructor. That includes doing any of these actions from a private/protected function which is called by the constructor.

Calling an start() method from your constructor could certainly be a red flag.

Instead, you should provide a public init(), start() or connect() method. And leave the responsibility to the consumer.

Simply put, you want to separate the moment of "preparation" from the "ignition".

  • if a constructor can be extended then it shouldn't self-ignite.
  • If it self-ignites then it risks being launched before being fully constructed.
  • After all, some day more preparation could be added in the constructor of a subclass. And you don't have any control over the order of execution of the constructor of a super class.

PS: consider implementing the Closeable interface along with it.

Gringo answered 22/9, 2021 at 11:0 Comment(0)
H
2

In the specific case of Wicket: This is the very reason why I asked the Wicket devs to add support for an explicit two phase component initialization process in the framework's lifecycle of constructing a component i.e.

  1. Construction - via constructor
  2. Initialization - via onInitilize (after construction when virtual methods work!)

There was quite an active debate about whether it was necessary or not (it fully is necessary IMHO) as this link demonstrates http://apache-wicket.1842946.n4.nabble.com/VOTE-WICKET-3218-Component-onInitialize-is-broken-for-Pages-td3341090i20.html)

The good news is that the excellent devs at Wicket did end up introducing two phase initialization (to make the most aweseome Java UI framework even more awesome!) so with Wicket you can do all your post construction initialization in the onInitialize method that is called by the framework automatically if you override it - at this point in the lifecycle of your component its constructor has completed its work so virtual methods work as expected.

High answered 5/5, 2017 at 22:10 Comment(0)
P
0

I guess for Wicket it's better to call add method in the onInitialize() (see components lifecycle) :

public abstract class BasicPage extends WebPage {

    public BasicPage() {
    }

    @Override
    public void onInitialize() {
        add(new Label("title", getTitle()));
    }

    protected abstract String getTitle();
}
Puckery answered 9/12, 2016 at 11:18 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.