14

I recently spent quite a few minutes debugging a problem in production code that in the end turned out to be caused by a class calling an abstract method in its constructor, and the subclass implementation of that method tried to use a subclass field that had not been initialized yet (An example that illustrates the point is included below)

While researching this, I stumbled across this question, and was intrigued by Jon Skeet's answer:

In general it's a bad idea to call a non-final method within a constructor for precisely this reason - the subclass constructor body won't have been executed yet, so you're effectively calling a method in an environment which hasn't been fully initialized.

This has me wondering, is there ever a legitimate reason to call a non-final or abstract method from a constructor? Or is it pretty much always a sign of bad design?

Example

public class SSCCE {
    static abstract class A {
        public A() {
            method();  // Not good; field arr in B will be null at this point!
        }

        abstract void method();
    }

    static class B extends A {
        final String[] arr = new String[] { "foo", "bar" };

        public B() {
            super();
            System.out.println("In B(): " + Arrays.toString(arr));
        }

        void method() {
            System.out.println("In method(): " + Arrays.toString(arr));
        }
    }

    public static void main(String[] args) {
        new B().method();
    }
}

And here is expected output:

In method(): null
In B(): [foo, bar]
In method(): [foo, bar]

The problem, of course, is that in the first call to method() the field arr is null because it hasn't been initialized yet.

Community
  • 1
  • 1
Kevin K
  • 9,344
  • 3
  • 37
  • 62
  • 1
    I assume, for the purpose of this discussion, you are not worrying about **static** methods. So the question is "Is it ever appropriate to call a non-final, non-static method from a constructor." – John B Sep 19 '11 at 21:40
  • Take a look at this answer: http://stackoverflow.com/questions/3288601/problem-in-instance-variable-initialization/3288873#3288873 – Zaki Sep 19 '11 at 21:41
  • @Zaki this shows an excellent example of a derived class calling code in the base but not the other way around. Where the base class calls an abstract (or non-final) method from its constructor. – John B Sep 19 '11 at 21:45
  • @JohnB Yes, I'm referring specifically to non-static methods. – Kevin K Sep 19 '11 at 22:44
  • @Zaki It looks like ultimately they solved the problem by moving the method call out of the constructor. Interesting, but my question is if there are situations where calling the method from the constructor is either required or difficult to avoid. – Kevin K Sep 19 '11 at 22:48

4 Answers4

12

There are times it can be very hard not to.

Take Joda Time, for example. Its Chronology type hierarchy is very deep, but the abstract AssembledChronology class is based on the idea that you assemble a bunch of "fields" (month-of-year etc). There's a non-final method, assembleFields, which is called during the constructor, in order to assemble the fields for that instance.

They can't be passed up the constructor chain, because some of the fields need to refer back to the chronology which creates them, later on - and you can't use this in a chained constructor argument.

I've gone to nasty lengths in Noda Time to avoid it actually being a virtual method call - but it's something remarkably similar, to be honest.

It's a good idea to avoid this sort of thing if you possibly can... but sometimes it's a real pain in the neck to do so, especially if you want your type to be immutable after construction.

Didier L
  • 18,905
  • 10
  • 61
  • 103
Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • What about keeping the subclass constructor code in the constructor, but just in the subclass's constructor? Still immutable after construction; with the only caveat that the subclass constructor must set it's own subclass specific stuff (or risk being poorly initialized). – Edwin Buck Sep 19 '11 at 21:55
7

One example is the non-final (and package-private) method HashMap#init(), an empty method which is in place for the exact purpose of being overriden by subclasses:

/**
 * Initialization hook for subclasses. This method is called
 * in all constructors and pseudo-constructors (clone, readObject)
 * after HashMap has been initialized but before any entries have
 * been inserted.  (In the absence of this method, readObject would
 * require explicit knowledge of subclasses.)
 */
void init() {
}

(from the HashMap source)

I don't have any examples of how it's used by subclasses - if anyone does, feel free to edit my answer.

EDIT: To respond to @John B's comment, I'm not saying it must be good design since it's used in the source. I just wanted to point out an example. I do notice that each HashMap constructor takes care to call init() last, but this is of course still before the subclass constructor. So an amount of responsibility is falling to the subclass implementation not to muck things up.

Community
  • 1
  • 1
Paul Bellora
  • 54,340
  • 18
  • 130
  • 181
  • 3
    +1 for finding example. But this being in the base does not mean that it is good design. I definitely see the risk in calling a subclasses instance method prior to the subclasss' constructor being executed. There are assumptions of initialization that are too easy to make. – John B Sep 19 '11 at 21:42
  • 3
    This is interesting. It seems it's necessary because of the constructor where you provide an existing `Map` and the constructor will add the entries from the existing map - `init()` must be called before adding any entries. At least they made the method package-private so they have some control over it. I can see `LinkedHashMap` overrides it to initialize the linked list header, as an example. – Kevin K Sep 19 '11 at 22:24
3

Generally, it's not good design to call methods on a class before it is constructed; however, Java allows exceptions in the case you know what you are doing (ie. you don't access uninitialized fields). With an abstract method, I don't think it is possible to "know" what you're doing in the parent class.

The above code can easily be solved by imposing stricter interpretation of "a class handles it's responsibilities." It's not the super class's responsibility to initialize the subclass, so it shouldn't be the super class's prerogative to call subclass code before such initialization might be complete.

Yes, it's done in the JDK (like the HashMap code) with special "init()" methods that imply initialization of all subclass code; but, I would offer that the following pattern of calls is much cleaner and permits more flexibility.

public class SSCCE {
    static abstract class A {
        public A() {

        }

        abstract void method();
    }

    static class B extends A {
        final String[] arr = new String[] { "foo", "bar" };

        public B() {
            super();
            method();
            System.out.println("In B(): " + Arrays.toString(arr));
        }

        void method() {
            System.out.println("In method(): " + Arrays.toString(arr));
        }
    }

    public static void main(String[] args) {
        new B().method();
    }
}

it just seems so much cleaner, in so many ways. Failing that, there's always the ability to construct your objects with proper "initialization sequence" through a Factory.

Edwin Buck
  • 69,361
  • 7
  • 100
  • 138
  • The example code was more of a "here's what can go wrong" sort of example, rather than a "how can I fix this" example, but your solution is a good one. – Kevin K Sep 19 '11 at 22:32
  • I follow your point, and the "how to fix this" may have stolen some of my thunder from the opening statement. It's not safe to call methods on an object that's not fully constructed. It's even worse to do it when you don't know (via abstraction) what those methods do. – Edwin Buck Sep 19 '11 at 22:34
  • Though I would say either `B` or `B.method()` should be declared final, otherwise the problem is simply extended if you declare `C extends B` and override method again. – Kevin K Sep 19 '11 at 22:37
  • @Kevin, You're right, and it's just more proof that (in general) calling methods in the constructor is just a bad idea. Yes, it can be done safely if you manually verify how it's going to be done is safe; but, if you open the door for someone else to abuse it, such safety is lost. Final methods? Seems like a solution at the cost of allowing flexibility in sub-classing which is at conceptual odds with what virtual permits. Final class? A better solution, but the interface cannot demand subclasses be declared final. Perhaps it's better to avoid creating the issue than to fix it. – Edwin Buck Sep 19 '11 at 22:52
  • This doesn't work for final fields in `A` which should be initialized based on the `B.method()`. – maaartinus Sep 15 '14 at 22:02
-1

One very useful pattern is to call abstract (or overridden) createX methods. This allows subclasses to affect the configuration of the base class.

Duncan McGregor
  • 17,665
  • 12
  • 64
  • 118