What's wrong with overridable method calls in constructors?

У меня есть класс Wicket page, который задает заголовок страницы в зависимости от результата абстрактного метода.

public abstract class BasicPage extends WebPage {

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

    protected abstract String getTitle();

}

NetBeans предупреждает меня сообщением "Переопределяемый вызов метода в конструкторе", но что в этом должно быть не так? Единственная альтернатива, которую я могу себе представить, - это передавать результаты других абстрактных методов суперконструктору в подклассах. Но это может быть трудно прочитать со многими параметрами.

О вызове переопределяемого метода из конструкторов

Проще говоря, это неправильно, потому что это без необходимости открывает возможности для МНОГИХ ошибок. Когда @Override вызывается, состояние объекта может быть непоследовательным и / или неполным.

Цитата из Effective Java 2nd Edition, пункт 17: Разрабатывайте и документируйте для наследования, иначе запретите это:

Есть еще несколько ограничений, которым класс должен подчиняться, чтобы разрешить наследование. Конструкторы не должны вызывать переопределяемые методы, прямо или косвенно. Нарушение этого правила приведет к сбою программы. Конструктор суперкласса запускается раньше конструктора подкласса, поэтому переопределяющий метод в подклассе будет вызван до запуска конструктора подкласса. Если переопределяющий метод зависит от какой-либо инициализации, выполняемой конструктором подкласса, метод не будет вести себя так, как ожидалось.

Вот пример для иллюстрации:

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"
    }
}

Здесь, когда Base конструктор вызывает overrideMe, Child не завершена инициализация final int x, и метод получает неправильное значение. Это почти наверняка приведет к ошибкам.

Вопросы по теме

• Вызов переопределенного метода из конструктора родительского класса


• Состояние объекта производного класса, когда конструктор базового класса вызывает переопределенный метод в Java


• Использование абстрактной функции init() в конструкторе абстрактного класса

Смотрите также

• FindBugs - Неинициализированное чтение метода field, вызываемого из конструктора суперкласса

О построении объекта со многими параметрами

Конструкторы со многими параметрами могут привести к ухудшению читаемости, и существуют альтернативы получше.

Вот цитата из Effective Java 2nd Edition, пункт 2: Рассмотрите шаблон builder, когда сталкиваетесь со многими параметрами конструктора:

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

• Wikipedia/Builder pattern


• Effective Java 2nd Edition, Item 2: Consider a builder pattern when faced with many constructor parameters (excerpt online)

Related questions

• When would you use the Builder Pattern?


• Is this a well known design pattern? What is its name?

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.

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.

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.