October 3, 2005

Pimp My Code, Part 5: Special Apple Sample Code Edition...

Ok, today we take a snippet of sample code from the great mothership herself, Apple, and we compare my style to theirs (or, at least, one of their programmers). Both of these routines work. You might feel one has advantages over the other, and I won't be peeved if you decide mine's not better.

Starting Position: Apple's Code from CoreData Sample Project
/* Change this path/code to point to your App's data store. */
- (NSString *)applicationSupportFolder {
NSString *applicationSupportFolder = nil;
FSRef foundRef;
OSErr err = FSFindFolder(kUserDomain, kApplicationSupportFolderType,
kDontCreateFolder, &foundRef);
if (err != noErr) {
NSRunAlertPanel(@"Alert", @"Can't find application support folder",
@"Quit", nil, nil);
[[NSApplication sharedApplication] terminate:self];
} else {
unsigned char path[1024];
FSRefMakePath(&foundRef, path, sizeof(path));
applicationSupportFolder = [NSString stringWithUTF8String:(char *)path];
applicationSupportFolder = [applicationSupportFolder
stringByAppendingPathComponent:@"ManyToManySQLTest"];
}
return applicationSupportFolder;
}
This code is from the sample application delegate class that is created when you create a new CoreData project in XCode that is NOT document-based.

