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:

- (NSPersistentStoreCoordinator *) persistentStoreCoordinator {

    if (persistentStoreCoordinator != nil) {
        return persistentStoreCoordinator;
    }

    NSFileManager *fileManager;
    NSString *applicationSupportFolder = nil;
    NSURL *url;
    NSError *error;
    
    fileManager = [NSFileManager defaultManager];
    applicationSupportFolder = [self applicationSupportFolder];
    if ( ![fileManager fileExistsAtPath:applicationSupportFolder isDirectory:NULL] ) {
        [fileManager createDirectoryAtPath:applicationSupportFolder attributes:nil];
    }
    
    url = [NSURL fileURLWithPath: [applicationSupportFolder stringByAppendingPathComponent: @"CD_Cleanup.xml"]];
    persistentStoreCoordinator = [[NSPersistentStoreCoordinator alloc] initWithManagedObjectModel: [self managedObjectModel]];
    if (![persistentStoreCoordinator addPersistentStoreWithType:NSXMLStoreType configuration:nil URL:url options:nil error:&error]){
        [[NSApplication sharedApplication] presentError:error];
    }    

    return persistentStoreCoordinator;
}

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.

- (NSPersistentStoreCoordinator*)persistentStoreCoordinator
{
  if (persistentStoreCoordinator) return persistentStoreCoordinator;
  NSError *error = nil;
  
  NSManagedObjectModel *mom = [self managedObjectModel];
  if (!mom) {
    NSAssert(NO, @"NSManagedObjectModel is nil");
    NSLog(@"%@:%s No model to generate a store from", [self class], _cmd);
    return nil;
  }

  NSFileManager *fileManager = [NSFileManager defaultManager];
  NSString *applicationSupportFolder = [self applicationSupportFolder];
  if ( ![fileManager fileExistsAtPath:applicationSupportFolder isDirectory:NULL] ) {
    if (![fileManager createDirectoryAtPath:applicationSupportFolder attributes:nil]) {
      NSAssert(NO, ([NSString stringWithFormat:@"Failed to create App Support directory %@", applicationSupportFolder]));
      NSLog(@"Failed to create application directory: %@", applicationSupportFolder);
      return nil;
    }
  }

  NSURL *url = [NSURL fileURLWithPath: [applicationSupportFolder stringByAppendingPathComponent: @"CD_Cleanup.xml"]];
  persistentStoreCoordinator = [[NSPersistentStoreCoordinator alloc] initWithManagedObjectModel:mom];
  if (![persistentStoreCoordinator addPersistentStoreWithType:NSXMLStoreType 
                                                configuration:nil 
                                                          URL:url 
                                                      options:nil 
                                                        error:&error]) {
    [[NSApplication sharedApplication] presentError:error];
    [persistentStoreCoordinator release], persistentStoreCoordinator = nil;
    return nil;
  }    

  return persistentStoreCoordinator;
}

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:

- (NSManagedObjectContext *) managedObjectContext {

    if (managedObjectContext != nil) {
        return managedObjectContext;
    }

    NSPersistentStoreCoordinator *coordinator = [self persistentStoreCoordinator];
    if (coordinator != nil) {
        managedObjectContext = [[NSManagedObjectContext alloc] init];
        [managedObjectContext setPersistentStoreCoordinator: coordinator];
    }

    return managedObjectContext;
}

And the revised code:

- (NSManagedObjectContext*)managedObjectContext
{
  if (managedObjectContext) return managedObjectContext;

  NSPersistentStoreCoordinator *coordinator = [self persistentStoreCoordinator];
  if (!coordinator) {
    NSMutableDictionary *dict = [NSMutableDictionary dictionary];
    [dict setValue:@"Failed to initialize the store" forKey:NSLocalizedDescriptionKey];
    [dict setValue:@"There was an error building up the data file.  Please contact support." forKey:NSLocalizedFailureReasonErrorKey];
    NSError *error = [NSError errorWithDomain:@"Zarra" code:8001 userInfo:dict];
    [[NSApplication sharedApplication] presentError:error];
    return nil;
  }
  managedObjectContext = [[NSManagedObjectContext alloc] init];
  [managedObjectContext setPersistentStoreCoordinator: coordinator];

  return managedObjectContext;
}

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!

- (NSApplicationTerminateReply)applicationShouldTerminate:(NSApplication *)sender {

    NSError *error;
    int reply = NSTerminateNow;

    if (managedObjectContext != nil) {
        if ([managedObjectContext commitEditing]) {
            if ([managedObjectContext hasChanges] && ![managedObjectContext save:&error]) {

                // This error handling simply presents error information in a panel with an 
                // "Ok" button, which does not include any attempt at error recovery (meaning, 
                // attempting to fix the error.)  As a result, this implementation will 
                // present the information to the user and then follow up with a panel asking 
                // if the user wishes to "Quit Anyway", without saving the changes.

                // Typically, this process should be altered to include application-specific 
                // recovery steps.  

                BOOL errorResult = [[NSApplication sharedApplication] presentError:error];

                if (errorResult == YES) {
                    reply = NSTerminateCancel;
                } 

                else {

                    int alertReturn = NSRunAlertPanel(nil, @"Could not save changes while quitting. Quit anyway?" , @"Quit anyway", @"Cancel", nil);
                    if (alertReturn == NSAlertAlternateReturn) {
                        reply = NSTerminateCancel;	
                    }
                }
            }
        } 

        else {
            reply = NSTerminateCancel;
        }
    }

    return reply;
}

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:

- (NSApplicationTerminateReply)applicationShouldTerminate:(NSApplication*)sender
{
  if (!managedObjectContext) return NSTerminateNow;

  if (![managedObjectContext commitEditing]) {
    NSLog(@"%@:%s unable to commit editing to terminate", [self class], _cmd);
    return NSTerminateCancel;
  }

  if (![managedObjectContext hasChanges]) return NSTerminateNow;

  NSError *error = nil;
  if (![managedObjectContext save:&error]) {
    BOOL result = [sender presentError:error];
    if (result) return NSTerminateCancel;

    NSString *question = NSLocalizedString(@"Could not save changes while quitting.  Quit anyway?", @"Quit without saves error question message");
    NSString *info = NSLocalizedString(@"Quitting now will lose any changes you have made since the last successful save", @"Quit without saves error question info");
    NSString *quitButton = NSLocalizedString(@"Quit anyway", @"Quit anyway button title");
    NSString *cancelButton = NSLocalizedString(@"Cancel", @"Cancel button title");
    NSAlert *alert = [[NSAlert alloc] init];
    [alert setMessageText:question];
    [alert setInformativeText:info];
    [alert addButtonWithTitle:quitButton];
    [alert addButtonWithTitle:cancelButton];

    int answer = [alert runModal];
    [alert release], alert = nil;
    if (answer == NSAlertAlternateReturn) return NSTerminateCancel;

  }

  return NSTerminateNow;
}

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