I recently read the book Clean Code by Robert Martin. I really enjoyed it, especially the parts where he shows you some dirty code and tears it apart, refactoring and refactoring until it’s a thing of beauty. While reading these sections I couldn’t help but notice that many of the bad code practices were things I’m guilty of.
I’m going to try confessing my own coding sins in the hope that by unpicking bad code I’ll learn more about exactly why it’s bad.
So here’s my first confession.
The bad code
Here’s a category method on
NSManagedObject that I wrote.
1 2 3 4 5 6 7 8 9 10
Looking at this now is rather painful. Why the heck did I write it in the first place?
Well here’s my excuse. I use permanent
NSManagedObjectID’s to lookup specific objects without holding on to the actual
NSManagedObject. I also use them to save object identity information in preferences (eg the last viewed object etc). The problem is that the standard cocoa method
objectID is not guaranteed to return the same
NSManagedObjectID unless that object id has first been made permanent. Since I rely on object id’s being permanent in quite a few parts of my code, I ended up repeating the block of code above in many places. In the interest of keeping to the DRY principle I decided to capture it all in a method.
I can see three main things wrong with this code;
- The name
permanentObjectIDsuggests simple property access but this isn’t what the method does
- It’s a function that does two very different things.
- The method doesn’t always return a permanent object id (eg if an error occurs) but it’s name suggests it should
These problems collaborate to produce WTF code at the call site because sometimes I’ll be calling
permanentObjectID with the expectation that the id is already permanent. At other times I might know full well that the id is not permanent but will call this method to make it so. This greatly reduces readability and searchability of code because the method name no longer describes the purpose of the method call. One reason that this is important is because making the object id permanent at the wrong point can lead to a mismatch between objectID’s of the same object on different NSManagedObjectContexts.
My solution was to make two separate methods like this;
1 2 3 4 5 6 7 8
1 2 3 4 5 6 7 8 9 10 11
Although this leads to slightly more code, it means that these two methods don’t overlap at all. It also allows me to be really explicit when making an object id permanent and to make the
permanentObjectID method behave in a way that befits its status as a noun (ie it should behave like a property).
I think this case is a pretty good example of a Schizophrenic method. It had two really different purposes, and one of them was pretty much completely hidden by the method name. The mistake was reasonably easy to see in this example, but I’ll be on the lookout for more subtle examples of this kind of thing in my code from now on.