0

When working on my company's legacy code, I encountered an NPE during runtime. After debugging it, this is what I encountered : 

public class ConcreteClass extends PreConcreteClass{
   private List<Object> internalDS = new ArrayList<>();
   public ConcreteClass() {
      super();
      ....
   }
   @Override
   protected void update() {
      ....
      for(Object o : internalDS) {
         ...
      }
      ...
}


public class PreConcreteClass extends AbstractClass{
   ......
   public PreConcreteClass() {
      super();
      ......
   }
   ......
}


protected abstract class AbstractClass {
    protected AbstractClass() {
       .....
       update();
       ....
    }
    protected void update() {
       .....
    }
}

The NPE was thrown when the overriding update method of the ConcreteClass was invoked, after the invocation of super from ConcreteClass and super from PreConcreteClass. The cause was internalDS - it was null, causing the for loop to threw the NPE.

First - this is against what I have always expected - that class fields which have been initialized at declaration are initialized before executing the constructor's scope. Is this not the case when calling the constructors of derived classes via super ?

Second - I solved the NPE by adding an init method which is called by the AbstractClass constructor, which is given an empty implementation by AbstractClass, and is overridden by the ConcreteClass with the initialization on internalDS.

I looked up some general advise in stack overflow. I had some discussions with my colleagues at work where we agreed that there is an inherit problem with the design above which caused the NPE. Since this is legacy code which we do not want to drastically change, I waned to know if anyone has a better alternative to the init method solution I used. Note - there are multiple constructors for each class.

Rann Lifshitz
  • 4,040
  • 4
  • 22
  • 42

1 Answers1

1

No, the compiler is moving those initializer in the constructor after the call to super, so your code is equivalent to that:

public class ConcreteClass extends PreConcreteClass{
   private List<Object> internalDS;
   public ConcreteClass() {
      super();
      internalDS = new ArrayList<>();
      ...
   }
   @Override
   protected void update() {
      ....
      for(Object o : internalDS) {
         ...
      }
      ...
}

Note that there is a general rule to avoid that in a cleaner way: Never call non-final methods in a constructor. It can screw things up.

Dorian Gray
  • 2,913
  • 1
  • 9
  • 25
  • As I suspected. Any thoughts on a better solution for initializing the DS, other then using an init method in AbstractClass ? – Rann Lifshitz Feb 24 '18 at 10:21
  • 1
    Apart from that general design flaw, using an init method seems to be fine. The whole design is somehow broken, but if you dont want to make any bigger changes to that, you can use an init method. See also http://stackoverflow.com/questions/3404301/whats-wrong-with-overridable-method-calls-in-constructors?rq=1 – Dorian Gray Feb 24 '18 at 10:27