My "anti-patterns" - 2

Continuing the "anti-patterns" subject. By the way, "anti-patterns" are not, strictly speaking, the "anti" of "patterns". "Anti-patterns" are specifically "don't do" while patterns do not necessarily mean "do". However, it's much easier to say "don't" than to say "do". Out of the 10 commandments the vast majority is "don't". Out of 613 mitzvot, 365 are negative. So let's continue the wave...

Inheritance

I just love to make controversial statements! So bombastic - do not use inheritance! OK, you know it was a joke. But! I see so much cases of inheritance being used for no good reason that I'm becoming worried. It seems that the new generation of developers was raised on a hype of object-oriented. Functions are passé, we should program in objects! The next (highly dangerous!) clause coming out of this is "when there's a common piece of code, one should use inheritance". Wrong, wrong, completely wrong!

Several horrible constructions come out of this thinking. Abstract classes that are full of unrelated methods placed there only in order to be accessible in subclasses. "This new class is almost like the one we have - let's derive it from the latter." "We have a flow that differs in some details - let's create a super-class with a couple of abstract methods and implement them in subclasses. The latter is called "template pattern". Finally, there's a pure abuse of inheritance (especially interfaces) that has to do with creating small abstract classes (or interfaces) no one really uses.

Let's look at these examples one by one. In this case there is already an hierarchy of classes (for whatever reason) and two of them need to share some code (generally a utility function). The temptation is to put this code into one of the abstract classes common to both. I guess it comes from the junior developers' fear to create new units of code - be it classes, interfaces or whole modules. It turns out this requires some sort of courage (who would have thought!). So a new protected method gets created in an abstract class.

Two immediate problems come out of this - the fact that the method is placed in the abstract class invites future users of this method to disguise themselves as members of this hierarchy of classes (otherwise they won't get access to this useful method) without real reason. The second problem arises when one tries to refactor the abstract class to a new infrastructure module. This offensive method in many cases knows some abstractions of higher levels and prevents the abstract class from being extracted.

It's generally very easy to find out these "weeds" - try to put the word 'static' before the definition of such method. If the code continues to compile, you can safely extract it out to a utility class (yes, there's nothing wrong in using utility classes with static methods!).

Second case - perhaps the most dangerous one. There's a class that does almost what we want? Let's extend it and override the offending method(s). This specifically creates inheritance for no good reason. In this case the most important thing to look for is a possible violation of the Liskov substitution principle. Can an instance of this new class be used in whatever context an instance of existing class is being used? Probably not. So try to refactor the existing class. Either make it suite your needs or take out the required functionality to another place. Beware of the Conway's Law.

The third case (aka template pattern) may be an evolution of the previous one but may also be created on its own. It's often characterized by an abstract class having methods 'postSomething' and 'preSomething'. Very curious idea - naming a method not according to what it should do but to when it is supposed to be called - sort of callbacks. However, contrary to callbacks which are supposed to react to events, there are no events here. For instance:

public abstract class AbstractCommand {
    public final void execute() {
        validate();
        doExecute();
        postProcess();
    }
    protected abstract void validate();
    protected abstract void doExecute();
    protected abstract void postProcess();
}

Wow! What a code! Excuse me, what exactly this "design" gives? To prevent any future command from forgetting to "validate"? And, as you may guess, 28 out of 30 commands have their 'postProcess' method empty. By the way, another LessAbstractCommand quickly comes up with a method 'doExecute' doing some "common" stuff and calling 'doRealExecute'. Within a couple of levels one quickly gets lost in those "real-real" executes. Great fun to debug as well!

Of course, this is an extreme example, but it's amazing how tempting it is to "establish a flow" and make derived classes only implement "plug" methods. Next time you are considering something of the king, think instead of an easy-to-use set of utilities, or corner stones to be used by future implementors. It's OK if more than one of them would call the same two methods in the same sequence - no harm done. This is not the kind of code that should be reused. With the template method, however intelligent you are, you won't be able to cover for all cases in your "universal" flow and one day your successor will add an ugly "prePostSomething" method implemented in only one subclass.

Finally! Little interfaces - one of my favorite ones. "Many of our objects have a name and a description - let's create a common interface (superclass) NamedEntity (AbstractNamedEntity)!" My favorite saying goes "the fact that both a kangaroo and a soldier are capable of running doesn't mean they should both implement Runnable". The main question you should ask yourself - will there be any code that gets an instance of any NamedEntity and treats it as such? Check yourself thoroughly and refrain from doing it.

I was once swearing hard when I had to study a system where every object that had a 'startup' and a 'shutdown' method was declared to implement a common interface. Surely there was no place in the system that handled all instances of this interface. But imagine what pain it was trying to understand where the 'startup' method of a certain object is called from. One gets 15 places, 14 of which are irrelevant for the specific case - now go find the right one!

So - is inheritance evil? No way! As indicated in a comment to my previous post - "guns don't kill, people do". Thanks, I liked it! But it's amazing how often the inheritance is being abused, this is all I want to say.

My "anti-patterns" - 1

Whole books are written about "anti-patterns" in software design, so I probably won't be saying anything new here. There are several anti-patterns however which I consider sort of "my favorites". I first thought about writing about all of them in one post, but then I recalled one of my readers criticizing me for putting too much statements together. So I'm going to publish them one by one instead.

Visitor pattern

It took me quite a time to grasp the idea of this pattern. Perhaps I'm kind of challenged but I still don't think something that complicated should be used unless really (and I mean really) justified. So my first problem with this construction - it's hardly readable. I hate reading a code where methods have obscure names. I always get lost in the labyrinth of 'visit' and 'accept' calling each other.

However, this is not the single reason. Look at the example from the Wikipedia article.

class CarElementPrintVisitor implements CarElementVisitor {
    public void visit(Wheel wheel) {      
        System.out.println("Visiting " +
            wheel.getName() + " wheel");
    }
 
    public void visit(Engine engine) {
        System.out.println("Visiting engine");
    }
 
    public void visit(Body body) {
        System.out.println("Visiting body");
    }
 
    public void visit(Car car) {      
        System.out.println("Visiting car");
    }
}

I guess I'm going to hurt the feelings of the advocates of this pattern, but this snippet makes me wonder how the pattern made its way to the classical book on object-oriented design. Sorry, but what I really see above is nothing else but a 'switch' statement nicely disguised in methods overloading. Wouldn't it be more straight to simply use 'instanceof' instead? At least the code would be more readable and would not require those obscure 'accept' methods.

Finally, my last objection has to do with modularization of the code. Normally, when extending a class, one doesn't necessarily have to place the derived class in the same module where the base class is. With visitor pattern, the family of classes derived from CarElement is a closed circle. Not only when adding a new derivative one has to go over all visitors, but the derivative has to be in the same module the rest of the family is. Surely, any design assumes something about possible future changes. The same design cannot support more than one direction of changes. This specific one supports adding new visitors but makes it practically impossible to add new elements visited.

To summarize, I have an impression that the visitor pattern is an illusion. It creates more problems than it solves and unnecessarily complicates the code. The only positive side is that it sounds cool when one claims at an interview "I have mastered the visitor pattern!"