11

How do I design a subclass whose method contradicts its superclass? Let's say we have this example:

# Example 1

class Scissor
  def right_handed?
    true
  end
end

class LeftHandedScissor < Scissor
  def right_handed?
    false
  end
end

Let's assume that the inheritance is necessary in this example e.g. that there might be other methods aside from right_handed? that are being inherited.

So a LeftHandedScissor is a Scissor. Its method right_handed? matches the signature and return value type of its superclass's method. However, the subclass method contradicts the return value of the superclass method.

This bothers me because it looks like a logical contradiction:

  1. All scissors are right-handed.
  2. A left-handed scissor is a scissor.
  3. Therefore, a left-handed scissor is right-handed.

I considered introducing an AbstractScissor to avoid this contradiction:

# Example 2

class AbstractScissor
  def right_handed?
    raise NotImplementedError
  end
end

class RightHandedScissor < AbstractScissor
  def right_handed?
    true
  end
end

class LeftHandedScissor < AbstractScissor
  def right_handed?
    false
  end
end

My colleague says the downside to this solution is that it's more verbose. Aside from adding an extra class, it requires us to call scissors "right-handed scissors", which no one does in the real world.

This leads me to my last solution:

# Example 3

class AbstractScissor
  def right_handed?
    raise NotImplementedError
  end
end

class Scissor < AbstractScissor
  def right_handed?
    true
  end
end

class LeftHandedScissor < AbstractScissor
  def right_handed?
    false
  end
end

It's nearly the same as example 2 but I just renamed RightHandedScissor as Scissor. There's a chance that someone might assume LeftHandedScissor is a subclass of Scissor based solely on the class names, but at least the code makes it obvious that that's not the case.

Any other ideas?

Rémi
  • 211
  • 2
  • 8
gsmendoza
  • 241
  • 2
  • 8
  • @gnat It looks like this is an example of that question. But I'm not 100% sure. – gsmendoza Jun 29 '15 at 09:21
  • 6
    Why don't you just implement a method "getHandedness" which returns an Enum value? – ASA Jun 29 '15 at 09:36
  • 2
    Why is it bad to contradict the superclass? – Superbest Jun 29 '15 at 09:42
  • 8
    Despite 13 years as a java developer, I've very rarely found a compelling reason to create a class hierarchy in ruby. What are you trying to accomplish that requires more than modules and duck typing? Most rubyists would scream the first time a subclass did anything that contradicted the parent. – Steve Jackson Jun 29 '15 at 10:13
  • 2
    I second Steve Jackson. Don't use OO where it's not necessary. Just because Java enforces this style doesn't mean it's a good idea to make everything a class. – leftaroundabout Jun 29 '15 at 12:46
  • 1
    I think your #1 is wrong. It should be "All scissors are by default right-haded". Overriding will change the behavior of the Scissor. – the_lotus Jun 29 '15 at 15:31
  • "All scissors are right-handed." no, all scissors have handedness, with right as the default. – Dave Cousineau Jun 29 '15 at 16:59
  • Override the method in subclass and provide own implementation. – Aamol Jun 29 '15 at 23:34

8 Answers8

37

There is nothing wrong with the design shown in the question. While one could also introduce abstract Scissor with two concrete subclasses, and maybe more overall clarity, it's also common to do it like shown (especially when the hierarchy is a result of years of incremental development, with Scissor being around for much longer than the concept of handedness).

"Okay, I guess the lesson here is that you don't make assumptions about the subclasses based solely on the superclass?"

You do make such assumptions based on the contracts (method signatures) of the base class, but not based on method implementations. In this case the contract says that right_handed is a Boolean method which can be true or false, so it can be either. You should ignore the fact that the base class implementation always returns true, especially if you allowed to derive from the base class by not freezing it. The base class implementation is then just a default, and the method right_handed exists exactly because the scissor could also be left handed.

