CODEWITHSUNDEEP
javaoop
Belun
Belun

Reputation: 4169

How to write proper OOP code in order to avoid having field initialization issues?

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?

Upvotes: 1

Views: 202

Answers (3)

millimoose
millimoose

Reputation: 39970

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.)

Upvotes: 1

Vala
Vala

Reputation: 5674

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

Upvotes: 1

Jon Skeet
Jon Skeet

Reputation: 1502106

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 :)

Upvotes: 3

Related Questions