Is overriding concrete methods a code smell?

Question

Is it true that overriding concrete methods is a code smell? Because I think if you need to override concrete methods:

public class A{
    public void a(){
    }
}

public class B extends A{
    @Override
    public void a(){
    }
}

it can be rewritten as

public interface A{
    public void a();
}

public class ConcreteA implements A{
    public void a();
}

public class B implements A{
    public void a(){
    }
}

and if B wants to reuse a() in A it can be rewritten as:

public class B implements A{
    public ConcreteA concreteA;
    public void a(){
        concreteA.a();
    }
}

which does not require inheritance to override the method, is that true?

Peter Walser · Answer 1 · 2016-04-12T07:36:47.060

No, it is not a code smell.

If a class is not final, it allows to be subclassed.
If a method is not final, it allows to be overridden.

It lies within the responsabilities of each class to carefully consider if subclassing is appropriate, and which methods may be overridden.

The class may define itself or any method as final, or may place restrictions (visibility modifiers, available constructors) on how and where it is subclassed.

The usual case for overriding methods is a default implementation in the base class which may be customized or optimized in the subclass (especially in Java 8 with the advent of default methods in interfaces).

class A {
    public String getDescription(Element e) {
        // return default description for element
    }
}

class B extends A {
    public String getDescription(Element e) {
        // return customized description for element
    }
}

An alternative for overriding behaviour is the Strategy Pattern, where the behaviour is abstracted as an interface, and the implementation can be set in the class.

interface DescriptionProvider {
    String getDescription(Element e);
}

class A {
    private DescriptionProvider provider=new DefaultDescriptionProvider();

    public final String getDescription(Element e) {
       return provider.getDescription(e);
    }

    public final void setDescriptionProvider(@NotNull DescriptionProvider provider) {
        this.provider=provider;
    }
}

class B extends A {
    public B() {
        setDescriptionProvider(new CustomDescriptionProvider());
    }
}

score 28 · Answer 2 · answered Apr 11 '16 at 02:21

Is it true that overriding concrete methods is a code smell?

Yes, in general, overriding concrete methods is a code smell.

Because the base method has a behavior associated with it that developers usually respect, changing that will lead to bugs when your implementation does something different. Worse, if they change the behavior, your previously correct implementation may exacerbate the problem. And in general, it's fairly difficult to respect the Liskov Substitution Principle for non-trivial concrete methods.

Now, there are cases where this can be done and is good. Semi-trivial methods can be over-ridden somewhat safely. Base methods that exist only as a "sane default" sort of behavior that is meant to be over-ridden have some good uses.

Hence, this does seem like a smell to me - sometimes good, usually bad, take a look to make sure.

score 7 · Answer 3 · answered Apr 11 '16 at 05:58

if you need to override concrete methods [...] it can be rewritten as

If it can be rewritten that way it means you have control over both classes. But then you know whether you designed class A to be derived in this way and if you did, it is not a code smell.

If, on the other hand, you don't have control over the base class, you can't rewrite in that way. So either the class was designed to be derived in this way, in which case just go ahead, it is not a code smell, or it was not, in which case you have two options: look for another solution altogether, or go ahead anyway, because working trumps design purity.

score 3 · Answer 4 · answered Apr 11 '16 at 06:14

First, let me just point out that this question is only applicable in certain typing systems. For example, in Structural typing and Duck typing systems - a class simply having a method with the same name & signature would mean that class was type compatible (at least for that particular method, in the case of Duck typing).

This is (obviously) very different from the realm of statically typed languages, such as Java, C++, etc. As well as the nature of Strong typing, as used in both Java & C++, as well as C# and others.

I am guessing you're coming from a Java background, where methods must explicitly be marked as final if the contract designer intends for them not to overridden. However, in languages such as C#, the opposite is the default. Methods are (without any decorating, special keywords) "sealed" (C#'s version of final), and must be explicitly declared as virtual if they are allowed to be overridden.

If an API or library has been designed in these languages with a concrete method that is overridable, then it must (at least in some case(s)) make sense for it to be overridden. Therefore, it is not a code smell to override an unsealed/virtual/not final concrete method. _{(This assumes of course that the designers of the API are following conventions and either marking as virtual (C#), or marking as final (Java) the appropriate methods for what they're designing.)}

See Also

Spotted · Answer 5 · 2016-04-12T07:04:18.720

Yes it is because it can lead to call super anti-pattern. It can also denatures the initial purpose of the method, introduce side-effects (=> bugs) and make tests fail. Would you use a class which unit tests are red (failling) ? I won't.

I'll go even further by saying, the initial code smell is when a method is not abstract nor final. Because it allows to alter (by overriding) the behaviour of a constructed entity (this behaviour can be lost or altered). With abstract and final the intent is unambiguous:

Abstract: you must provide a behaviour
Final: you're not allowed to modify the behaviour

The safest way to add behaviour to an existing class is what your last example shows: using the decorator pattern.

public interface A{
    void a();
}

public final class ConcreteA implements A{
    public void a() {
        ...
    }
}

public final class B implements A{
    private final ConcreteA concreteA;
    public void a(){
        //Additionnal behaviour
        concreteA.a();
    }
}

Is overriding concrete methods a code smell?

5 Answers5