Jirka Hanika
  • 710
  • 6
  • 12
  • 5
    Thanks. I like how you distinguished between method signatures and method implementations. I also like how you explained how this is a common situation (it can be result of years of incremental development). – gsmendoza Jun 29 '15 at 09:15
  • 3
    Contracts are not identical to method signatures. Contracts can, and usually do include behavior that cannot be specified in the method signature and is therefore specified in the docs. For example, a method that returns an integer may guarantee that it only returns integers in a certain range. Subclasses should never violate this kind of contract. – Kevin Krumwiede Jun 30 '15 at 02:57
  • 1
    @KevinKrumwiede - [This answer](http://programmers.stackexchange.com/a/288202/56014) is explaining this in more detail, and more importantly, showing why it's often clearer to keep classes intended for derivation separate from classes intended for instantiation. However, contracts are more than just signatures and documentation and it is sometimes difficult to establish what they are in real, underdocumented software coded by someone who didn't believe in its reuse. – Jirka Hanika Jun 30 '15 at 05:59
  • If a contract was just a method signature then it would be useless to ever specify them explicitly, since the signature would enforce it already. – user541686 Jun 30 '15 at 07:52
  • @Mehrdad - In the specific case we are looking at, the question mark isn't part of the method signature. It's not documentation either. It's not a part of a separate contract formalism. It's just a naming convention. However, that's as much of a contract as you can find in the original post. I don't want to get to the mud of discussing the difference between structured code documentation and ad hoc code comments when there's not a single comment in the original post. – Jirka Hanika Jun 30 '15 at 08:33
23

You are thinking too logically! There is no logical contradiction because class definitions are not logical propositions.

Having the Scissor base class return true does not correspond to saying that all scissors are right-handed. It just means that a scissor instance is right-handed unless the method is overridden in a subclass.

JacquesB
  • 57,310
  • 21
  • 127
  • 176
  • "It just means that a scissor instance is right-handed unless the method is overridden in a subclass." Okay, I guess the lesson here is that you don't make assumptions about the subclasses based solely on the superclass? – gsmendoza Jun 29 '15 at 08:44
  • 2
    @gsmendoza The lesson here is that defaults aren't bad things when they properly reflect the class that has them. To say that all scissors are right-handed unless overridden is conceptually wrong and only risks to confuse any programmers reading it. However, it isn't that you *can't* do it this way, just that you shouldn't. This is also a property of a class, not a defining characteristic, so you shouldn't even need to create a subclass for it (at least in this particular example). – Neil Jun 29 '15 at 08:51
  • 1
    @gsmendoza: Exactly - otherwise there would be no use for subclasses, if they couldn't modify base class behavior. – JacquesB Jun 29 '15 at 08:55
  • @JacquesB I can think of one: performance improvements. For example, a polygon's perimeter method adds all the sides, while its square subclass just multiplies one side by 4. So there is some overriding, but the subclass doesn't contradict the meaning of the superclass's method. – gsmendoza Jun 29 '15 at 09:06
  • 4
    @gsmendoza: You shouldn't contradict the *meaning* of a method in a subclass, but presumably the meaning of "right_handed" is tell us if the scissor is right handed. – JacquesB Jun 29 '15 at 10:34
  • @gsmendoza You probably don't have clear what is the contract in the base class. The return values of single methods are *not* part of the contract. However you may have some property of methods that should be preserved. For example: a `Rectangle` class with an `area` and a `set_base` and `set_height` would probably require that after calling `set_base` the height doesn't changes. But if you derive a `Square` from `Rectangle` then `set_base` and `set_height` coincide and thus affect each other (and the value for `area`) breaking the assumption that `h=x.height; x.set_base(y); x.height==h`. – Bakuriu Jun 29 '15 at 11:21
16

You don't. It's like saying that all animals are dogs, and then asking how to make cats meow instead of bark. If you were naming your classes properly, your Scissor class would rather be named RightHandedScissor; now does it make sense to inherit LeftHandedScissor from RightHandedScissor?

  • One possible approach is to make Scissor class abstract, and right_handed would be abstract as well (unlike your example 2).

    Then, you may have RightHandedScissor and LeftHandedScissor if it makes sense (although I can't find an example where it would make sense).

    The major issue here is that you are repeating yourself. The property right_handed is barely rephrasing the type of the object. This leads to two issues:

    1. Some developers using your code will use right_handed property, while others will do if scissor.is_a?(RightHandedScissor). Why not keeping only one way of doing this?

    2. What if, one day, a change is made to the classes, and there is a contradiction between what property says and the type of the object?

    If the business logic dictates that the type is the proper location for separating right handed from left handed scissors, then either remove the property, or at least make its value automatically computed based on the type of the object.

  • A better approach is to have a concrete Scissor class where the property right_handed, and not the actual class type determines whether the scissor is right handed or left handed.

    This makes it easier to deal with the code: you have two classes less and don't have the redundancy (that is a property which barely rephrases the type). If you don't want a left handed scissor to become right handed, you may consider a read-only/constant property instead (i.e. the one which can be set only by the constructor, and cannot be changed after that; the terminology may change from a language to another).

Arseni Mourzenko
  • 134,780
  • 31
  • 343
  • 513
  • I agree. If "right_handed" doesn't determine behavior, it is a *property* and not a defining characteristic of that class. Though even if it were, it is definitely a WTF moment in code review when you create a default value in the base class to avoid creating a separate class. If it defines behavior, while it may be more verbose, it is actually *less* complicated overall in my humble opinion. – Neil Jun 29 '15 at 08:39
  • 1
    @Neil The example of course is trivial. In my "real-life" problem, the method determines what kind of other objects the classes can act on. I haven't considered though whether the property is a method or a behavior and how this determines if the method is a "defining characteristic" of the class. – gsmendoza Jun 29 '15 at 08:54
  • @Neil "If it defines behavior, while it may be more verbose, it is actually less complicated overall in my humble opinion." So I guess this means you think Examples 2 and 3 are better than 1? – gsmendoza Jun 29 '15 at 08:55
  • @MainMa "If you were naming your classes properly, your Scissor class would rather be named RightHandedScissor". "One possible approach is to make Scissor class abstract, and right_handed would be abstract as well" - This is like Example 2 right? – gsmendoza Jun 29 '15 at 08:57
  • @gsmendoza Absolutely, in particular example 2. I see your point. The example is to summarize the problem here. Consider though that if you're using such a property to determine how to use that instance, there is another error at play here. Ideally, you shouldn't *care* about whether it is right-handed or left-handed when you use an instance of Scissors. The subclasses should take care of these details. – Neil Jun 29 '15 at 08:58
  • @Neil Yeah, this is more of a limitation of the example: I can't think of a hand-agnostic method that can illustrate the problem while still make distinctions between left and right handed scissors :) – gsmendoza Jun 29 '15 at 09:01
  • "A better approach is to have a concrete Scissor class where the property right_handed, and not the actual class type determines whether the scissor is right handed or left handed" -- This is something I haven't considered yet in the real-life problem this example is based on. I'll see if it's applicable :) – gsmendoza Jun 29 '15 at 09:03
9

Another option is to introduce the handed-ness as a dependency with a default value of right-handed. In pseudocode here as I am not familiar with Ruby:

class Scissors {
    Scissors(isRightHanded = true) {
        _isRightHanded = isRightHanded
    }

    IsRightHanded() {
        return _isRightHanded
    }
}

class LeftHandedScissors : Scissors {
    LeftHandedScissors() : Scissors(isRightHanded: false) { }
}

In fact, you then don't even need the LeftHandedScissors class - you can just create Scissors(isRightHanded: false) when you need a left-handed scissor.

This approach is called preferring composition over inheritance

Alex
  • 1,213
  • 8
  • 22
  • @Neil correct :) although the above isn't *quite* C# – Alex Jun 29 '15 at 08:56
  • 3
    No, of course. I just like to guess a programmer's language based on how they style pseudocode. – Neil Jun 29 '15 at 09:00
  • @Neil I even went out of my way to put opening braces on the same line! ;) – Alex Jun 29 '15 at 09:03
  • I bet that took everything to not put the open brace on its own line. ;) – Neil Jun 29 '15 at 09:08
  • 1
    @AlexFoxGill Thanks! I think this is the same as what MainMa suggested right? "A better approach is to have a concrete Scissor class where the property right_handed, and not the actual class type determines whether the scissor is right handed or left handed." – gsmendoza Jun 29 '15 at 09:09
  • @gsmendoza yes, although I would recommend it for not just simple properties but methods as well - this is the [strategy pattern](https://en.wikipedia.org/wiki/Strategy_pattern). It has the added benefit of keeping your code loosely coupled – Alex Jun 29 '15 at 09:12
4

Summary: The design without the abstract class will be only be acceptable if it is carefully documented to distinguish its abstract and concrete behaviours.

The Liskov Substitution Principle is generally regarded as a "good thing". By the LSP, I mean that if type S is a subtype of type T, then objects of type S should behave as objects of type T are expected to behave.

You can not follow this principle unless the expectations for behaviour are made clear. This set of expectations is the so called "contract" of the class. In most languages it is expressed by a combination of signature and documentation.

Therefore, whether your first design is correct depends on the contracts.

Incorrect:

class Scissor
  # Returns true because all scissors are right-handed.
  def right_handed?
    true
  end
end

class LeftHandedScissor < Scissor
  # Returns false
  def right_handed?
    false
  end
end

Acceptable:

class Scissor
  # Returns true for right-handed scissors but false for left-handed ones.
  def right_handed?
    true
  end
end

class LeftHandedScissor < Scissor
  # Returns false
  def right_handed?
    false
  end
end

The code is the same. Whether the design is sensible (i.e. follows the LSP) or not depends on the contracts.

Now it is clear that, if you only know x.kind_of Scissors, then x.right_handed? might return either true or false.

But what about the true? Should that be documented? Yes. A class that can be extended needs to be documented twice. It needs documentation of its abstract behaviour, which is the behaviour we can expect from all its instance (direct or indirect), and it needs documentation of its concrete behaviour, which is the behaviour that its direct instances will have, i.e., the behaviour as implemented at this level of the inheritance hierarchy and which is inherited if not overridden.

Even better:

class Scissor
  # Abstract behaviour: Returns true for right-handed scissors but false for left-handed ones.
  # Concrete behaviour: Returns true.
  def right_handed?
    true
  end
end

class LeftHandedScissor < Scissor
  # Abstract and concrete behaviour: Returns false
  def right_handed?
    false
  end
end

Now it is clear also --from the documentation alone-- that, if you know s instance_of Scissors, then x.right_handed? will return true.

Often people don't bother to document the concrete behaviour, since, assuming there are no bugs, it can often be reverse engineered from the code.

The same is not true of the abstract behaviour. Unless it is documented, it can not be recovered from the code. It is left to future programmers to guess what your intentions were. Implementors of client code must guess at what they can assume about objects that are indirect instances of the class. Implementors of subclasses must guess at what is and what is not reasonable override.

All that said, I prefer your solution with the abstract class. I consider it better style. But this is only a matter of taste.

Bottom line: The design without the abstract class will be only be acceptable if it is carefully documented to distinguish its abstract and concrete behaviours.

Theodore Norvell
  • 858
  • 6
  • 10
  • 1
    Thanks for the insightful answer. I like that you distinguished between abstract and concrete behaviors. In RSpec, you could document abstract behaviors by creating a "shared examples" module for Scissors. So you'd have a test `describe RightHandedScissor { it_behaves_like(Scissor) }`. – gsmendoza Jul 06 '15 at 08:40
3

"All scissors are right-handed"? Where do you get that idea from? Your code only expresses "scissors are right-handed by default". It's a default value, not a design decision. If there were no way of having a different value, what's the point in programming a boolean accessor function?

Kilian Foth
  • 107,706
  • 45
  • 295
  • 310
  • 3
    S/he got that idea by treating the code as documentation. – user253751 Jun 29 '15 at 08:22
  • Okay. Your answer is similar to @JacquesB in that the superclasses only provide default values. My thinking was that the values provided by superclasses should be treated "like canon" :) – gsmendoza Jun 29 '15 at 08:49
2

This bothers me because it looks like a logical contradiction:

All scissors are right-handed.

Incorrect at this point. You've defined scissors as all having handedness, and that this defaults to right-handed if not overridden. You have not said that all scissors are right-handed, that would require overriding being prohibited (as can be done in some languages, and not in others).

Remove that false premise, and the contradiction is no longer there.

Jon Hanna
  • 2,115
  • 12
  • 15
1

Congratulations, you just have discovered how the violation of Liskov Substitution Principle looks like, as the very first commenter politely pointed you at.

To answer exactly to your question: according to the aforementioned principle, you don't design a subclass whose method contradicts its superclass. Reasons for this are explained in the literature about LSP, Robert Martin, for example, talks quite a lot about it his learning videos.

To address your specific problem with logical contradictions: classes in your application do not correspond to real-world objects, and do not need to. You can read and hear about this from quite a lot of experienced speakers, J. B. Rainsberger, for example.

hijarian
  • 159
  • 8