Delicious Monster
Pimp My Code, Part 13: The Pimp Before Christmas, Redux
Well, it's been awhile since I've pimped anything (except other developers' moms), and frankly I feel my pimp muscles starting to atrophy. A reader was nice enough to send me some code today with the following note:
I'm sure there's a better way to do this, and there's probably some entertaining mockery to be had as well. The code's from a motion-sensing alarm thing I wrote, along the lines of iAlertU.
Mockery? I believe sir may have me confused with someone else? At any rate, let's view our victim:
| Original: -awakeFromNib |
| - (void)awakeFromNib { NSRect screenRect = [[NSScreen mainScreen] frame]; NSRect boxRect = [lockBox frame]; [lockBox setFrame:NSMakeRect((screenRect.size.width - boxRect.size.width)/2, (screenRect.size.height - boxRect.size.height)/2,boxRect.size.width, boxRect.size.height)]; NSShadow *lockShad = [NSShadow alloc]; [lockShad setShadowOffset:NSMakeSize(0,-4)]; [lockShad setShadowBlurRadius:8.0]; [lockShad setShadowColor:[NSColor colorWithDeviceWhite:0 alpha:0.75]]; NSMutableParagraphStyle *paraSty = [NSMutableParagraphStyle alloc]; [paraSty setAlignment:NSCenterTextAlignment]; shadAttrs = [[NSDictionary dictionaryWithObjectsAndKeys:[NSFont fontWithName: @"Lucida Grande Bold" size:72],NSFontAttributeName,lockShad,NSShadowAttributeName, [NSColor whiteColor],NSForegroundColorAttributeName,paraSty,NSParagraphStyleAttributeName, nil] retain]; [lockShad release]; [paraSty release]; def = [NSUserDefaults standardUserDefaults]; [def registerDefaults:[NSDictionary dictionaryWithObjects:[NSArray arrayWithObjects: [NSNumber numberWithInt:0],[NSNumber numberWithInt:5],[NSNumber numberWithFloat:0.8], @"System locked.\nDo not touch.",nil] forKeys:[NSArray arrayWithObjects:@"compType", @"sensitivity",@"lockedOpacity",@"lockedText",nil]]]; [self getPrefs]; // Menu icon thing menuItem = [[[NSStatusBar systemStatusBar] statusItemWithLength:24] retain]; menuItemIcon = [[NSImage alloc] initWithContentsOfFile:[[NSBundle mainBundle] pathForImageResource:@"lockicon"]]; menuItemSelIcon = [[NSImage alloc] initWithContentsOfFile:[[NSBundle mainBundle] pathForImageResource:@"lockiconsel"]]; [menuItem setImage:menuItemIcon]; [menuItem setAlternateImage:menuItemSelIcon]; [menuItem setHighlightMode:YES]; [menuItem setMenu:menuItemMenu]; } |
There's some general clean-up to be done here, plus some of this code needs to move. First off, let's rip that defaults registration out of the middle of -awakeFromNib and into +initialize, because the latter is automatically called before any method can be called on this class, and "before everything" is a damn fine time to set up preferences.
| Pimped: New +initialize method |
| + (void)initialize; { [[NSUserDefaults standardUserDefaults] registerDefaults:[NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithInt:0], @"compType", [NSNumber numberWithInt:5], @"sensitivity", [NSNumber numberWithFloat:0.8], @"lockedOpacity", NSLocalizedString(@"System locked.\nDo not touch.", @"panel message"), @"lockedText", nil]]; } |
Note I also don't store off +standardUserDefaults in a temporary variable, because it's really fast to call this method and very self-documenting, and more variables == more clutter. And I've made the English text for the locked message localizable, which you should always do as a matter of course.
Let me say this again in slow motion: NEVER type in ANY English string without typing NSLocalizedString() around it! This will save you SO MUCH HASSLE later on when your app is popular. Remember that enterprising polyglots can localize your code from just the binary you ship if you follow a few rules of localization, so you may wake up one day and find that someone from across the world has mailed you a your app in another language. It's a fuzzy feeling and it gets you instant market-share.
Also note I'm using +dictionaryWithObjectsAndKeys: instead of +dictionaryWithObjects:forKeys:, the latter of which is obviously wordier AND harder to read and edit.
Next off, we're going to move the text attributes code out of -awakeFromNib as well, and put it into his code right before it gets used. Since I don't actually have this code, I'm just going to pretend, but imagine there's a method in which he's about to set the attributed string value of a text field... this code would be inserted right before that happened:
| Pimped: Insert right before attributes are used |
| [...some code...] static NSDictionary *shadowedTextAttributes = nil; if (!shadowedTextAttributes) { NSShadow *lockShadow = [[[NSShadow alloc] init] autorelease]; [lockShadow setShadowOffset:NSMakeSize(0,-4)]; [lockShadow setShadowBlurRadius:8.0]; [lockShadow setShadowColor:[NSColor colorWithDeviceWhite:0 alpha:0.75]]; NSMutableParagraphStyle *paragraphStyle = [[[NSMutableParagraphStyle alloc] init] autorelease]; [paragraphStyle setAlignment:NSCenterTextAlignment]; shadowedTextAttributes = [[NSDictionary alloc] initWithObjectsAndKeys:[NSFont boldSystemFontOfSize:72], NSFontAttributeName, lockShadow, NSShadowAttributeName, [NSColor whiteColor], NSForegroundColorAttributeName, paragraphStyle, NSParagraphStyleAttributeName, nil]; } [...actually use shadowedTextAttributes here...] [...more code...] |
Why do we do this in the main code instead of in one of the class initialization methods? First, because this offers us better locality of code -- when we see "shadowedTextAttributes" being use we don't wonder what the heck it is, and have to scroll to the top of the file to find out. It's right there. Similarly, if we want to, say, change the size of the text, we just find where we use it and the size is right there. Second, and related, we don't forget that we set up "shadowedTextAttributes" this way -- if we delete the code that uses it, we'll quickly see that we should delete the "shadowedTextAttributes" setup code as well. You would be amazed how many times I've seen classes that set up variables in their +initialize or -awakeFromNib methods that they never use any more. There's no compiler warning if the variable is declared at the top level, but there is if it's declared inline like this. Finally, our program launches faster, because we are even more lazy about initializing (or not initializing) our state until the very second we need it. The biggest performance wins you will find are always to not do any work until you absolutely need to.
I also use +boldSystemFontOfSize: instead of typing in "Lucida Grande Bold" by hand, which reduces the chances of a typo and also means our font will automatically change when Apple redoes their fonts for Leopard. (Right, Apple? Right?... Apple? Anyone?)
I'm going to remove this code, because if the NIB is set up correctly the 'lockBox' view should auto-center itself anyways. If for some reason it didn't, this is what the code would look like pimped out a bit -- note that we get the screenRect of the screen our view is actually on, instead of arbitrarily picking the main screen, and that we don't assume a screen's rect starts with 0.
| Pimped: Delete this |
| NSRect screenRect = [[[lockBox window] screen] frame]; NSRect boxRect = [lockBox frame]; [lockBox setFrame:NSMakeRect(NSMinX(screenRect) + (NSWidth(screenRect) - NSWidth(boxRect))/2, NSMinY(screenRect) + (NSHeight(screenRect) - NSHeight(boxRect))/2, NSWidth(boxRect), NSWidth(boxRect)]; |
Finally, that leaves us with the now-gutted -awakeFromNib, which I have rewritten to not use as many temporary variables (or any at all, actually), to be less wordy when looking up images. Also, I don't know what -getPrefs does, but I'm guessing it can at least be made private:
| Pimped: Final -awakeFromNib |
| - (void)awakeFromNib; { [self __getPreferences]; // Menu icon thing menuItem = [[[NSStatusBar systemStatusBar] statusItemWithLength:24] retain]; [menuItem setImage:[NSImage imageNamed:@"lockicon"]]; [menuItem setAlternateImage:[NSImage imageNamed:@"lockiconsel"]]; [menuItem setHighlightMode:YES]; [menuItem setMenu:menuItemMenu]; } |
And that's how we pimp it up in my house, nerds and nerdettes. So, happy holidays, and don't spend all your savings snorting coke off of hoo... oh, wait, I have One More Thing!
No, it's not an Apple Phone, it's... MORE CODE. Yes, our friend actually mailed me TWO snippets of code, so we get to double our fun today.
Also, a method from another application, this one a game cache manager:
| Original: -applicationWillFinishLaunching: |
| -(void)applicationWillFinishLaunching:(NSNotification *)aNotification { // Get cache-file contents NSString *fileCont = [NSString stringWithContentsOfFile: [@"~/Library/Application Support/Unreal Tournament 2004/Cache/cache.ini" stringByStandardizingPath]]; NSArray *lines = [fileCont componentsSeparatedByString:@"\n"]; // Split it up NSEnumerator *lineEnum = [lines objectEnumerator]; // Enumerator to iterate through all the lines NSScanner *scanner; // String-parser scanny thingy NSString *tmpStr; // set to full line in loop NSString *idStr = nil; // ID string NSString *nameStr = nil; // Name string NSString *fileExt; // File extension NSString *typeStr; // Type string, also folder to copy to NSFileManager *fileMan = [NSFileManager defaultManager]; // File manager - used to get the file attributes NSDictionary *fattrs; // File attributes holder cacheArray = [[NSMutableArray alloc] initWithCapacity:([lines count]-1)]; // Initialize the main array [lineEnum nextObject]; // Skip the "[Cache]" header while(tmpStr = [lineEnum nextObject]) { scanner = [NSScanner scannerWithString:tmpStr]; [scanner scanUpToString:@"=" intoString:&idStr]; // get ID [scanner scanString:@"=" intoString:nil]; // skip the = [scanner scanUpToString:@"" intoString:&nameStr]; // get name fileExt = [nameStr pathExtension]; // get extension //Figure out where it goes if([fileExt isEqualToString:@"ut2"]) typeStr = @"Maps"; else if([fileExt isEqualToString:@"ogg"]) typeStr = @"Music"; else if([fileExt isEqualToString:@"utx"]) typeStr = @"Textures"; else if([fileExt isEqualToString:@"usx"]) typeStr = @"StaticMeshes"; else typeStr = @"System"; nameStr = [nameStr stringByDeletingPathExtension]; fattrs = [fileMan fileAttributesAtPath:[self pathToCacheFile:idStr] traverseLink:YES]; // get file attributes // make the final dictionary [cacheArray addObject:[NSDictionary dictionaryWithObjects:[NSArray arrayWithObjects: nameStr,idStr,typeStr,[[fattrs fileModificationDate] descriptionWithCalendarFormat: @"%b %e, %Y" timeZone:nil locale:nil],fileExt,nil] forKeys:[NSArray arrayWithObjects:@"name",@"id",@"type",@"date",@"ext",nil]]]; } maxIndex = [cacheArray count]-1; // set the index properly [table setDataSource:self]; [table setDelegate:self]; [table display]; // make the table update } |
Ok, let's start with the code we're going to cut, because (sing it along with me) the line of code we cut is the line of code we don't ever debug. I'm going to delete the "maxIndex" ivar (towards the end of the method), since asking an array for its count is O(1) and it's MUCH less fragile and much more readable to just use "[cacheArray count]" whenever you need that count, instead of caching an int at launch time.
Let me stress that I hate instance variables: they add needless complexity to code most of the time. Objective-C is mostly self-documenting; it's much clearer to me and any casual observer what "[cacheArray count]" is (the count of the cacheArray, maybe?) than what "maxIndex" is. DO NOT CACHE VALUES THAT ARE O(1) TO RECOMPUTE.
Also, there's a problem here, in that if there are NO lines in the file we read in, maxIndex will be set to -1 or NSNotFound (I don't know if it's unsigned or signed), and either value is likely to cause subsequent code to act anomolously (which is why we normally use 'count' instead of 'max index' for arrays and sets -- 'count' works for empty collections, and 'max index' is undefined). Not caching this count makes us face up to "cacheArray"'s possible emptiness every time we use it.
| Pimped: delete this |
| maxIndex = [cacheArray count]-1; // set the index properly |
And while I'm in a deleting mood, let's change our tableView to use bindings in NIB, so I can delete these three lines, which shouldn't exist anyways because the dataSource and delegate should be set up in NIB even if you AREN'T using bindings, which you should:
| Pimped: delete this |
| [table setDataSource:self]; [table setDelegate:self]; [table display]; // make the table update |
Ah, that's getting better.
Now let's rewrite the main method, shall we?"
| Pimped: -applicationWillFinishLaunching: |
| -(void)applicationWillFinishLaunching:(NSNotification *)notification; { NSString *fileContents = [NSString stringWithContentsOfFile:[[NSString pathWithComponents: [NSArray arrayWithObjects:[NSSearchPathForDirectoriesInDomains(NSApplicationSupportDirectory, NSUserDomainMask, YES) lastObject], @"Unreal Tournament 2004", @"Cache", @"cache.ini", nil]] stringByStandardizingPath]]; // Get cache-file contents cacheArray = [[NSMutableArray alloc] init]; // Initialize the main array NSEnumerator *lineEnumerator = [[fileContents componentsSeparatedByString:@"\n"] objectEnumerator]; [lineEnumerator nextObject]; // Skip the "[Cache]" header NSString *lineString; // set to full line in loop while (lineString = [lineEnumerator nextObject]) { // parse the line NSScanner *scanner = [NSScanner scannerWithString:lineString]; NSString *idString = nil; if (![scanner scanUpToString:@"=" intoString:&idString]) // get ID break; if (![scanner scanString:@"=" intoString:nil]) // skip the = break; NSString *nameString = nil; if (![scanner scanUpToString:@"" intoString:&nameString]) // get name break; // figure out where it goes NSString *pathExtension = [nameString pathExtension]; static NSDictionary *pathExtensionsToDirectoryNames = nil; if (!pathExtensionsToDirectoryNames) pathExtensionsToDirectoryNames = [[NSDictionary alloc] initWithObjectsAndKeys:@"Maps", @"ut2", @"Music", @"ogg", @"Textures", @"utx", @"StaticMeshes", @"usx", nil]; NSString *typeString = [pathExtensionsToDirectoryNames objectForKey:pathExtension]; // Type string, also folder to copy to if (!typeString) typeString = @"System"; NSDictionary *fileAttributes = [[NSFileManager defaultManager] fileAttributesAtPath:[self pathToCacheFile:idString] traverseLink:YES]; NSDictionary *cacheDictionary = [NSDictionary dictionaryWithObjectsAndKeys:[nameString stringByDeletingPathExtension], @"name", idString, @"id", typeString, @"type", [[fileAttributes fileModificationDate] descriptionWithCalendarFormat:@"%b %e, %Y" timeZone:nil locale:nil], @"date", pathExtension, @"ext", nil]; [cacheArray addObject:cacheDictionary]; } } |
Starting at the end, I've added "cacheDictionary" as a temporary ivar because I really want for there to be a first-class object here instead of an NSDictionary. Approximately 100% of the time that I've used NSDictionaries as a cheapie way to store data structures I've discovered myself adding code to that data in some other class, and then 100% of the time I hit my head at some point and say, "Duh, code + data = objects" and I write a class instead of using an NSDictionary and my life is a lot better. Don't make the same mistakes I did!
Also, it bothers me that we're storing NSDates in this dictionary as strings -- we're losing data in our model layer for no good reason, and forcing a particular style of internationalization in the model as well. I'd much rather we store this as an NSDate and then apply formatting in a (-gasp-) NSDateFormatter when we actually display the value. This has the double-advantage of letting us set it up in NIB. Hey, this NIB thing is like a theme with me, isn't it?
I didn't change either of those things because I don't have all the relevant code, but I'm calling them out here as something I'd want fixed from my team.
Other changes I've made include moving variable declarations down to where they are used, which we've talked about before -- it reduces the scope of variables, and having a short-lived variable is a nice step towards not having a variable, and you know how I hate variables. It also increases readability by not introducing a bunch of extraneous concepts in different places in the code.
I've also renamed variables to remove all but approved abbreviations, and after that I removed a bunch of comments which I found entirely self-evident, because I find this kind of comment worse than no comment. "nameString" is "// Name string"? No shit? Well, what's "idString"? Oh, it's the "// ID string"? I'll be damned. Wait, wait, don't tell me what "fileExtension" is, I want to guess...
I replaced a bunch of if/elses with a single dictionary lookup, which happens to be more efficient, but also makes it a lot clearer at a glance -- we're going to translate "pathExtension" into "typeString", and that happens on one line now instead of over eight or so. Now there's no chance for me to have a typo on one of the lines that I miss forever ("oops, there's a ! in one of the if statements, that condition doesn't work!").
I've added some trivial error-checking in our parsing code, so if the file is corrupted we'll bail early and not try to copy something invalid to someplace unknown. Failing nicely is always a good thing to do -- I don't know exactly what the later code does, so there may be more error-checking to be done here, but I'd like to note you don't have to always, like, create an NSError and recovery code and localized error panels to handle every stupid error condition. But you SHOULD always fail gracefully. When you are parsing anything from the disk, assume the disk hates you and wants to kill you. We have enough customers with Delicious Library now that we've actually had more than a couple of plain old cosmic-ray-zapped data files show up (eg, every 3,000th character in the file was turned into "~" or some such).
Finally, working our way back to the start of the method, I've changed the way we create the path to be more verbose. I've seen NEXTSTEP switch filesystems TOO many times to EVER type a path separator, EVER. Sure, sure, you say, Mac OS X will use "/" forever -- except I've seen this same OS run on Windows FAT32 ("\") and I've seen it run on UFS ("/") and I've seen it run on HFS (":", now translated to "/"), so I'm not buying it. Don't use "~" for the home directory, either -- don't use UNIXisms at this layer, and don't assume the application support directory is where you think it is in the directory structure. Give the Apple guys the chance to move stuff around between releases, and always use "NSSearchPathForDirectoriesInDomains()" instead.
Well, that's it for now. Have a reasonably suicide-free holiday season, if you can. Seriously, we're all miserable here -- just survive the season, and you're ahead of the game. If you get too sad, try doing something nice for somebody -- it'll make you feel good inside and, hey, you might get laid. (My personal plan is to stay drunk and happy until the sun comes out again in Feburary.)
Labels: code