I have a codebase where the programmer tended to wrap things up in areas that don’t make sense. For example, given an Error log we have you can log via
ErrorLog.Log(ex, "friendly message");
He added various other means to accomplish the exact same task. E.G.
SomeClass.Log(ex, "friendly message");
Which simply turns around and calls the first method. This adds levels of complexity with no added benefit. Is there an anti-pattern to describe this?
14
It is only worthwhile to call some bad coding habit an Antipattern if it is reasonably widespread.
The rest we just call “rubbish code” …
If I was to suggest a name for this particular bad habit, it would be “Obsessive Abstraction Disorder” 🙂
1
No, it’s not a anti pattern but I would raise the following issues.
Violation of Single Responsibility Princple
SRP says that a class should have one reason to change. Adding a logger method means that the class also have to change if the logging logic should be changed.
Violation of a is-a
relationship
Base classes are not toolboxes where you can add several convenience methods.
Doing so effectively couples all inherited classes to the implementations that the base class has.
Compare that with composition.
1
Not that this makes it acceptable but this could just be an unfinished refactoring. Maybe the SomeClass.log() had it’s own logic an was implemented first. And later they realized that they should be using ErrorLog.Log(). So rather than change 100 places where SomeClass.Log() is called, they just delegated to the ErrorLog.Log(). Personally I would change all the references to ErrorLog.Log() or at least comment SomeClass.log() to say why it delegates the way it does.
Just something to consider.
The term “Lasagna Code” comes to my mind, though apparently it means different things to different people. My interpretation has always been “layered for the sake of being layered”.
1
I’m not sure to describe this as an offense to the single responsibility principle, because if there is a change to the ErrorLog method signature (the method that is called by SomeClass), every client code that calls this method will fail.
There is obviously a way to use inheritance (or making classes that needs logging implementing a logging interface) there.
2
Or we could look at this as a “teachable moment” 🙂
Your developer may be halfway to a good idea. Suppose you want to have the requirement that all of your business objects are capable of intelligent logging. You might define:
public interface IBusinessObjectLogger
{
void Log(Exception ex, string logMessage)
}
Now internal to your objects you can use the ErrorLog object to actually do the logging while SomeClass code adds object specific value to the log message. You can then further extend your logging APIs to implement file based, db based or message based logging functionality without touching you business objects.
I’m not sure this is automatically evil.
If you’re calling SomeClass.Log it most certainly is evil but if Log is only being used from WITHIN SomeClass it’s cutting down the coupling and I would call it acceptable.
This is actually pretty common in certain coding styles, and the basics of the line of thought are not, in themselves, an anti-pattern.
It’s very probably the result of someone coding by necessity without knowledge of the wider codebase: “OK, this code needs to log an error, but that function is not the code’s primary purpose. Therefore, I need a method/class that will do that for me”. That’s a good way of thinking; but, without knowing ErrorLog existed, they created SomeClass. They then found ErrorLog at some later point, and rather than replace all usages of the method they’d put in, they made their method call ErrorLog. That is where this becomes a problem.
Accidental complexity is complexity that arises in computer programs or their development process which is non-essential to the problem to be solved. While essential complexity is inherent and unavoidable, accidental complexity is caused by the approach chosen to solve the problem.
http://en.wikipedia.org/wiki/Accidental_complexity
It may be an abuse/misunderstanding of the Facade Pattern. I have seen similar things in a codebase I’ve worked with. The developers went through a phase when they were crazy about design patterns without understanding the overarching principles of OO design.
A misuse of the Facade pattern results in a blurring of the levels of abstraction across the layers of the application.
What this programmer is doing is wrapping some code, so that it does not have a direct dependency on the logging module. Without more specific information, it’s not possible to give a more specific pattern. (That these wrappers are not useful is your opinion which is impossible to agree or disagree with without much more information.)
Patterns that this may fall under are Proxy, Delegate, Decorator, Composite or some such which have to do with wrapping, hiding or distributing calls. It may one of such things in an incomplete state of development.
I could imagine it to be cultural difference. Maybe there is a good reason for having duplicate functionality in this particular code base. I could image ease of writing to be such a reason. The Python programmer in me would still say it is bad, since “There should be one– and preferably only one –obvious way to do it.”, but some Perl guys might be accustomed to the fact that “There’s more than one way to do it”.
2