Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

According to a well-accepted answer well-accepted answer the invocation of overridable methods within the constructor can easily get fixed using a builder pattern.

According to a well-accepted answer the invocation of overridable methods within the constructor can easily get fixed using a builder pattern.

According to a well-accepted answer the invocation of overridable methods within the constructor can easily get fixed using a builder pattern.

Source Link

Initial code review

  • You declared your NonInitializedInstanceException as RuntimeException and later declared your methods to throw this unchecked exception. Either change the excetpion type to Exception or remove the throws NonInitializedInstanceException from the methods.

  • I'm not sure how a class can be dependent on its name. If you meant that it needs to be specified requiredName would probably more suitable

  • Based on the API itself it is not clear if a method may be invoked while initialization or not. I doubt that a real business case would include a naming like youCanCallMeBeforeInitialization or overridablePrintName. Therefore a documentation overhead is mandatory which leads to an API that might be harder and more discouraging to use.

    I'm also not fully sure if at the given initialization time preventing overriden method invocations are necessary as first the super class constructor is executed, which does nothing, followed by the child constructor, which also does nothing. In a Java sense the object is now fully initialized. Afterwards the childs initialize method is invoked which invokes the super initialization method at some point. But as the child is already initialized in a Java sense, invoking any other method within the initializer method is admissible. An overriden method call within the initialization method will thus always return the correct overriden state.

Problem description

The actual problem you try to solve is to set state within the constructor which may not yet be available as the parent class might invoke the overridable method, but as the child is not yet initialized, the overridden method does not return what may be assumed to be returned leaving the currently initialized object in an inconsistent state.

Your code initializes a new object and afterwards invokes initialize which is basically a factory method which is similar to a builder but instead of returning a product reference it returns the reference of itself. As you already have a valid object through instantiating via new SuperClass() throwing a NonInitializedInstanceException may prevent further usage of the object, though overridable methods will always set the correct value as the object was already created.

Alternative approaches

As you asked for alternative approaches, I'll try to depict two other ways to fix the overridable method in constructor issue:

  • Parameter object
  • Builder pattern

Parameter object

In Java the constructor is usually used to initialize the object state. Delaying the initialization to a further method is possible but only useful in certain circumstances like de/serialization or some lazy initializations IMO.

If you don't need lazy initialization you may be better off with a parameter object, which can be a simple map or a specialized parameter class.

public class Sample {
 protected final SomeType someField;
 protected SomeOtherType someOtherField;
 
