1

This is the code in question, comments point it out:

class Actor extends Entity {
    private MutableVector2f position;
    private MutableIdentity identity;

    public Actor(MutableVector2f initialPosition, MutableIdentity initialIdentity) {
        super(initialPosition, initialIdentity); // !! pass to supertype !!

        this.position = initialPosition; // !! reference in subtype !!
        this.identity = initialIdentity; // !! "" "" !!
    }

    public void moveTowards(Vector2f targetPosition) {
        // !! change position member, purpose of keeping reference !!
    }

    public void changeNameTo(String name) {
        identity.changeNameTo(name);
    }
}

It seems like a code smell. Is it? Maybe there's a problem with the type structure?

Are there any downfalls to this type of design?


The base class Entity is used for any object with a position:

abstract class Entity {
    private Vector2f position;
    private Identity identity;

    public Entity(Vector2f position, Identity identity) {
        this.position = position;
        this.identity = identity;
    }

   public Vector2f getPosition() {
       return position;
   }

   public Identity getIdentity() {
       return identity;
   }
}

The Actor subtype has a mutable position to reduce the amount of objects created (could have hundreds of thousands of actors moving around and changing identity)

The WorldObject subtype has an immutable position and identity:

class WorldObject extends Entity {
    public WorldObject(ImmutableVector2f position, ImmutableIdentity identity) {
        super(position, identity);
    }
}

Vector2f is an interface which 'ImmutableVector2fandMutableVector2f` implement:

interface Vector2f {
    float getX();
    float getY();
}

MutableVector2f exposes methods such as add, multiply and normalize.

Identity follows the same structure (is an interface, has immutable and mutable subtypes)


I feel this may violate LSP. The purpose of the interfaces is to prevent modification from an Entity perspective.

It's not to say Entity is immutable, but to discourage mutation at that level of abstraction, since systems using Entity should only be reading values.

Keeping a reference in the subclass will prevent the need to cast Entity#getPosition(), but it feels wrong. Making the properties generic could lead to a long list of type parameters if I were to continue scaling up Entity in this way.

Is it possible that Entity#getPosition() should be abstract, and the subtypes should maintain their own position variables? It would require me to declare getters in the subtypes (duplicate code), so I'm not a huge fan of the idea.

  • Have you considered using a generic parameter on Entity for the type of Vector2f? – cbojar Mar 25 '17 at 12:19
  • @cbojar Yes, but there were a few downsides. The biggest being a long list of type parameters if I continued to scale Entity in this way. I also don't want Actor#getPosition to be exposed as a MutableVector2f, as that'll further encourage users to simply mutate the vector instead of using behavioral methods. Of course they can still cast Entity#getPosition, or even use reflection if they really wanted, but the point is to discourage us I such tactics by making the (correct) alternatives easier. I edited my post – Vince Emigh Mar 25 '17 at 18:26
  • What will getPosition return after a call to moveTowards - the original value or the updated value? As a user of that class, I'd expect the updated position, but with getPosition defined in the base class and using a private non-updated member, it looks to me that it's the original value. (Note: I use C#, not Java, and there might be implementation differences) – Bernhard Hiller Mar 27 '17 at 13:02
1

If Entity specifies in some contract (documentation, comments?) that its position must not change after its initial construction, this violates the LSP (and the invariants of the base class).

If it doesn't explicitly specify such a thing, it may still be an implicit assumption. But given the way Java works, I would not expect such a thing. However, changing getPosition to be abstract would make it more explicit that the subclasses have control over the position.

If no such assumption exists, the LSP is not violated.

  • "If it doesn't explicitly specify such a thing, it may still be an implicit assumption" - I guess that's basically the question at hand: is this bad practice due to the potential assumption that Entity is immutable? The contract of Entity does not specify immutability, but the type does not expose any mutation behaviors. The only thing specified in the contract of Entity is the ability to have a position and an identity, is that bad? – Vince Emigh Mar 25 '17 at 18:31
  • So add some (doc)comments to the base class that explicitly state such assumptions or the lack thereof. – Sebastian Redl Mar 27 '17 at 11:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.