The Turnstone's Bill

Code Confession : Multi-purpose Methods

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
- (NSManagedObjectID*) permanentObjectID {
  NSManagedObjectID *theID = [self objectID];
    if ( [theID isTemporaryID] ){
        NSError *err=nil;
        BOOL success = [[self managedObjectContext] obtainPermanentIDsForObjects:@[self] error:&err];
        if (!success)
          // Deal with error ... that's another story
    }
    return [self objectID];
}

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;

  1. The name permanentObjectID suggests simple property access but this isn’t what the method does
  2. It’s a function that does two very different things.
  3. 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
- (NSManagedObjectID*) permanentObjectID {
    NSManagedObjectID *theID = [self objectID];

    if ( [theID isTemporaryID] )
        return nil;

    return theID;
}
1
2
3
4
5
6
7
8
9
10
11
- (void) makeObjectIDPermanent {
    if ( [[self objectID] isTemporaryID] ){

        NSError *err;
        NSManagedObjectContext *moc = [self managedObjectContext];

        BOOL success = [moc obtainPermanentIDsForObjects:@[self] error:err];
        if ( !success )
          // Deal with error
    }
}

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).

Summary

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.

Comments