 public Sample(Map<String, Object> parameters) {
 if (parameters != null) {
 if (parameters.contains(KEY) {
 this.someField = (SomeType)parameters.get(KEY);
 } else if (parameters.contains(OTHER_KEY)) {
 this.someOtherField = (SomeOtherType)parameters.get(OTHER_KEY);
 }
 } else {
 throw new IllegalArgumentException("Could not initialize field 'someField'!");
 }
 }
}

An inheriting class could look like this:

public class SomeSample extends Sample {
 private final YetSomeOtherType yetSomeOtherField;
 public SomeSample(Map<String, Object> parameters) {
 super(parameters);
 if (parameters.contains(YET_OTHER_KEY)) {
 this.yetSomeOtherField = (YetSomeOtherType)parameters.get(YET_OTHER_KEY);
 this.someOtherField = null; // admissible
 } else {
 throw new IllegalArgumentException("Could not initialize field 'someOtherField'!");
 }
 }
}

Here, first the fields of the parent are initialized and for none final fields, the may even be updated like with someOtherField in the example below.

This approach also allows to set arguments via reflection easily. A sample method which could be put into the parent class may look like the one below:

protected final void setRequiredField(Field field, Map<String, Object> parameters, String key) {
 if (null == field) {
 throw new InvalidArgumentException("Invalid field to inject value into for key '" 
 + key + "' provided");
 }
 if (null == key || "".equals(key)) {
 throw new InvalidArgumentException("Could not inject value into field '" 
 + field.getName() + "' as key was invalid");
 } 
 if (null == parameters || parameters.isEmpty()) {
 throw new InvalidArgumentException("Invalid parameter object provided. Could not inject value with key '" 
 + key + "' into field '" + field.getName() + "'!");
 }
 if (parameters.contains(key)) {
 field.set(this, parameters.get(key));
 } else {
 throw new IllegalArgumentException("Could not set value for field '" 
 + field.getName() + "' as provided parameters did not contain key: '" 
 + key + "'");
 }
}

In the respective constructor you then need to change the code to something like:

public Sample(Map<String, Object> parameters) {
 setRequiredField(getClass().getDeclaredField("someField"), parameters, KEY);
}
...
public SomeSample(Map<String, Object> parameters) {
 super(parameters);
 setRequiredField(getClass().getDeclaredField("someOtherField"), parameters, OTHER_KEY);
}

Instead of a map you can also use a custom parameter object which contains respective getters and setters. Note that I don't handle all kinds of exceptions here as this should only be a short tutorial on parameter objects rather then reflection.

This approach either sets the field value directly or invokes a final and therefore none overridable setter method, which also can be reused by children.

Builder pattern

According to a well-accepted answer the invocation of overridable methods within the constructor can easily get fixed using a builder pattern.

Compared to generic parameter maps, builders allow for compile time safetiness, especially if the parameter object is bound to objects rather then more specific types. In addition to that, reflection also has its price and if you create objects often, this may slow down the application notably.

Builders may use certain default values for fields to set and only overwrite those that are explicitly set. Also, builders usually create only fully initialized instances. Builders are especially useful if you have objects which allow to inject multiple values within the constructor. Besides minimizing the overhead of remembering all kinds of constructors they also allow to group certain parameters logically. Consider some shape builder for instance. you could do something like withPosX(x).withPosY(y).withColor(yellow)... though, logically more expressive would be to use something like withPosition(x, y).withColor(yellow).... Modern Java editors also provide code completion support for builder setters and therefore provide a fluent interface for setting needed parameters. Last but not least, on using builders you usually can assign the values upon build-time to final fields and therefore prevent reassigning other references to this field.

While builders are not used often in combination with inheritance, it is possible with some overhead though. I tend to keep necessary parameters which have no default values within the constructor of the builder and optional parameters or ones which value can be overwritten as typical builder setter methods.

The following sample code presents a scenario where a base builder is extended by child builders. The code is also available on Github. A basic builder has a form like this:

public abstract class Base<Z> {
 public static abstract class Builder<B extends Builder<? extends B, Z>, Z> {
 // necessary parameters
 protected final String a;
 protected final String b;
 protected final String c;
 // optional parameters
 protected Z z = null;
 public Builder(String a, String b, String c) {
 this.a = a;
 this.b = b;
 this.c = c;
 }
 @SuppressWarnings("unchecked")
 public B withOptionalZ(Z z) {
 this.z = z;
 return (B)this;
 }
 public abstract <T> T build();
 }
 protected final String name;
 protected final String a;
 protected final String b;
 protected final String c;
 protected Z z = null;
 protected Base(String name, String a, String b, String c) {
 this(name, a, b, c, null);
 }
 protected Base(String name, String a, String b, String c, Z z) {
 this.name = name;
 this.a = a;
 this.b = b;
 this.c = c;
 this.z = z;
 }
 public String getA() {
 return a;
 }
 public String getB() {
 return b;
 }
 public String getC() {
 return c;
 }
 protected abstract String getContent();
 @Override
 public String toString() {
 return name+"[A: " + a + ", B: " + b + ", C: " + c 
 + (z != null ? ", Z: " + z.toString() : "") + getContent() +"]";
 }
}

Z is the generic type of the optional parameter whereas B is the generic type of the actual builder. In order to invoke the concrete builder on child builders rather then the base builder the generic type is passed as return value on the builder setters. Specifying the generated type the builder produces via the generic type would make sense, though in the current version of Java (Java 7 and 8) on using the diamond opperator, Java is incapable of resolving the correct types. To bypass this limitation instead of returning some type T of the form ? extends Param<Z> the return type is based on the type the builder product is assigned to via <T> T.

The genericReturnType branch in the Github repository also showcases how to solve this issue, though instead of a simple diamond operator a quite complex generic definition has to be provided in order to execute the tests without editor warnings.

A product and builder extension do look like this:

public class SampleA<D,E,Z> extends Base<Z> {
 @SuppressWarnings("unchecked")
 public static class Builder<B extends SampleA.Builder<? extends B, D, E, Z>,
 D,E,Z>
 extends Base.Builder<SampleA.Builder<B,D,E,Z>, Z> {
 protected D d;
 protected E e;
 public Builder(String a, String b, String c) {
 super(a, b, c);
 }
 public B withD(D d) {
 this.d = d;
 return (B)this;
 }
 public B withE(E e) {
 this.e = e;
 return (B)this;
 }
 @Override
 public <T> T build() {
 return (T) new SampleA<>("Test A", a, b, c, z, d, e);
 }
 }
 protected final D d;
 protected final E e;
 protected SampleA(String name, String a, String b, String c, Z z, D d, E e) {
 super(name, a, b, c, z);
 this.d = d;
 this.e = e;
 }
 public D getD() {
 return d;
 }
 public E getE() {
 return e;
 }
 @Override
 protected String getContent() {
 return ", D: " + d + ", E: " + e;
 }
}

D, E, F and G in this sample are just simple classes of the form:

public class D {
 @Override
 public String toString() {
 return "D";
 }
}

to provide some test output and showcase the functionality of the pattern. SampleB is just a ripoff of SampleA which defines setters for F and G rather then D and E.

The code below is just a simple showcase scenario:

public class Main
{
 public static void main(String ... args)
 {
 // sample builder invocation
 SampleA<D,E,?> a = new SampleA.Builder<>("a","b","c")
 .withD(new D()).withE(new E()).build();
 // sample with invoking setter for optional parameter
 SampleB<F,G,String> b = new SampleB.Builder<>("a","b","c")
 .withF(new F()).withG(new G()).withOptionalZ("z").build();
 // assigning the product of the builder to a parent type
 Base<String> c = new SampleA.Builder<>("a","b","c")
 .withD(new D()).withE(new E()).withOptionalZ("z").build();
 // omitted generic type by intention
 Base d = new SampleB.Builder<>("a","b","c")
 .withF(new F()).withG(new G()).build();
 test(a);
 test(b);
 test(c);
 test(d);
 test(new SampleA.Builder<>("a","b","c")
 .withD(new D()).withE(new E()));
 test(new SampleB.Builder<>("a","b","c")
 .withF(new F()).withG(new G()).withOptionalZ("z"));
 testBase(new SampleA.Builder<>("a","b","c")
 .withD(new D()).withE(new E()).withOptionalZ("z"));
 testBase(new SampleB.Builder<>("a","b","c")
 .withF(new F()).withG(new G()));
 }
 public static void test(SampleA<?,?,?> sample)
 {
 System.out.println("Test for SampleA: " + sample.toString());
 }
 public static void test(SampleB<?,?,?> sample)
 {
 System.out.println("Test for SampleB: " + sample.toString());
 }
 public static void test(Base<?> base)
 {
 System.out.println("Test for Base: " + base.toString());
 }
 public static void test(SampleA.Builder<?,?,?,?> builder)
 {
 System.out.println("Test for BuilderA: " + builder.build().toString());
 }
 public static void test(SampleB.Builder<?,?,?,?> builder)
 {
 System.out.println("Test for BuilderB: " + builder.build().toString());
 }
 public static void testBase(Base.Builder<?,?> builder)
 {
 System.out.println("Test for Base builder: " + builder.build().toString());
 }
}

Eclipse and IntelliJ act differently on this code but both editors (as well as compiling and executing the code manually) produce the same output.

The code above should produce the following output:

Test for SampleA: Sample A[A: a, B: b, C: c, D: D, E: E]
Test for SampleB: Sample B[A: a, B: b, C: c, Z: z, F: F, G: G]
Test for Base: Sample A[A: a, B: b, C: c, Z: z, D: D, E: E]
Test for Base: Sample B[A: a, B: b, C: c, F: F, G: G]
Test for BuilderA: Sample A[A: a, B: b, C: c, D: D, E: E]
Test for BuilderB: Sample B[A: a, B: b, C: c, Z: z, F: F, G: G]
Test for Base builder: Sample A[A: a, B: b, C: c, Z: z, D: D, E: E]
Test for Base builder: Sample B[A: a, B: b, C: c, F: F, G: G]

While builder have a couple of advantages, the almost duplicate code requirement is for sure an overhead and therefore may be discouraging to use, especially for objects that do not come with a lot of parameters. Some also argue that the setters that need to be invoked to pass values to the object, which may result in a long chain of invocations, leads to code that is hard to read and therefore is also discouraging. With proper code formatting though I find the code often more readabl compared to invoking setters on the real object.

Certain setters (Product and Builder) may be replaced by Lombok annotations which may help to clean the code further and thus reduce the number of actual lines of code.

lang-java

AltStyle によって変換されたページ (->オリジナル) /