16

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
}
  • 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 4 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 4 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 5 at 0:01
  • Why do you have a base class to begin with? – jpmc26 Jan 5 at 1:13
35

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.

  • 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 4 at 17:21
  • 11
    A boat has 0 wheels. What does that break? – D Drmmr Jan 4 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 4 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 4 at 19:50
  • 10
    Then it all goes to hell when you need to store a paddle boat. – IllusiveBrian Jan 5 at 2: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.

  • This is actually what I've decided to do (A and B overlap mostly, so B will derive from A). – samis Jan 4 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.

  • 2
    A wise observation that is, I would say, required for making the decision. – radarbob Jan 4 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 5 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.

  • 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. – samis Jan 4 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 5 at 16:49

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.