While doing a code reviews in a class of studients at Laval University, I heard that some people think that design quality is a very subjective matter. As long as they were using design patterns, the code they developped was clean. In this post, I will explain that everyone should be careful when using design patterns. While there are a lot a reasons to use them (and we should!), some patterns have drawbacks and flaws that should be considered.
Don’t get me wrong, I believe that the work of the Gang of Four (Erich Gamma, Richard Helm, Ralph Johnson et John Vlissides) is very important to the history of software design. They provided a great collection of efficient solutions to solve programming problems in a good way. Still, my point is that a software don’t necessarily need to use a design pattern. One should think about the necessity of a pattern in its code. Moreover, some patterns have flaws and drawbacks that can worsen a software. Furthermore, some patterns might be used for the wrong reason.
I recently was told that the concept of a “clean code” was very subjective. I personally think that the answer to this assumption is that once one know the quality metrics, the design patterns and the architectural patterns, and that he can wrap all this knowledge with well known clean code principles, like the S.O.L.I.D. principles, Demeter’s law or Tell-Don’t-Ask, one should be able to foresee design problems without being subjective at all.
Now, in a previous post concerning the S.O.L.I.D. principles, I told that I would discuss about some flaws with the design patterns in the work of the Gang of Four. Here’s a list of smells people should always have when choosing to use these patterns.
This pattern should only be used when you are restricted with the external interface from a client. If you use this pattern because of an internal interface that don’t match… Well… You should refactor the code instead.
I’ve got a small thing to say about this pattern. I noticed that a lot of programmers that implement the command pattern tends to simply name the Command’s execution method “execute()” for the purpose of doing just like they saw in any design pattern’s book. Why so general? It makes the code hard to read. Be more specific!
This pattern should be used when you have tree-structured data. You would then be able to use groups of objects (branches) the same way as you would use a single object (leaf). Although, I saw people implement a Composite “just for fun”: They though it could be useful someday if they were to have tree-structured data. YAGNI (You Ain’t Gonna Need It) tells you not to do that, since it can block your creativity and kind of force you to have tree-structured data or make you think this is the only solution.
Be careful with this pattern, since it is easy to violate the Single Responsibility Principle. In fact, most of the time, this pattern will. Also, make sure that the decoration does not break the Liskov Substitution Principle.
A Facade is useful when you have a lot of low level classes and when you want some class with high level methods that are the only one needed. The Facade should not be bypassed by a careless programmer. Also, a Facade should not give 1-1 methods, i.e. it should not have high level methods for only one low-level class. Even worse, I once saw a Facade that only redirects to low level methods like a god-class proxy (one class to use them all!). Facepalm, don’t do that. Finally, be careful about the Interface Segregation Principle, i.e. maybe you should have multiple facade instead of only one?
Be careful, this pattern tends to appear during refactor or while doing maintenance because of design problems. The Mediator knows a lot of things. If you have no way to avoid this pattern, just verify if you could not use multiple smaller Mediators instead of a big one god-Mediator.
I’ll only to say like you will see in many design patterns book. The caretaker (which saves the Memento object) should only care about one object and should never modify the Memento object.
This good old Observer… Always there to manage events! Suppose I have an alarm system. I think I will have an observer for this door to raise an alarm if it’s opened. Oh and another observer for this window, and another one that sniffs to make sure the air is OK. So I have 3 observers and 10 observees, so I’ll add a WatchTower, which will observe the observers for managing the alarms…
Be really carefull with Observers. Of course, I was joking above, but this is a really serious matter. Observers can be implemented because of a bad design or because we have not thought enough about when they should be used. But the thing I wanted to point out is that Observers cant give you a big pain in the ass when you will have to debug. You will have sometime to dive deep in the code to find the state of a certain class that make your code crash or loop indefinitely. Use observers with parsimony.
First of all, singletons are hard to test and they makes it hard to have a good unit test coverage. The question you should ask yourself is: Why am I using it? Is it because there is only one in my whole program, or is it because there must not be two? It’s not because there should be one instance of a class running in your code that having two would be necessarly a problem. The huge majority of the time, the singleton can be avoided. If however, you are in a situation that two instances of a class would necessarily be catastrophic, then I suggest this unit-testable version of the singleton pattern, Service Locator.
The visitor might look like a good pattern for many reason. Unfortunately, if you implement one, you will see that it creates loads of dependency cycles in your code, thus making it ugly and very hard to debug and modify. It also breaks the single responsibility principle, most of the time. I suggest you never use this pattern. If you have no other options, check out my post about this variant of the Visitor pattern that have no cycle of dependencies.
I hope that these warnings will be helpful when you will have to choose the right solution to your system.