3

I'm trying to clean the code of a class that use initialization bloks in a manner I would never do, and I'm just wondering if I am missing some informations. The code looks like this:

@Entity
class MyClass extends BaseClass {

    @ManyToMany(fetch=FetchType.EAGER)
    private Set<OherClass> others;

    {
        if (others == null)
            others = new HashSet<OtherClass>();
    }

    public MyClass(){
        super();
    }

    //getters, setters and other stuff follows
}

I think there is no reason to prefer the above code against this:

@Entity
class MyClass extends BaseClass {

    @ManyToMany(fetch=FetchType.EAGER)
    private Set<OherClass> others = new HashSet<OtherClass>();
}

Or this:

@Entity
class MyClass extends BaseClass {

    @ManyToMany(fetch=FetchType.EAGER)
    private Set<OherClass> others;

    public MyClass(){
        this.others = new HashSet<OtherClass>();
    }
}

I asked my college, but the only thing he was able to answer is how initialization block works and other things I already know. I wonder if there are some subtle misbehaviour of java (even old one already fixed) or frameworks (hibernate, spring) in case of serialization, reflection, database persistence, injection or any unusual situation that could make that code necessary.

holap
  • 438
  • 2
  • 18

2 Answers2

5
private Set<OherClass> others;
{
    if (others == null)
        others = new HashSet<OtherClass>();
}

The above code was written without an understanding of Java semantics. others == null will always be true. Therefore this is nothing but a very convoluted and confused way of writing

private Set<OherClass> others = new HashSet<OtherClass>();

Hibernate will indeed wrap some "magic" around the object construction, but it will still need to obtain an instance from the default constructor. At that point all the instance initializers have run.

On a more general note, always prefer to immediately initialize collection-valued variables with constant expressions and, even better, make the field final. That's one less worry for the rest of your code.

With the above you still have all the options open on how to populate the set, which can differ from constructor to constructor.

Marko Topolnik
  • 195,646
  • 29
  • 319
  • 436
  • Thank you, that is what I think too. But that code is so lame I just get uncertain and think there should be a very silly reason to make that. Or maybe I just hoped there is a reason to code that way. – holap Feb 07 '14 at 10:43
1

The only place I see this could be useful, is when the initialization of others needs more than one step and if in the constructor of BaseClass a method is invoked that may be overridden by a sub-class such as MyClass but you want to be sure that others is already properly initialized as it is used by this method. Sounds complicated. I hope that the Java code makes this more clear:

public class BaseClass {

    public BaseClass() {
        init();
    }

    public void init() {
        // do something...
    }

}

public class MyClass extends BaseClass {

    private Set<OtherClass> others;
    {
        others = new HashSet<OtherClass>();
        others.add(new OtherClass("hallo"));
        // or do more complicated stuff here
    }

    public MyClass() {
        super();  // explicit or may also be implicit
    }

    @Override
    public void init() {
        // use initialized others!!
        doSomething(this.others);
    }

    ....
}

However, this is a very, very bad pattern and you should never invoke a non-final or non-private method in your constructor as the sub-class may not be properly initialized yet.

Beside that, others is always null if not initialized and you don't have to test this.

Peter Keller
  • 7,526
  • 2
  • 26
  • 29
  • Good point. But as you mentioned calling a subclass method from the superclass constructor is definitely a very bad practice and one more reason to clean it. Thanks. – holap Feb 07 '14 at 11:00