tags:

views:

268

answers:

7

Can anyone comment with their opinion on the best way to resolve the following issue? I've tried init blocks and initializing the field in the constructor, but neither get called before the overridden method tries to clear it.

 class TestRunner {
        public static void main(String[] args) {
         ftw ftw = new ftw();
         ftw.dontPanic();
        }
    }

    class wtf {

        wtf(){
         this.dontPanic();
        }

        void dontPanic() {
         System.out.println("super don't panic");
        }
    }

    class ftw extends wtf {
        final HashSet variableOfDoom = new HashSet();

        ftw(){
         super();
        }

        void dontPanic() {
         super.dontPanic();
         System.out.println("sub don't panic");
         variableOfDoom.clear(); //This line prints a null pointer exception, because the field hasn't been intialized yet.
        }
    }
+7  A: 

This is why they recommend you don't call non-final methods from inside a constructor. private methods are also safe, since they can't be overriden.

Assuming that you can't follow that sound advice, for some reason, here are some other observations that might help:

  • I'm assuming you simplified for the sake of the example, but the call to .clear() doesn't really accomplish anything in the example; you may be able to just delete it.
  • It's tough to say without seeing the actual program in total, but you may be able to skate by with assigning the variable a new HashSet instead of invoking .clear().
Hank Gay
That is correct, clear was just an example to throw the exception.
qbn
I'm accepting this answer because I wasn't aware that this was recommended practice. I've enabled inspections in IntelliJ to prevent us from doing this in the future. Thanks Hank!
qbn
you cannot assign a final variable (variableOfDoom in this case) a new HashSet anywhere outside the constructor (the dontPanic() method in this case). Is that what your second point is alluding to?
neesh
@neesh I was implying it might be reasonable to make it non `final`.
Hank Gay
Question Hank, do you have a link to any sort of documentation talking about calling non-final methods in constructors? Googling hasn't found me much, perhaps I'm using a bad search query though.
qbn
Here's a link: http://benpryor.com/blog/2008/01/02/dont-call-subclass-methods-from-a-superclass-constructor/More authoritative is this quote from Effective Java: **Constructors must not invoke overridable methods**, directly or indirectly. (1st edition, page 80, item 15: Design and Document for Inheritance or Else Prohibit It)
Hank Gay
Here's a couple links explaining it more: http://www.informit.com/articles/article.aspx?p=20521 and http://benpryor.com/blog/2008/01/02/dont-call-subclass-methods-from-a-superclass-constructor/
kenj0418
+1 for the Effective Java quote
matt b
+6  A: 

Here's the problem: Your execution flow looks like this:

main
->ftw()
  ->wtf()
    ->wtf.dontPanic()

But since method invocations are determined at runtime in Java, ftw.dontPanic() is the method that's really being called. And variableOfDoom won't be set until the constructor completes, but the constructor will never complete because of the exception.

The solution is not to do work in your constructor (at least not with non-private non-final methods).

Michael Myers
+1 faster than me on 30 sec.
Artem Barger
Yes I realize the issue and the exec flow, I'm just looking for suggestions on best practices to avoid this.
qbn
That would be the "don't call non-private non-final methods in a constructor" bit then.
Michael Myers
+2  A: 

The takeaway from this example is: super class initialization cannot depend on the child class being completely initialized.

If ftw did not extend wtf then it would be ok to assume that any initialization specified at the field definition will be done before the constructor is called. However, since ftw extends wtf, wtf must be fully initialized before any initialization can happen in ftw. Since part of wtf initialization depends on variableOfDoom in the sub class to have been initialized you are getting a null pointer exception.

The only way out of this is for you to separate your call to dontPanic outside the constructor.

neesh
A: 

Don't call non-final methods from within constructors - not all instance variables may be initialized yet - your code being a classic example.

Without know what you're really trying to accomplish, its difficult to recommand "the best way to resolve the issue", but here's a go:

class TestRunner {
    public static void main(String[] args) {
            ftw ftw = new ftw();
            ftw.dontPanic();
    }
}

class wtf {

    wtf(){
            wtfDontPanic();
    }

    private final void wtfDontPanic() {
            System.out.println("super don't panic");
    }

    void dontPanic() {
            wtfDontPanic();
    }
}

class ftw extends wtf {
    final HashSet variableOfDoom = new HashSet();

    ftw(){
            super();
            ftwDontPanic();
    }

    private final ftwDontPanic() {
            System.out.println("sub don't panic");
            variableOfDoom.clear(); //This line prints a null pointer exception, because the field hasn't been intialized yet.
    }

    private void dontPanic() {
            super.DontPanic();
            ftwDontPanic();
    }
}
Bert F
+1  A: 

You might note that private methods are not prevented from calling overrides; so even then, exercise some caution in calling anything that is overridden from constructors. In other languages the advice is to avoid calling any virtual methods from a constructor because the object may not be fully constructed when the method is called.

An alternative to consider is two-stage construction. http://www.artima.com/forums/flat.jsp?forum=106&thread=106524

There's a related idiom in the .NET Framework Design Guidelines book called "create-set-call". Create the object, set some properties, call some methods. I've seen this usage sometimes criticized as being less discoverable than the simple construction call (i.e., just create-and-use).

devgeezer
A: 

quick & dirty solution (not very nice, I know)

...
System.out.println("sub don't panic");
// must test if object fully initialized, method called in constructor
if (variableOfDoom != null) {
    variableOfDoom.clear();
}
Carlos Heuberger
And variableOfDoom can still be final.
Peter Lawrey
Why the downvote (after 6 months)?
Carlos Heuberger
+1  A: 

Firstly, don't do that.

Secondly, if you absolutely have to do that, do this:

class wtf {

    wtf(){
            this.dontPanic();
    }

    void dontPanic() {
            System.out.println("super don't panic");
    }
}

class ftw extends wtf {
    HashSet _variableOfDoom; // underscore to remind you not to access it directly

    ftw(){
            super();
    }

    private HashSet getVariableOfDoom()
    {
        if (_variableOfDoom == null) _variableOfDoom = new HashSet();
        return _variableOfDoom;
    }

    void dontPanic() {
            super.dontPanic();
            System.out.println("sub don't panic");
            getVariableOfDoom().clear(); // FOR GREAT JUSTICE!
    }
}

Note that you can't have variableOfDoom be final.

Tom Anderson
Not sure why you would create a HashSet just so it can be cleared. I would check for a null value and assume it does need to be cleared if it hasn't been initialised.
Peter Lawrey