17

I currently have two derived classes, A and B, that both have a field in common and I'm trying to determine if it should go up into the base class.

It is never referenced from the base class, and say if at some point down the road another class is derived, C, that doesn't have a _field1, then wouldn't the principal of "least privileged" (or something) be violated if it was?

public abstract class Base
{
    // Should _field1 be brought up to Base?
    //protected int Field1 { get; set; }
}

public class A : Base
{
    private int _field1;
}

public class B : Base
{
    private int _field1;
}

public class C : Base
{
    // Doesn't have/reference _field1
}
samus
  • 465
  • 1
  • 5
  • 13
  • 18
    I think this question is unclear because you haven't given us any idea of what `Base`, `A`, `B`, `C`, and `_field1` are. Those are important details that should not be left out; I think you should edit the question to talk about what those are. – Tanner Swett Jan 04 '19 at 21:12
  • Based on the answers I would: rebuild Vehicle to have a virtual Suspension, then Car and Bicycle could have Wheels and Boat could have Buoyancy, which will move the abstraction upwards. I find that if my abstraction leads directly to specific settings then I haven't moved the concept far enough up the chain. – Patrick Hughes Jan 04 '19 at 21:19
  • 6
    Don't use class inheritance to avoid duplicating code. Use it for inheriting and extending _behavior_, i.e. polymorphism. Move a common field to a base class *if and only if* it is logically the same field, not two unrelated pieces of information which happen to share the same name in their respective contexts. – Brandon Jan 05 '19 at 00:01
  • Why do you have a base class to begin with? – jpmc26 Jan 05 '19 at 01:13

4 Answers4

34

It all depends upon the exact problem you're trying to solve.

Consider a concrete example: your abstract base class is Vehicle and you currently have the concrete implementations Bicycle and Car. You're considering moving numberOfWheels from Bicycle and Car to vehicle. Should you do this? No! Because not all vehicles have wheels. You can already tell that if you try to add a Boat class then it's going to break.

Now, if your abstract base class was WheeledVehicle then it's logical to have the numberOfWheels member variable in there.

You need to apply the same logic to your problem, because as you can see, it's not a simple yes or no answer.

Pete
  • 3,181
  • 1
  • 12
  • 18
  • 3
    One could temporarily accept that 0 is a valid numberOfWheels. However, eventually you might add a `roll()` method, at which point the subclass idea is looking prescient. – user949300 Jan 04 '19 at 17:21
  • 11
    A boat has 0 wheels. What does that break? – D Drmmr Jan 04 '19 at 18:51
  • 14
    @DDrmmr It's not that a Boat has 0 wheels, it that Wheels don't even exist as a concept for a Boat - hence your models shouldn't allow for it. – Peter M Jan 04 '19 at 19:09
  • 10
    My point is that the example is bad. There is nothing conceptually wrong with a vehicle (that happens to be a boat) stating that it has 0 wheels. – D Drmmr Jan 04 '19 at 19:50
  • 1
    @DDrmmr Your point may be more of a physiological argument in subjectivity, but for the boat example in particular I would say it would best be represented by the number of sails, oars, rutters, props/engines, ... So I'd instead rather have an explicitly named field for exactly what is being represented that a generic catch all that could represent the number of wheels or the number of sails. – samus Jan 04 '19 at 20:18
  • @DDrmmr It's all fine and dandy to say that a boat has zero wheel, and that is a great philosophical point of view. But computers don't do philosophy at all, so it can all fall in a heap when someone accidentally sets the number of wheels of a boat instance to a non-zero value. – Peter M Jan 04 '19 at 21:37
  • @PeterM "philosophical" thanks, haven't used that one it in a while : ) – samus Jan 04 '19 at 22:01
  • 1
    There is nothing wrong with saying a boat has 0 wheels. The question is how useful or misleading is that? The moment you start writing code that does things based on whether or not a vehicle has wheels (like `if (vehicle.hasWheels()) { vehicle.rotateTires()`), you make confusing, inconsistent, and potentially buggy code. If whether a vehicle depends on wheels or not is an important characteristic, then the object model should reflect that (i.e. a `WheeledVehicle` interface) . When reading code, defining something as having a number of wheels suggests wheels are relevant, and they aren't. – Brandon Jan 04 '19 at 23:56
  • 3
    There is also a difference between a vehicle which requires wheels but has 0 (because they are in the shop being replaced) and a vehicle that has no concept of wheels, like a boat. This is a bit like `NULL`. If a boat does not have a concept, don't give it wheel traits. – Brandon Jan 04 '19 at 23:59
  • 10
    Then it all goes to hell when you need to store a paddle boat. – IllusiveBrian Jan 05 '19 at 02:04
13

Logically speaking, beyond placing the field replicated in subclasses vs. in common in the base class, there is a third option: which is to introduce a new subclass into the hierarchy that has the common properties between the two.  @Pete hints at this without fully going there.

Using @Pete's example, we would introduce a (possibly abstract) subclass for Wheeled Vehicle that descends from the original base class — while the two subclasses descend from it.  Thus, the original base class is not polluted with wheels, yet the commonality of wheels is DRY (not repeated among subclasses that have wheels).

This may, of course, be overkill for your purposes, but such is supported by the class hierarchy mechanism.

Erik Eidt
  • 33,282
  • 5
  • 57
  • 91
  • This is actually what I've decided to do (A and B overlap mostly, so B will derive from A). – samus Jan 04 '19 at 18:34
9

I'm going to play devil's advocate here.

Right now you should do nothing.

Is it DRY? No. But it's better to have a little duplication than a premature abstraction that you can't easily back out of later. The refactor to move a property to a common base class is easy. Going the other way isn't. Wait and see.

When making this sort of decision I tend to use a "rule of 3": once I have repeated the same thing in e.g. three different places, and only then, do I consider moving it up the chain. N.B. you're only at 2.

Jared Smith
  • 1,620
  • 12
  • 18
  • 2
    A wise observation that is, I would say, required for making the decision. – radarbob Jan 04 '19 at 22:25
  • 1
    I mostly agree, but "Right now" (with no extra code as the one we see) the refactoring in both directions is trivial. But if there is additional code which uses the field, refactoring in **both directions** can become significantly harder. – Doc Brown Jan 05 '19 at 13:35
2

In general, I would move it to the base class. I don't think there's an objective yes/no, because there's a trade-off here - carrying unused fields vs reducing complexity.

I typically prefer 'heavy' base classes that contain anything that might be shared. This makes serializing to files simpler since you don't need descendant serializing methods in every derived class. But if you don't have that or a similar issue, or perhaps you need to do everything you can to reduce memory usage, then only keeping the fields where you need them should be fine.

An 'intermediary' class that introduces the common fields will be fine if you have a very limited number of fields. But be aware that approach can dramatically increase complexity if you have dozens of fields used in different combinations, leading to many intermediary classes each introducing a specific set of fields. That can become a maintenance problem.

GrandmasterB
  • 37,990
  • 7
  • 78
  • 131
  • My example is trivial, though it is still production code. You make a good argument for "compromising" the hierarchy with a "heavy" base class, which I too would see myself leaning towards in such a case. – samus Jan 04 '19 at 20:11
  • 1
    @samis: you use classes named "A, B, C" and "Base" with just one field and no method in production code? I question that. – Doc Brown Jan 05 '19 at 16:49