Object-oriented – When to stop inheritance

design-patternsdesign-principlesobject-orientedprogramming practices

Once upon time ago I asked a question on Stack Overflow about inheritance.

I have said I design chess engine in OOP fashion. So I inherit all my pieces from Piece abstract class but inheritance still goes. Let me show by code

public abstract class Piece
{
    public void MakeMove();
    public void TakeBackMove();
}

public abstract class Pawn: Piece {}

public class WhitePawn :Pawn {}

public class BlackPawn:Pawn {}

Programmers has been found my design a little bit over engineering and suggested to remove colored piece classes and hold piece color as a property member like below.

public abstract class Piece
{
    public Color Color { get; set; }
    public abstract void MakeMove();
    public abstract void TakeBackMove();
}

In this way Piece can know his color. After implementation i have seen my implementation goes like below.

public abstract class Pawn: Piece
{
    public override void MakeMove()
    {
        if (this.Color == Color.White)
        {

        }
        else
        {

        }
    }

    public override void TakeBackMove()
    {
        if (this.Color == Color.White)
        {

        }
        else
        {

        }
    }
}

Now I see that keep color property causing if statements in implementations. It makes me feel we need specific color pieces in inheritance hierarchy.

In such a case would you have classes like WhitePawn, BlackPawn or would you go to design by keeping Color property?

Without seen such a problem how would you want to start design? Keep Color property or having inheritance solution ?

Edit: I want to specify that my example may not completely match real life example. So before try to guess implementation details just concentrate the question.

I actually simply asking if using Color property will cause if statements better to use inheritance ?

Best Answer

Imagine you design an application which contains vehicles. Do you create something like this?

class BlackVehicle : Vehicle { }
class DarkBlueVehicle : Vehicle { }
class DarkGreenVehicle : Vehicle { }
// hundreds of other classes...

In real life, color is a property of an object (either a car or a chess piece), and must be represented by a property.

Are MakeMove and TakeBackMove different for blacks and whites? Not at all: the rules are the same for both players, which means that those two methods will be exactly the same for blacks and whites, which means that you are adding a condition where you don't need it.

On the other hand, if you have WhitePawn and BlackPawn, I will not be surprised if soon or later you'll write:

var piece = (Pawn)this.CurrentPiece;
if (piece is WhitePawn)
{
}
else // The pice is of type BlackPawn
{
}

or even something like:

public abstract class Pawn
{
    public Color Player
    {
        get
        {
            return this is WhitePawn ? Color.White : Color.Black;
        }
    }
}

// Now imagine doing the same thing for every other piece.

Type System Abuse?

Reading the post office example, there is some evidence to suggest that you might be trying to abuse the type system to encode property values in interfaces. And failing...

The way you are currently implementing it, Speed vs. Normal are mutually exclusive, while there is no reason why a class cannot simultaneously be an ExecutivePostageLot and a PublicPostageLot, as far as I can tell.

From your description, however, it appears that you want to categorize postage lots (and postages for that matter) along two dimensions: velocity (speed vs. normal) and importance (executive vs. public).

If so, having an interface for each of the values is not how you should approach it.

Alternative Interpretation

But your question and the diagrams seem to allow another way to interpret what you're trying to do. Maybe I'm imagining things, but I think you want to do the following (I'll use a different example, but it should map 1:1 to your diagram):

You have an Entity class with common methods - e.g. int Id() and DateTime Created().

You have multiple modules, each module defining its own Entity subclasses. So for example, you have a purchasing module (with entities like Order and Product), and a newsletter module (with entities like Post and Subscription).

Now, it seems you want to work with various kinds of sets of a particular subclass of Entity. Some of those sets need to provide only generic functionality that is not specific to a kind of entity. For example, there might be a SortableSet like the following:

class SortableSet<T extends Entity> 
    implements EntitySet<T>
{
    // resorts the set (i know, this makes no sense for a set).
    void Sort(IComparer<T> cmp);

    // ... whatever methods are required by EntitySet ...
    // ... these might also be abstract, of course ...
}

Each of the modules, however, may also need more specific functionality added to our sets. The newsletter module might need to combine all posts in a set to form a new super-post:

// assuming Post has subclasses, otherwise this does not
// need to be generic, of course.
interface CombinablePostSet<T extends Post>
    implements EntitySet<T>
{
    T combineAll();
}

Now we can define a class that combines the required functionality of a module with the generic functionality provided by a general purpose EntitySet, like SortableSet.

class CombinableSortablePostSet<T extends Post>
    extends SortableSet<T>
    implements CombinablePostSet<T>
{
    T combineAll() { ... }
}

If, for some reason, we want to create a class that produces a SortableSet, it should probably not create a set of basic entities, but of a specific entity type. This means we have to let the method know the type of entity that will be in the set. This can be solved with a generic method. (Or with a generic class, but let's go with the method for now).

Of course, in the method body, you can still only treat the elements as Entities, not as Products or Posts, because this method does not know anything about their module-specific functionality.

// this might be a Factory?
class CommonSortableSetLogic
{
    <T extends Entity> SortableSet<T> Processor()
    {
        SortableSet<T> set = null;
        return set;
    }
}

If instead the method returned a SortableSet<Entity> or even an EntitySet<Entity>, we cannot simply cast the returned result to just any subclass of EntitySet with any generic parameter, just because that is the type we need. This is the reason you're getting errors - your CommonMethod1EntitySetLogic methods need to provide the right kind of EntitySet.

But, factories and the like aside, the common logic for a SortableSet<T> should be implemented in the SortableSet<T> class itself. That's why its a class (from which you derive) and not an interface.

Some Notes

Assuming I have understood what you wanted to do, there's still the question of whether you should actually do it like this. We would need to know more about your actual application (what's the purpose of the Entity class - what's special about entity sets that is different from regular sets, etc).

With proper use of generics, operation classes that work on regular sets (like a SetSorter<T>) might lead to less coupling and a clearer structure, if composed well. Anyways, this is a topic for another day maybe.

If you do stick to the design, just remember that A<X> cannot be cast to B<X> or A<Y>, even if B extends A and Y extends X. This kind of thing tends to bite you when not being careful with generics.

Best Answer

Related Solutions

Java Swing – When to Extend a Class

Java – Designing generic type inheritance hierarchy

Type System Abuse?

Alternative Interpretation

Some Notes

Related Topic