1

Running this piece of code will print null

public class Weird { static class Collaborator { private final String someText; public Collaborator(String text) { this.someText = text; } public String asText() { return this.someText; } } static class SuperClass { Collaborator collaborator; public SuperClass() { initializeCollaborator(); } protected void initializeCollaborator() { this.collaborator = new Collaborator("whatever"); } public String asText() { return this.collaborator.asText(); } } static class SubClass extends SuperClass { String someText = "something"; @Override protected void initializeCollaborator() { this.collaborator = new Collaborator(this.someText); } } public static void main(String[] arguments) { System.out.println(new Weird.SubClass().asText()); } }

(Here is also the GitHub Gist)

Now, I know why this happens (it's because the field of the superclass is initialized and then the constructor of the superclass is called, before the field of the subclass is initialized)

The questions are:

  1. What is the design issue here? What is wrong with this design, from an OOP point of view, so that the result looks weird to a programmer? What OOP principles are being broken through this design?
  2. How to refactor so it does not work 'weird' and is proper OOP code?
Improve this question

3 Answers 3

Reset to default
3

What's wrong with the design: you're calling an instance method from a constructor, and then overriding it in a subclass. Don't do that. Ideally, only call private or final instance methods, or static methods, from constructor bodies. You're also exposing a field (which is an implementation detail) outside SuperClass, which isn't a great idea - but writing a protected setCollaborator method and calling that from initializeCollaborator would give the same problem.

As to how to fix it - it's not really clear what you're trying to achieve. Why do you need the initializeCollaborator method at all? There can be various ways of approaching this problem, but they really depend on knowing eaxctly what you're trying to achieve. (Heck, in some cases the best solution is not to use inheritance in the first place. Prefer composition over inheritance, and all that :)

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

@Belun: That's not really enough information to help you work out the best solution.
what i mean was that, most of this code (i have shown you), already exists. it's legacy code. what i need to do is customize Collaborator for SubClass, while leaving it the same as i was for SuperClass (and my solution led to that problem)
@Belun: Well, that pretty much rules out your question 2, if you don't want to really refactor it to sanity :) Can you initialize your collaborator within the body of the subclass constructor instead of via an overridden method call? At that point your subclass will be "more" initialized, so you'll be in a better position.
i will try refactoring, of course, just as little as possible
1

My issue with the design is the someText string should either be an explicit dependency (or "collaborator") of the Collaborator object, or of SubClass, or explicitly a part of the global context (so a constant or a property of a shared context object).

Ideally, either Collaborator should be responsible for retrieving its dependencies; or, if SubClass is responsible for this, it should have someText as a dependency (even if it's always initialised to the same value), and only initialise Collaborator when someText is set.

Conceptually speaking, the dependency relation between objects in the design imposes a partial ordering of the initialisation. The mechanism to implement this ordering should always be explicit in your design, instead of relying on implementation details of the Java language.

An (overengineered) example:

interface ICollaboratorTextLocator { String getCollaboratorText(); } class ConstantCollaboratorTextLocator implements ICollaboratorTextLocator { String text; ConstantCollaboratorTextLocator(String text) { this.text = text; } } class SuperClass { Collaborator collaborator; public setCollaboratorTextLocator(ICollaboratorTextLocator locator) { collaborator = new Collaborator(locator.getCollaboratorText()); } SuperClass() { setCollaboratorTextLocator(new ConstantCollaboratorTextLocator("whatever")); } } class SubClass { String text = "something"; SubClass() { setCollaboratorTextLocator(new ConstantCollaboratorTextLocator(text)); } }

(Now excuse me, I need to go stand under a waterfall after writing something called ConstantCollaboratorTextLocator.)

Improve this answer

2 Comments

One thing that eluded me before and I caught it on a reread: if SubClass depends on Collaborator, and Collaborator depends on a field of SubClass, the result is a subtle cyclic dependency. This would explain why it's prone to an order-of-initialisation bug.
i have marked your answer as the right one because my solution was closest to one of your suggestions ('if SubClass is responsible for this, it should have someText as a dependency (even if it's always initialised to the same value'). my solution is quite hard-coded: in SubClass, just did new Collaborator("something");
1

Instead of using the intializeCollaborator method, make the Collaborator a parameter on the constructor and call it using super(new Collaborator(..)) from the child.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.