29
May
2008
 

Coding Practice: Cleaning up the default Core Data project

by Marcus Zarra

In this entry I am going to do something a little different. Instead of showing some wicked code, I am going to reformat one of Apple’s default templates.

Apple writes some amazing code. Unfortunately a lot of their templates demonstrate simply terrible coding practices. What is worse, people take these templates and assume that they are the proper practice for coding in Objective-C and propagate that poor code into their own projects.

The first example I am going to tackle is the AppDelegate class that is auto generated in the Core Data template. This is not the Core Data Document template but the standard application template.

persistentStoreCoordinator

This method is not the worst in the list but will help us get into the flow of things. The original method:

Could be worse. There is a lot of wasted whitespace in the method itself but the majority of the problems are error checking. Here is the cleaned up method.

The first change I made is in the conditional check for nil on the persistentStoreCoordinator. This can be cleaned up to make it a simple one line of code rather than three. It is a waste of whitespace and makes the line look far more complicated than it is. Its simple, make it look simple.

Next I changed the order of things by getting a reference to the NSManagedObjectModel first. If the model is nil then the rest is a waste of time. Log the error and assert it as this is a show stopper in every core data application that I can think of.

When building the application support folder, check to make sure it succeeds! The original code just assumes that the directory creation will work. If it doesn’t for whatever reason is a failure and stops everything coming after it.

At this point you might wonder why I have both an assertion and a log statement. I do this because I turn off assertions in the production code. I would rather have a softer failure in production than a hard crash. However in Development I want the code to slap me in the face when something goes wrong.

The last change is if there is an issue with adding the store to the coordinator. In this case the error is presented to the user but I added a release to the coordinator. If this doesn’t happen, any re-entrant to this method is going to be in for a nasty surprise when they get back an empty coordinator!

Last piece of advice on this code. If you release an object, set its pointer to nil! You released it, your done with it. Why have it hang around and potentially crash your code. Point to nil and let the failure of accessing it later be much softer.

managedObjectContext

Next up on the block is the context generation code itself. This one is also not that bad other than some back-ass-wards logic statements and no error handling. First the original code:

And the revised code:

The first thing I did (and always do) is reverse and clean up the conditionals. Putting the happy path inside of the conditional is just dumb. Makes the code damn hard to read and maintainability goes out the window also.

The other change was to present an error to the user if something went wrong. This is the single point that every other piece of code has to touch to use Core Data so it makes sense to present the error here. As discussed in other articles, I am using an NSError to scream about the failure and then return a nil.

applicationShouldTerminate:

Here is the last one I will touch on today and I have definitely saved the worst for last!

This one just makes me want to cry. Follow that happy path, go ahead, I’ll wait.

Not very clean cut is it? It goes in and out of several conditions, else statements, an error and finally an alert panel. Code like this makes people think that Objective-C is hard! Try this version instead:

In this revised method, all of the logic is reversed. The happy path is now along the left edge of the method and much easier to follow. First, if there is no context, terminate now. If we cannot commit any active editing, give up. If there are no changes, terminate now. All of those are now extremely easy to follow and the bulk of the situations are taken care of. This leaves us with only one issue remaining, an actual honest failure.

When we try to save we look at the result. If there is a failure in the save we present it to the application that is passed (why not, its there!) to the method. After that we give the user a choice, just like in the original method to quit anyway or abort the application termination. You might notice that my alert message is a lot more code than the original. That is for two reasons. First I do not like to use the C calls and second, my message is localized. Always, always, always localize any strings that are going to be presented to the user. Even if the application will never, ever be localized, do it anyway. You will thank me down the road when that never ever turns into now!

Conclusion

Now I doubt that Apple is using code like this themselves. It is clearly quick and dirty code that they put together and never went back to revisit it. I get that and totally understand. However WE should never use code like this in our applications. It is asking for trouble and people will laugh at us when they see it.

There is a taste of my coding style. I like the happy path to be on the left; I think the “single return” mythology is a bunch of crap and you should failure early and return often.

Comments? I would love to hear them. If you are going to comment on the multiple returns, don’t bother, you will be ignored.

Per bbum’s suggestion this new code is filed under radar #5972712

Comments

jason says:

Great post Marcus!

I always take a sweep through and clean up autogenerated code, regardless of the language/environment/etc. since my style might not be the same as whoever wrote the original boilerplate. I’m also completely in favor of multiple returns, so it’s nice to finally have some agreement on it.

bbum says:

And the radar # with the cleaned up code attached to it is what….?

:)

(Thanks for doing this)

cocoauser says:

Is there coding style guide on the web or elsewhere that covers the issues you did in your post?
(any for Cocoa? I’ve seen naming guides but it seems to me a lot of code I see still has 1-2 character names for variables. Even Hillegass does this in some of his examples. I feel like I’m still living in the 70’s)
And you’re right, a lot of Apple example code finds it’s way into production code or can be an augmentation of it.

evan says:

The problem with early returns is that they make debugging much more difficult. Especially if you didn’t write the code. And double-especially if you’re debugging memory leaks.

Trivial early bailouts are one thing, but sprinkling early returns all over the place is simply not maintainable.

Marcus Zarra says:

cocoauser,

None that I am aware of. Just a few people who are willing to hang it all out there and announce how they style their code. Like most languages each developer has their own style of code. The goal here is to help some people that think the templates are a _good_ example which they are not — at least for style.

Evan,

Someone who can’t read the entire article is hardly in a position to give coding advice :)

shivaprasad2452 says:

i have doing document based application .. i have to load a window called template window inital after user select one template i have to load document window…. i done this my loading template window in applicationWillFinishLaunching after saving when user double click on app it must load document window…. but it loading template window how to avoid that….. please sugesst…

Marcus Zarra says:

shivaprasad2452,

A question like that is better answered on Apple’s Cocoa Mailing List which you can find at http://lists.apple.com.