Wicket and the 'constructor calls overridable method' PMD warning

We've been avoiding this PMD warning by moving most of our constructor code into onInitialize(). But are we just moving the problem (design flaw?) into a difference place?

i.e. is our onInitialize() just a surrogate constructor, that isn't noticed by PMD?

We are having problems of the sort that pop up when you call overridable methods in a constructor, but that seems to stem from the fact that Wicket itself calls one (can't find the exact source line, but onInitialize(), an overridable method, ends up getting called when you call add() in a constructor).

Happy to provide sample code if it'll help.

public class PageA extends WebPage {

    protected SomeBean bean;

    public PageA() {
        add(new Label("foo", "bar"));
        bean = new SomeBean();
    }

}

public class PageB extends PageA {

    public PageB() {
        super();
    }

    @Override
    protected void onInitialize() {
        add(new Label("rofl", bean.getSomeText()));
    }
}

Yo开发者_JS百科u would expect this to be fine but the call to onInitialize does not happen where you may think it does:

When calling add() on a page the method flow is:

MarkupContainer    add()    
MarkupContainer    addedComponent() 
Page               componentAdded()
MarkupContainer    initialize()
Component          fireInitialize()
Component          onInitialize()

So as you can see if you add a component to a WebPage the onInitialize() method is fired, which is an overridable method leading to instances of the above looking sane code creating NullPointerExceptions.

The only warning you get that this may happen is the JavaDoc of onInitialize():

NOTE:The timing of this call is not precise, the contract is that it is called sometime before {@link Component#onBeforeRender()}.

If you only add components to a container from inside its onInitialized() method, this problem won't arise. But it can't be verified by PMD, at least not by the built-in rules.

I don't think it's a design flaw, though. It's a design decision. You just can't base all your design on static analysis tools and predefined rules. API usability is also an important aspect of design, sometimes even more relevant than design principles.

For example, the CQS (Command-Query Separation) principle would mandate that a method that does something (changes state) should not return anything, and methods that return something shouldn't have any side effects (change state).

If this was a hard rule, fluent interfaces (methods that change the object's state, and return this, allowing method chaining) couldn't be implemented. Wicket uses it extensively (almost all component manipulation methods return this), and this is one of the things that makes it a joy to use.

PMD is a very useful tool. But you must be the tool's master, not its slave. You should consider its warnings as possible issues, but if you are confident of your design choices, just mark the code to be bypassed and be happy.

Consider the following class:

public class A {

    public A() {
        System.out.println(val().toString());
    }

    protected Integer val() {
        return 0;
    }

}

It looks fine at first sight, assume val() should never return null, which is the case. Now B subclasses A:

public class B extends A {

    private final Integer i;

    public B() {
        //super();    //implicit
        i = 1;
    }

    @Override
    protected Integer val() {
        return i;
    }
}

Class B is fine at first sight as well - it returns i from val() which is never null since it's final at it is initialized in constructor. However creating B instance will throw NullPointerException. Can you see why? Hint: look where implicit super() takes place.

Do you think moving i initialization will help? Why not?

private final Integer i = 1;

As a rule of thumb, never call non-private methods from constructor of non-final class. In fact I would even trigger compilation error in this case. As you can see this problem has nothing to do with Wicket and moving initialization to onInitialize() is to avoid such traps.

We are having problems of the sort that pop up when you call overridable methods in a constructor, but that seems to stem from the fact that Wicket itself calls one (can't find the exact source line, but onInitialize(), an overridable method, ends up getting called when you call add() in a constructor).

I'm pretty sure the onInitialize() method that's called when you call add(component) is the onInitialize() method of the component being added (the argument to the add method) and not of the class you're currently constructing. This should be fine, as that component has already been completely constructed.