Does anybody see a reason in DesignForExtension Check in Checkstyle?

Question

Check: http://checkstyle.sourceforge.net/config_design.html#DesignForExtension

False positives: Checkstyle "Method Not Designed For Extension" error being incorrectly issued? checkstyle Method is not designed for extension - needs to be abstract, final or empty https://sourceforge.net/p/checkstyle/bugs/688/

Look like all switch that Check off in their configurations.

Does anybody could show real code example where this Check is useful ? Is it useful for developers in practice, not in theory?

Answer 1

The documentation you linked to already explains the rationale behind the check. This can be useful in some situations. In practice, I've never turned it on, mostly because it is too cumbersome to administer, and you certainly don't want it for all your classes.

But you asked for a code example. Consider these two classes (YourSuperClass is part of the API you provide, and TheirSubClass is provided by the users of your API):

public abstract class YourSuperClass
{
    public final void execute() {
        doStuff();
        hook();
        doStuff();
    }

    private void doStuff() {
        calculateStuff();
        // do lots of stuff
    }

    protected abstract void hook();

    protected final void calculateStuff() {
        // perform your calculation of stuff
    }
}

public class TheirSubClass extends YourSuperClass
{
    protected void hook() {
        // do whatever the hook needs to do as part of execute(), e.g.:
        calculateStuff();
    }

    public static void main(final String[] args) {
        TheirSubClass cmd = new TheirSubClass();
        cmd.execute();
    }
}

In this example, TheirSubClass cannot change the way execute works (do stuff, call hook, do stuff again). It also cannot change the calculateStuff() method. Thus, YourSuperClass is "designed for extension", because TheirSubClass cannot break the way it operates (like it could, if, say, execute() wasn't final). The designer of YourSuperClass remains in control, providing only specific hooks for subclasses to use. If the hook is abstract, TheirSubClass is forced to provide an implementation. If it is simply an empty method, TheirSubClass can choose to not use the hook.

Checkstyle's Check is a real-life example of a class designed for extension. Ironically, it would still fail the check, because getAcceptableTokens() is public but not final.

Answer 2

I have this rule enabled for all of my Spring-based projects. It's a royal PITA at first because it does represent a lot of code cleanup. But I've learned to love the principle of this rule. I find the rule to be useful at enforcing everyone to be consistent and clear in their thinking about which classes should be designed for extension and which shouldn't. In the code-bases that I've worked with, there are in reality, only a handful of classes that should be open to extension. Most are just asking for bugs by allowing extension. Maybe not today, but down the road when the current engineer is long-gone or that section of code is forgotten about and a quick change needs to come in to fix "X customer is complaining about Y and they just need Z".

Consider:

1. It's too easy to subclass anything in Java willy-nilly and therefore behavior can change over time through different extended classes. New programmers may get OOP happy and everything then extends something generic just because they can.

2. Overly-extended class-depth is difficult to reason about, and while you might be an amazing developer who'd never do something as atrocious as that ... I've worked in code-bases where that was the norm. And those deeply nested HTML Form generators were awful to work with and reason about what would actually happen So, this rule would, in theory, make the original engineer think twice about writing something so awful for their peers.

3. By enforcing the final rule or documenting a class designed for extension the possible bugs that could occur through inadvertent extension of a class may be avoided. I don't personally like the idea that some subclass could alter the behavior of my application because that could cause unintended side-effects in weird ways (especially large and partially tested applications). And, it's in these extended classes where complex behavior is hidden that the hard-to-solve bugs exist.

4. The DesignForExtension rule forces conversations amongst developers whenever something might be extended as an initial choice to get a quick-fix out the door when really what should happen is that the developers need to meet up and discuss what's changing, and discuss why extension might be appropriate. Many times, modifications to the main class are more appropriate and additional tests would be written given the new circumstances. This promotion of conversations is healthy for long-term code quality and intra-organizational knowledge sharing.

That being said, I do add to my checkstyle-suppressions.xml in my code for Spring-specific classes that cannot be declared final. Because, frameworks.

<suppress checks="DesignForExtension" files=".*Configuration\.java"/>