First Pass: Doll It Up a Bit
// Change this path/code to point to your App's data store.
- (NSString *)applicationSupportFolder;
{
OSErr err = FSFindFolder(kUserDomain, kApplicationSupportFolderType,
kDontCreateFolder, &foundRef);
if (err != noErr) {
NSRunAlertPanel(NSLocalizedString(@"Cannot find folder", @"error title"),
NSLocalizedString(@"Your home folder appears to be missing its
""Application Support"" folder, which is required by many parts of
Mac OS X. You should restore this folder from backups or create a
new account and move your files over to it.", @"error text"), @"Quit",
nil, nil);
[[NSApplication sharedApplication] terminate:self];
}

FSRef foundRef;
unsigned char path[PATH_MAX];
FSRefMakePath(&foundRef, path, sizeof(path));
return [[NSString stringWithUTF8String:(char *)path]
stringByAppendingPathComponent:@"ManyToManySQLTest"];
}
For my first pass at this code I leave it semantically the same, but I apply my personal taste in spacing and variable declaration to it. You might find this makes it more clear or less clear; I think having the variables declared "just-in-time" reduces their scope, and leaves your head with less variables rattling around in it as you are analyzing the top part of the code. It's like, if someone's going to tell you a story, do they start out, "Ok, there's Susan, she's a cosmetologist, and Jenny, who works at a store, and this guy Phil, who's a sleaze from Nordstrom, and also this other person..." or do they start out, "Ok, so Susan is a cosmetologist, and she's working at the Nordstrom counter when this sleaze Phil walks up to her..."

I think, using the latter, it's easier to remember where each name fits into the larger picture.

Also, I change the error strings to suggest what the user should do to get out of the bind she's in. I mean, yes, it's going to be a rare condition. But if it's so rare we don't believe it'll ever happen, let's just exit the program silently if it does. We don't know that it's that rare, so we're duty-bound to have our error message not just describe the severity of the error, but also offer some suggestions on how to work around this problem.

Finally, I localized the errors, because, darn it, EVERY TIME you add an English string to a program, LOCALIZE IT. RIGHT THEN AND THERE. YOU WILL BE SORRY IF YOU WAIT UNTIL THE END. YOU WILL BREAK YOUR PROGRAM DOING LOCALIZATIONS RIGHT AS YOU ARE ABOUT TO SHIP.

Ok, so that's just a brief once-over. Can I do more? As it happens, yes. See that ugly FSRef and OSErr stuff? WTF? I never learned that in Cocoa class. Ah, because that's not part of Cocoa; it's Carbon. Snuck it right in there, didn't they. Of course, because Carbon is 22 years old, they've got to do some munging to get it to deal with modern strings and stuff, including hard-coding the maximum resolved pathlength of 1024 (I replaced it with its symbolic constant, hoping for a better day) . If only there were an easier way, using, like, strings and shit...

Second Pass: Replace Carbon with Cocoa
- (NSString *)applicationSupportFolder;
{
NSArray *applicationSupportDirectoryPaths =
NSSearchPathForDirectoriesInDomains(NSApplicationSupportDirectory,
NSUserDomainMask, YES);
if ([applicationSupportDirectoryPaths count] == 0) {
NSRunAlertPanel(NSLocalizedString(@"Cannot find folder", @"error title"),
NSLocalizedString(@"Your home folder appears to be missing its
""Application Support"" folder, which is required by many parts of Mac
OS X. You should restore this folder from backups or create a new
account for yourself and move your personal files over to it.",
@"error text"), @"Quit", nil, nil);
[[NSApplication sharedApplication] terminate:self];
}

return [[applicationSupportDirectoryPaths objectAtIndex:0]
stringByAppendingPathComponent:[[NSProcessInfo processInfo]
processName]];
}
Check it out now. We've gone from 15 lines of code to 8, by my count. We've eliminated all calls to Carbon and any allocating of buffers ourselves. And, as an added bonus, we look up our process name automatically, so we don't have ask the user to change the code if she changes the program's name.

But, hey, does NSSearchPathForDirectoriesInDomains() actually check to see if the directory it just pointed us to physically exists, or is it just saying, "Here's the name of the directory, hope that works out for you"? Because, if it's the latter, there's really no reason for us to test the status of the return, because if Mac OS X has obviated the Application Support directory altogether then we're going to need to rewrite our app to get things working; there's nothing the user can do for us.

In that case, really, we should think about our method just looking like this:

(Optional) Third Pass: Screw Error Checking
- (NSString *)applicationSupportFolder;
{
return [[NSSearchPathForDirectoriesInDomains(NSApplicationSupportDirectory,
NSUserDomainMask, YES) objectAtIndex:0]
stringByAppendingPathComponent:[[NSProcessInfo processInfo]
processName]];
}
And, voila, we have the one-line version.

This version will raise an exception if Mac OS X is ever changed in an incompatible way, and the paths returned is an empty array. I highly doubt that'll happen, but, if it does, well, an exception is not the worst way to go. I mean, seriously, Apple could change any method in Mac OS X in an incompatible way at any moment, and we can't write alternatives to each call or we end up solving the halting problem.

Some people might say, "Well, I like the previous one better, it's safer, etc," but remember, each line of code you have in your project is a line you have to understand, read past, support, and maintain. That previous version had two extra English strings it added to our localization dictionary, which means you'll be paying all your localizers to translate them.

My point is, those original 15 lines of code actually had a pretty big support footprint for what they actually did. (Especially when the name of the app was hard-coded in like that.) Our goal as programmers is to solve problems (big and small) in a way that causes the smallest support burden.

Labels:

17 Comments:

Blogger Carl Johnson said...

You know, the Halting Problem is only insoluable in general. There's a 50-50 chance that we CAN solve the halting problem for OS X! Them's good odds! And since we won't know until we try, I volunteer you to do it. C'mon Wil, prove to me that OS X will or will not halt: I'm considering offering a $50,000,000 + job bounty for it.*

* Offer actually void. Sucker!

** Yes, I know that the concept of a "halting problem" doesn't really apply to OS X since it has a near infinite amount of possible user input and crashing it (AKA halting) is relatively easy.

October 03, 2005 5:07 AM

 
Anonymous Cameron Hayne said...

A minor quibble:
The "first pass" code won't compile because the 'foundRef' variable is declared too late. A premature optimization?

October 03, 2005 5:16 AM

 
Anonymous Jonathan Johnson said...

Of course, one long line with a lot of sub-method calls can even be worse to maintain, because you have to figure out exactly what that line is doing. I'd much rather see something like:

NSArray *searchPaths = NSSearchPathForDirectoriesInDomains(NSApplicationSupportDirectory,
NSUserDomainMask, YES);
NSString *applicationSupportFolder = [searchPaths objectAtIndex:0];
return [applicationSupportFolder stringByAppendingPathComponent:[[NSProcessInfo processInfo]
processName]];

There. That's code that's easily readable at a glance, and should *ease* the support of the code despite the line count going up by two. Less lines doesn't mean easier to maintain.

October 03, 2005 5:24 AM

 
Anonymous Anonymous said...

NSApplicationSupportDirectory was only defined in OS X 10.4. I think they probably used the Carbon crap so that it worked for pre-Tiger users.

October 03, 2005 6:31 AM

 
Blogger thomas Aylott said...

I love reading opinions about code.
Although I don't program in this language, you have helped mold my opinions about code.

Less code = better code
Declare variables just in time.

I hate having to read the entire book just to find out the name of the bar the character is in at this moment. I should only have to read the paragraph about the bar!
Consarnit!

I deal with a lot of legacy (aka. slapped together by apes) code. I have come to have very strong opinions about how code should be written.

October 03, 2005 6:42 AM

 
Anonymous Jesper said...

"NSApplicationSupportDirectory was only defined in OS X 10.4. I think they probably used the Carbon crap so that it worked for pre-Tiger users."

The code is from a Core Data sample project. Core Data is a Tiger-and-up technology, so while you do have a point (in that it's not supported by 10.3), in this example it doesn't need to be supported by 10.3 and Wil's code does the exact same for all users that can get the app flying in the first place.

October 03, 2005 7:00 AM

 
Anonymous Brian Webster said...

Looking at the second pass of the code, when you call -[NSApplication terminate:], if your delegate opts to delay the termination of the program, then the terminate: method will exit and continue to execute the rest of the code in the calling method. Since we know from the if statement that the array is empty, the return statement will always raise an exception when it tries to access objectAtIndex:0. So if you write something like this, you should either be absolutely certain that NSApplication will actually terminate, put the rest of the method in an else statement, or add a "return nil" after the terminate call.

October 03, 2005 9:06 AM

 
Blogger Wil Shipley said...

Cameron: Nice catch.

JJ: I'm on the fence on this one. I don't like mega-lines myself, but over the years I've come to hate defining a variable that never varies. It confuses the issue for me (why is this a variable?), and also makes the code read out-of-order.

But you're right that mega-lines are kind of messy. I wish that "const" worked a bit better in Obj-C, but it's basically just an invitation to get lots of warnings from the compiler that it's ignoring you.

Brian: You are right; it was an oversight not to also have a return statement after the call to -terminate:. I like the explicit return after a call that you know isn't returning also because it gives the compiler a better idea what's happening, so in general use there's a possibility it'll optimize your code and/or issue warnings that'll end up being important. (Eg, "Hey, this other code can't possibly be reached.")

October 03, 2005 11:23 AM

 
Anonymous Grayson said...

Regarding:
"But, hey, does NSSearchPathForDirectoriesInDomains() actually check to see if the directory it just pointed us to physically exists, or is it just saying, 'Here's the name of the directory, hope that works out for you'?"

In my informal tests, it appears that it just spits out the path that the folder should be at. It doesn't bother to check if the folder exists or not.

October 03, 2005 1:13 PM

 
Anonymous pontus ilbring said...

You're not escaping the quotes around Application Support properly.

"Your home folder appears to be missing its ""Application Support"" folder..."

is three different strings that the compiler then immediately concatenates into one, sans quotes. Whereas

"Your home folder appears to be missing its \"Application Support\" folder..."

is one string, with proper quotes in it.

October 03, 2005 3:52 PM

 
Blogger jay said...

Just want to say that I'm a recent Mac convert, and am enjoying reading your tidbits on Mac programming.

October 03, 2005 7:55 PM

 
Anonymous xyz3 said...

Hmm. If it were up to me, I would create the Application Support folder if it didn't exist - at least in this case. Does the app need the folder? YES! Does the user care that the folder is missing, or does he/she just want things to work? If the app cannot create the Application Support folder, then move on to the nice dialog warnings.

October 04, 2005 8:11 AM

 
Blogger Drew Hamlin said...

15 lines down to 1 line sounds good to me. :)

I'm surprised they even used Carbon in that original check.

October 05, 2005 12:03 AM

 
Anonymous Anonymous said...

Oh my, you started pimping Apple's sample code!

It is often so badly written... Have you ever looked at Quicktime code samples? ImageCapture code samples? OUCH! Makes you wonder how these frameworks are implemented.

October 05, 2005 12:45 PM

 
Anonymous Anonymous said...

Um yes they did slip a bit of Carbon in there.

However, the Carbon example are a little strange all together, as well.

However I did pick up on on thing I kind of like (Carbon though):

switch ( a ) {
case b:
{
// b in brackets
} break;
case c:
{
// c in brackets
} break;
default:
{
// default
} break;
}

A bit off topic, but I remember I learned this from Carbon code and still do it today.

http://brianray.chipy.org

October 05, 2005 7:54 PM

 
Anonymous Anonymous said...

What about:

[@"~/Library/Application Support/MyApp/" stringByExpandingTildeInPath]

October 12, 2005 2:17 PM

 
Blogger Rob said...

This is what I've got in my Leopard/Xcode 3.0 CoreData non-document-based app:

-(NSString *)applicationSupportFolder {
NSArray *paths = NSSearchPathForDirectoriesInDomains(NSApplicationSupportDirectory, NSUserDomainMask, YES);
NSString *basePath = ([paths count] > 0) ? [paths objectAtIndex: 0] : NSTemporaryDirectory();
return [basePath stringByAppendingPathComponent: @"Taskspace"];
}

I can't decice if the NSTemporaryDirectory() call is a nice touch or missing the point.

May 10, 2008 1:06 AM

 

Post a Comment

<< Home