Saturday, February 9
Mr. Rentczchxh has posted my talk from C4, and if you would enjoy watching a talk without paying, you can watch it. It's on hype, and how I generate it, but it also touches on other topics concerning having your own software company, like making good software, bundling, getting into stores, having sex with cylons, &c.
Watch it!Or don't.Labels: advice, business, code, interface design, mac community
Tuesday, December 18
It's been a crazy couple of weeks for me...
First off, Lucas Newman is leaving Delicious Monster -- as of January 1 he will be an iPhone engineer. This is an amazing opportunity for him, one I would never ask a friend to pass up. We remain buddies, although I'm running around Zoka these last couple weeks telling every girl I see that Lucas was secretly super-hot for her and is leaving now, which I think is starting to annoy him. Although, honestly, they'll probably all end up throwing themselves at him and he'll end up on top, again.
For those keeping score at home, this makes Mike Matas, Scott Maier, Tim Omernick, Drew Hamlin, and Lucas Newman that Apple has hired out of my employ. Yes, in fact, 100% of Delicious Monster's ex-employees are now working for Apple! You'd almost think Apple would start to pay me to train people for them. Oh, well. It's every kid's dream to work there, I can't say I blame them. Heck, I might work for Apple myself if they ever asked. And, like, wanted to give me EIGHTY ZILLION DOLLARS.
Also, seriously, if you want to work for Apple, you MIGHT want to, you know... GET TO KNOW ME.
--
Mike Lee is staying at Delicious Monster -- for now... DUM DUM DUM! You have to figure he's playing the various Apple teams off each other -- when you work at Delicious Monster, you don't jump for the girl that asks you to dance. Mike's all: "CoreAudio? Don't waste my time, sweetheart." "OS X Server? I'm sorry, you're not even getting an interview." "Ali Ozer and Scott Forstall got into a fistfight over me at lunch today? Now, see, these guys understand what kind of ball we are playing."
--
I realized tonight that I had yet another problem with CoreData, and it was a doozy, and not something where I could just put a hack on it. In fact, it was indicative of a fundamental architecture mismatch that I've been struggling with since I started this project.
So, this is a little vague, but I thought it might be important to document the process. Basically, when I bang up against a wall, I start looking bigger and bigger and bigger. Like, imagine I'm having trouble with a crumbling wall in an aqueduct -- my programmers brain does this: "Ok, why did I build this wall?" To keep the water in. "Why do I have water?" Because you need that to turn the water-wheel. "Is there some other way to turn it?" Not easily. "Why must it turn?" To power the grinder. "What needs grinding?" Corn. "Is there some other way to grind it?"
I'll get to truly huge things, where I start asking if the world even needs an app that catalogs books and DVDs and now boardgames when we could all be under five feet of water in a few years. Then it's time to take a nap and wake up and start again.
But my point is, you HAVE to question all the basic assumptions that led you to where you are, or you end up spending all your time
writing the wrong code. I have always said that if you give me a perfectly spec'ed out program (one with a spec that can actually work, that I'm not going to have to modify as I go along), I can write that program for you in days. Always. The problem with coding is (a) fighting with frameworks, and (b) trying to figure out how the program should look, work, and interact even as we code it.
So we end up spending a lot of times fixing bugs in code that we really shouldn't have written in the first place -- code that doesn't really help the user, that just makes the app more complex, that is for a feature that never should have been put in, or is interacting with the user incorrectly and we're just putting spackle on a wall that's crumbling.
So here I am, tonight, running into my 1,000th bug with the fundamental mis-architecture in CoreData, which is that interacts with the UI layer and the disk layer / undo layer all using the same mechanism. They all rely on -didChangeValueForKey:, which is a huge mistake, because it means that, as a programmer, I can't sneak any data in -- I can't change a value without it creating an undo event.
Consider if, for example, I had a clock and its hands were CoreData objects. As they move forward through time, their position updates, so I'd tell them to update. And each time I did, an undo event would get pushed -- so the user actually could undo time.
This is obviously a contrived example, but it also points to the fundamental problem -- CoreData objects can't mix undoable and non-undoable changes.
So I've been struggling for three years now, trying to bend and hack and cajole CoreData's undo architecture into allowing me to do some actions synchronously and some asynchronously. (For instance, obviously, once the program has downloaded a cover from Amazon in a background thread, you don't want to UNDO the download -- it's not actually a state change, it's just a cache change -- yet, by default we end up with an undo event on the stack, in the MIDDLE of whatever the user is actually doing in the foreground.)
Fight fight gnash gnash complain complain. Tonight I hit on it. I needed to step back. Why isn't this working? Because undo wasn't designed this way in CoreData.
Well, I have undo in Delicious Library 1. It's not "magic" like with CoreData, but it works. In fact, now that I am thinking about it -- I've spent months and hundreds of lines of code trying to get CoreData's "magic" undo to work, when, in fact, there are really only FOUR actions that are ever undone:
1) Add a book -- undo to delete it
2) Delete a book -- undo to add it back
3) Change a property on a book, like its title or author -- undo to change it back
4) Make a loan -- undo to return the book
5) Return a book -- undo to re-make the loan
That's... about it. SO WHY HAVE I SPENT ALL THIS TIME TRYING TO GET COREDATA'S MAGIC SYSTEM TO WORK?
There's only five damn methods, at the top level, that need to participate in undo. It's pretty obvious I should be managing my OWN undoManager, turn off the one in CoreData, and just use CoreData for what it is EXTREMELY good at, which is minimal change tracking and fetching and storing data VERY VERY quickly.
Suddenly all these issues I've been having disappear. I don't have strange extra undo events on my stack when I fault in an object, because although CoreData might think my object changed, it's not driving the undo manager any more -- and when it goes to save, it's going to quickly discover there's no real substantive changes and just discard the whole event.
I don't have to try to work around some undo events by turning undo on and off, which required me to flush CoreData's transactions queue by hand, which was extremely sketchy because if you do it in some circumstances (eg, the middle of inserting a new object) the object will be corrupted.
--
I haven't started this yet -- I'll try it tomorrow. It's nice -- it'll pick up a bunch of the remaining issues I'm having in DL2, and should give us a good solid beta. The important thing here is, I was just too married to part of the code. I was so into using CoreData's magic undo that I kept going farther and farther to make it work, when I really needed to say, "Ok, this doesn't work in this situation, I'm doing my own undo in 40 lines of code."
Labels: code, mac community, stories
Wednesday, May 30
I ask you, grasshopper, which is better: flexible code or tiny code?
"Ah," you exclaim, "Learned master, it is a trick question: code which is tiny yet flexible is best!"
WRONG! Tiny code is always best. Now you must carry water up the hill for the rest of the day.
--
What can we learn from this simple tale? Well, one thing is, I'm not very good at writing stories. But is there something deeper?
When I first started coding, way, long ago (on a PDP-11, which was essentially when I'd get eleven Pterodactyl Dinosaurs to sit down and do some Processing for me), I thought code should be flexible at all costs. If I were creating, say, a program to write a one-line message to the screens of other people logged in to the same machine, I'd write it, say, so you could plug in other kinds of screen-manipulation packages besides curses(3). Even though, you know, none would be invented until "time sharing" was something you did with condos, not processors.
The fundamental nature of coding is that our task, as programmers, is to recognize that every decision we make is a trade-off. To be a master programmer is to understand the nature of these trade-offs, and be conscious of them in everything we write.
You've probably seen some variant of this, but I'll show you my version. In coding, you have many dimensions in which you can rate code:
- Brevity of code
- Featurefulness
- Speed of execution
- Time spent coding
- Robustness
- Flexibility
Now, remember, these dimensions are all in opposition to one another. You can spend a three days writing a routine which is really beautiful AND fast, so you've gotten two of your dimensions up, but you've spent THREE DAYS, so the "time spent coding" dimension is WAY down.
So, when is this worth it? How do we make these decisions?
The answer turns out to be very sane, very simple, and also the one nobody, ever, listens to:
"START WITH BREVITY. Increase the other dimensions AS REQUIRED BY TESTING."
--
In Delicious Library 2 we have a feature where we will automatically find the libraries of your friends if they have published them. Of course, with any matching system, the trick is, how do you know this is really *my* friend named "Mike Lee", and not one of the other 17 million "Mike Lee"s around the world (that whore).
So, I came up with a basic algorithm for pulling all Delicious Libraries by the owner's name first, then disambiguating within those afterwards. One of my programmers said, "Well, what happens if we get 1,000 hits for John Smith?"
And my reply, not at all tongue-in-cheek, was, "Well, then we would be very, very rich." Seriously, if 1,000 John Smiths had registered our program, think of how many customers we'd have total: millions. Multiply that by $40, and one possible response to the problem of "too many John Smiths" would be: "Who cares, let's all move to Tahiti and spend the rest of our lives on the beach sipping rum."
I kid! Mostly. My point is, we'll have PLENTY of warning and PLENTY of resources when 1,000 John Smiths start to plague us. Note that both of these are necessary -- if I were, say, deploying just some website, and I didn't make money based on the number of people using the site, I'd be a lot more worried about it blowing up before I was ready -- there would be no guarantee that I'd have the time or resources needed to handle the problem.
In this particular case, there is a slightly slower (execution time) way to do the search that would eliminate the 1,000 John Smith problem, which I will do the day it starts to become a problem. Then I'll push a free update, and my customers will never know that it could have been an issue.
But note that, even though I *know* how to solve this problem (and increase the flexibility dimension), I'm not going to solve it now. Why? Because (a) this would kill my code brevity, (b) it would make the program run slower for everyone in the meantime, (c) it would introduce more instability, and (d) it would take a bunch of time to program and debug, so I couldn't do other, cool features.
This is really key: there's a solution out there that I know is more flexible -- many people would instantly consider this the "best" solution, and consider everything else a hack. My point, which I'll say again and again, is that there are MANY dimensions with which to evaluate any solution to a problem, and flexibility is NOT paramount.
--
When most people learn objective languages, the first thing they do is go ape. I mean, they create superclasses that have one method, which is stubbed out, and twenty children classes, each of which varies by one line of code. They fall so in love with objects that they think everything needs to be its OWN TYPE of object.
Often this is done in the name of flexibility. "Look, I have this abstract superclass which currently does the drawing for all my buttons, but you could subclass it to, say, draw 3D text!"
There is a related ailment, which is the "complete class" syndrome. Many programmers, when they create a new class, add a ton of features to that class to make it "complete" -- that is, they try to anticipate everyone who may ever use this class, and they add methods that those hypothetical users may want.
Let's say, for instance, Apple didn't have an NSArray class. So, you write your own. Great! I support you. Now, in your program you need to add objects to the end of the array and remove them from the end of the array. Ok, write those methods. But, wait, you say. Maybe I should add some more methods? Get an object from any index? Insert at the beginning? Why don't I make this more flexible, you say? NO! NO NO NO!
Now, you may be saying to yourself, "What's wrong with flexibility?" Strangely, I was about to tell you. The problem is YOU ARE NOT A LIBRARY PROGRAMMER. YOU WRITE APPLICATIONS. (Note to Ali Ozer: IGNORE THIS SECTION.)
If you find yourself writing a class for your "library," then:
(a) You're not writing your application, which is where you make your money,
(b) You're writing something that you're hoping Apple will someday replace, which is a sucker's game,
(c) You're writing code you are going to have to test SEPARATELY from your app, because BY DEFINITION you've added functionality you didn't need,
(d) You're never going to really know which methods in your library work and which ones don't (eg, which ones are used in shipping programs) because you don't have user base that a company like Apple does (and witness how buggy even their under-used frameworks are),
(e) You're writing code that is going to need documenting (or some other way to comprehend it), so you're requiring yourself and everyone at your company to understand not JUST all of Apple's APIs (which are, at least, SOMETIMES documented) but also yours, and, possibly worst of all,
(f) You are attempting to predict how your application's needs will change in the future, and spending time NOW on your guess, instead of shipping the damn application, getting feedback, and THEN making changes.
Let's look more closely at (f). It's the same old thing again, isn't it? "Don't optimize your code until after you time it" becomes "Don't make your code more flexible until after you have a plan for what your app."
--
Here are some concrete rules I enforce at Delicious Monster, now:
- We don't add code to a class unless we actually are calling that code.
- We don't make a superclass of class 'a' until AFTER we write another class 'b' that shares code with 'a' AND WORKS. Eg, first you copy your code over, and get it working, THEN you look at what's common between 'a' and 'b', and THEN you can make an abstract superclass 'c' for both of them.
- We don't make a class flexible enough to be used multiple places in the program until AFTER we have another place we need to use it.
- We don't move a class into our company-wide "Shared" repository unless it's actually used by two programs.
--
So, next time your boss tells you to "be more flexible," tell him Wil Shipley says you shouldn't. He'll probably give you a raise!
Labels: code
Wednesday, December 20
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:
|
- (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.
|
+ (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:
|
[...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.
|
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:
|
- (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:
|
-(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.
|
| 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:
|
[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?"
|
-(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
Thursday, October 5
Carbon, briefly defined, is a compatibility library that ships with Mac OS X that enables older applications, written for Mac OS 9 and before, to run under Mac OS X with minimal changes (and a recompile). Carbon is descended from the original Mac Toolbox written in the early 1980s, and still shows signs of its Pascal and machine-language origins, even though it is now primarily accessed through and written in the C language.
Cocoa, briefly defined, is a new application environment that Apple got (and improved upon) when it bought NeXT in 1997. Cocoa uses a dynamic, object-oriented language (Objective-C) by design, and was the inspiration for modern languages like Java.
For a few years after the initial release of Mac OS X (10.0), there was speculation about whether Carbon or Cocoa would end up dominant. Many Mac programming groups inside and outside of Apple didn't know Cocoa and assumed it would be another flavor-of-the-month that would be quickly abandoned as unworkable, like Pink and Taligent and Copland.
Apple's official stance was initially that developers were encouraged to use either Carbon or Cocoa to write new applications; in the last year or so that message has quietly changed to just encouraging Cocoa, both because Cocoa is being enhanced faster than Carbon and because increasing numbers of Apple engineers are trying Cocoa and finding it an easier and faster to code environment within which to code.
But there are still people out there who use the Carbon APIs to program on the Mac, and there are still those who assume
Carbon is "faster" and Cocoa is just "easier". While this has no basis in any timing tests I've seen, it's a
myth that persists.
It should be noted that over the past eight years Apple has merged the Carbon and Cocoa runtime environments to the point where one can call Carbon routines from inside any Cocoa program, and vice-versa, so the idea that one environment is inherently "faster" is kind of crazy, although one can obviously fail to optimize one's code, and there is a partly-relevant question which goes: if you write the same number of lines of code in each language to do the same job, which would run faster?
The answer, in my experience, is Cocoa wins hands-down, because there's no such thing as a Carbon program that's the same number of lines as a Cocoa program that does the same thing. Carbon is wordy, in part because it is based on C, and in part because it still uses metaphors and programming conventions that were in vogue twenty to thirty years ago.
--
What's distressing to Cocoa programmers is that there are still critically important Apple APIs that are only available through the incredibly byzantine and ill-documented Carbon libraries, and some groups at Apple are
still generating Carbon code, under the guise of "Core".
Now, let me be clear. I have nothing against "real" Core libraries, like CoreFoundation or CoreGraphics or CoreAnimation. These are all modern APIs that happen to be written in C, and they tend to have good to great support above them in Objective-C, either via separate APIs (like NSBezierPath in AppKit, which automatically emits the correct CoreGraphics calls) or via toll-free bridging (like NSStreams and CFStreams).
But some "Core" libraries aren't "Core" at all -- they are still using the conventions and structures of Carbon, and these libraries are nigh-impossible to use. QuickTime, Keychain / Security Services, Core Audio, Launch Services, Speech and Voice recognition -- these are the ones I've recently run up against. The problems with even well-written Carbon libraries are myriad:
- There is usually one, giant, all-encompassing "setAttributes" function and one "getAttributes" function for setting and getting every value associated with a Carbon "object", which requires to you laboriously build up and then laboriously unparse huge, special parameter structures for even the simplest call. For example, see one of the newer (v6.4) QuickTime calls, for setting parameters on a FireWire video camera: VDIIDCSetFeatures(VideoDigitizerComponent ci, QTAtomContainer *container). That one call is used to set a HUGE number of parameters, which is "simple," except, oh, it takes eighty lines of code to prepare for that one call, and the parameters you can set are not actually documented in the documentation. (This is not an exaggeration, I'll show Apple's own example code later.)
- Carbon calls with monolithic parameter blocks and anonymous parameters (like the one above) are not inherently self-documenting, and the documentation for them is often wrong and/or incomplete because the documenters' job is so much harder. For example, with the given documentation there's really no way you could intuit how to build up a legal parameter block to actually call VDIIDCSetFeatures() unless you also read the technote Apple wrote on it in 2005, which is very helpful, but was written a year too late for Delicious Library and several years after QuickTime 6.4 was released.
- Parameters passed into functions are passed by reference, whether they are going to be modified or not, which is just obscure. For example, see SecKeychainItemCopyAttributesAndData(), which takes the input "info" by reference, even though it's read-only!
- Carbon uses the same type, OSTypes (eg, 'sit!'), extensively for everything: return codes, building parameter blocks (specifying what you want to do, how you want to do it), loading QuickTime components, etc., which means reading a function description doesn't actually give you enough information to know what to actually pass it. (Eg: "Pass it some combination of four-character constants. Good luck!")
- Carbon memory management is a mish-mash of new and old metaphors, so you might easily find yourself using, in addition to CoreFoundation objects that can be retained and released, "Handles" or "Atoms", which are obtuse and easy to screw up.
- Carbon often uses FSRefs (although there are now often string-path equivalents) to refer to files, which are, again, not fun or easy to build or use.
Let me note that I'm not trying to slam Apple's Carbon programmers here; if I were a programmer on some Carbon toolkit I wouldn't necessarily try to break every metaphor everyone else was using, either -- I'd add my code and clean things up a bit where I could. (Or, actually, I would, but I would suggest we just rewrite everything in Cocoa, which is what I'm getting to in this post.)
Carbon
has matured a lot in the last several years (eg, more Cocoa-like). But it's not enough. What I want to do, with this post, is encourage Apple to finally move those necessary but neglected frameworks all the way to Cocoa.
--
Ok, so I've been a bit abstract. Let's look at two function calls from two different Carbon libraries that are under active development and that you MUST use if you want their functionality -- there is not equivalent in any other framework. We'll take them apart, see why they are so hard to use, and then look at an ideal Cocoa API that would accomplish the same thing but be a zillion times easier to call and understand.
First off, let's look at how you might set the hue, saturation, sharpness, brightness, gain, iris, shutter, white balance, gamma, temperature, zoom, focus, pan, tilt, optical filter, focus point, and more on IIDC cameras.
Ok, wait, wait a minute. What did I just say? "IIDC cameras"? What the hell is an IIDC camera? And why do I care? Doesn't QuickTime isolate me from the low-level nonsense and just provide me with an opaque Sequence Grabber object which handles all the hardware bullshit and provides me with a single unified interface?
Well, the answer is, "Of cou--no. No, not really. Psych."
If you have a certain class of FireWire cameras which have a common way of setting their parameters,
Apple has provided low-level calls to set those parameters that only work with those cameras, and deprecated the other function calls. Note that, if you have an older (or newer) camera, or a USB camera, like
the internal USB iSight that ships with EVERY iMac and notebook Apple sells, these IIDC functions don't work for you, and you have to use the older functions (where available), even though Apple says they are deprecated. Wheee!
Ok, so assume you have one of these cameras, and you want to set, say, the gain on it. How would you do it? Well, normally, you'd spend months guessing at how to build up the parameter block to please VDIIDCSetFeatures(), but as of 2005, as I said,
Apple has a technote, which shows how the gain can be set, in only eighty lines of code.
Seriously. Eighty. Let's look:
|
ComponentResult ConfigureGain(SGChannel inChannel)
{
QTAtomContainer atomContainer;
QTAtom featureAtom;
VDIIDCFeatureSettings settings;
VideoDigitizerComponent vd;
ComponentDescription desc;
ComponentResult result = paramErr;
if (NULL == inChannel) goto bail;
// get the digitizer and make sure it's legit
vd = SGGetVideoDigitizerComponent(inChannel);
if (NULL == vd) goto bail;
GetComponentInfo((Component)vd, &desc, NULL, NULL, NULL);
if (vdSubtypeIIDC != desc.componentSubType) goto bail;
// *** now do the real work ***
// return the gain feature in an atom container
result = VDIIDCGetFeaturesForSpecifier(vd, vdIIDCFeatureGain, &atomContainer);
if (noErr == result) {
// find the feature atom
featureAtom = QTFindChildByIndex(atomContainer, kParentAtomIsContainer,
vdIIDCAtomTypeFeature, 1, NULL);
if (0 == featureAtom) { result = cannotFindAtomErr; goto bail; }
// find the gain settings from the feature atom and copy the data
// into our settings
result = QTCopyAtomDataToPtr(atomContainer,
QTFindChildByID(atomContainer, featureAtom,
vdIIDCAtomTypeFeatureSettings,
vdIIDCAtomIDFeatureSettings, NULL),
true, sizeof(settings), &settings, NULL);
if (noErr == result) {
/* When indicating capabilities, the flag being set indicates that the
feature can be put into the given state.
When indicating/setting state, the flag represents the current/desired
state. Note that certain combinations of flags are valid for capabilities
(i.e. vdIIDCFeatureFlagOn | vdIIDCFeatureFlagOff) but are mutually
exclusive for state.
*/
// is the setting supported?
if (settings.capabilities.flags & (vdIIDCFeatureFlagOn |
vdIIDCFeatureFlagManual |
vdIIDCFeatureFlagRawControl)) {
// set state flags
settings.state.flags = (vdIIDCFeatureFlagOn |
vdIIDCFeatureFlagManual |
vdIIDCFeatureFlagRawControl);
// set value - will either be 500 or the max value supported by
// the camera represented in a float between 0 and 1.0
settings.state.value = (1.0 / settings.capabilities.rawMaximum) *
((settings.capabilities.rawMaximum > 500) ? 500 :
settings.capabilities.rawMaximum);
// store the result back in the container
result = QTSetAtomData(atomContainer,
QTFindChildByID(atomContainer, featureAtom,
vdIIDCAtomTypeFeatureSettings,
vdIIDCAtomIDFeatureSettings, NULL),
sizeof(settings), &settings);
if (noErr == result) {
// set it on the device
result = VDIIDCSetFeatures(vd, atomContainer);
}
} else {
// can't do it!
result = featureUnsupported;
}
}
}
bail:
return result;
} |
Wow, easy!
Remember, this is only for a certain class of cameras (including external iSights, but excluding internal iSights) -- if you want your code to work in all cases, you also have to call
SGSetSettings() (or some such, I honestly don't know, as the focus calls I was trying to do don't have equivalents) to set up other kinds of cameras, except this call isn't as flexible, and, of course, nowhere in the documentation for the call does it say
which settings you can actually set. Because, since everything in Carbon just takes and returns OSTypes (see sin #4), it's not self-documenting what kinds of parameters you can pass in to any method -- there are tens of THOUSANDS of OSTypes defined in the system, good luck finding the list of the ones your particular function takes and returns! They certainly are never listed in the documentation of the functions that take them.
Now, if Apple had used an enumerated type instead of an OSType, as such:
|
typedef enum {
kSequenceGrabberFocus, kSequenceGrabberZoom, kSequenceGrabberGamma,
[...]
} SequenceGrabberParameters; |
Well, then we could just command-double-click on the function prototype in XCode, and we'd have our own documentation (after a fashion), in the form of the header file where the parameters are defined.
(On a side note, sadly, the AppKit and Foundation team just announced they are moving AWAY from using enumerated types in Cocoa methods and switching everything to take plain ints, because they HATE self-documenting code and hate Cocoa programmers. No, it's because they are worried that enumerated types could have unspecified sizes. But, seriously, Ali, this is totally broken -- enumerated types make reading and writing code MUCH easier, and they enforce sanity in switch() statements and if() comparisons. Please don't break this! [Update: actually, it's not quite as bad as it could be: the proposed new types aren't totally anonymous, they just are typedef'ed ints, but at least they are defined right below real enums, so command-double-click still works. Debugging and switch() and if () still aren't handled as nicely, though.]
Now, the above enumerated type would be helpful IF QuickTime had to stay with the giant, all-encompassing set-get functions. But must they? Well, no. For some reason some Carbon programmers think it's easier for them to add and remove APIs if they never commit to any particular functionality, so they use those giant anonymous set-get functions and then just have you build up a list of parameters yourself, with the idea being that it's a lot more flexible if they change the parameters out from under you than if they change the functions.
Acrazypersonsezwhat? Ok, I don't know what life was like under Mac OS 9, but nowadays we've got a well-defined system for deprecating APIs, and it works. And the advantages to not building up parameter blocks are huge. To wit, imagine the above gain-setting code, rewritten in a (hypothetical) Cocoa interface to QuickTime:
|
ComponentResult ConfigureGain(QTSequenceGrabblerChannel *sequenceGrabblerChannel)
{
return [sequenceGrabblerChannel setGain:1.0];
} |
Sure, four lines is less than eighty, but the observant amongst you will say, "Wait a minute, in the original code he checks to see if this is an IIDC camera first!" Well, yes, but, honestly, I should be able to call my mythical -setGain: on
any physical sequence grabber, and if the user's particular hardware doesn't implement gain, QT should just ignore me.
More observant folks may exclaim, "But, he sets the camera's gain feature to be: on, manual, and raw control!" And I must calmly say, if I call -setGain: on a sequence grabber, it's
implicit that I am putting it under manual control! Otherwise, what does setting the gain mean? I should
not have to write this code.
Even more observant folks may add, "Well, but, he also has some crazy code where he either sets the value to the camera's raw maximum or 500..." to which I respond: I should be abstracted away from such nonsense. Parameter values should be normalized from 0.0 to 1.0 for me; where 1.0 is the hardware maximum, whatever it is. I should never have to query the hardware.
Now, it could be I'm oversimplifying a bit. I can imagine there are complexities in this code that require, say, TWO lines of Cocoa inside my imaginary rewrite of ConfigureGain(), not just one. But you can see where I'm going with this.
The fact that Apple had to write EIGHTY lines of example code to show how to set A SINGLE PARAMETER on SOME TYPES of cameras to its maximum value shows that THESE APIS ARE SERIOUSLY BROKEN. If you ever find yourself saying, "Look, to use these APIs I've written, you just have to write four tons of this glue code which never changes..." just stop! Write the damn glue code yourself, and provide a higher-level API!
--
You might hope that QuickTime for Leopard, which has been publicly announced to be at least partly rewritten with Cocoa APIs, will solve these problems.
I certainly hope so. But, historically, some Carbon teams, when faced with writing Cocoa code, take a simple and not-very-helpful route -- they provide only one or two methods in their Cocoa classes, when their Carbon APIs are incredibly rich and feature-ful. The first iteration of QuickTime in Cocoa was like this -- look at
NSMovie, which has TWO whole methods, "-URL" and "-QTMovie".
Wow! I'm overwhelmed! I mean, it's pretty clear how I would add a track, or record from a camera to a movie, or save to disk, or... Oh, wait, no, I can't do any of those things from Cocoa. I've essentially been given a busy-box. "Here, you're a Cocoa programmer, you can't handle actual functionality... just put your little movie in your little view in your little NIB and be happy."
But, wait, my scorn is too quick. Look what we got in 10.4, to replace NSMovie:
QTMovie, which seems pretty darn rich from where I'm standing. At least, it has more than two methods, so I think that's a big improvement.
Or let's take
NSSpeechRecognizer, written by a new friend of mine from WWDC, whom I'm sorry to single out this way.
Now, NSSpeechRecognizer has a nice interface. It's clear how to use a NSSpeechRecognizer, and it's easy as heck to integrate into your applications. I put speech recognition in Delicious Library 1 in about a day using this class, and most of the work was in building up a sane list of words to recognize and keeping it up-to-date without too much overhead.
But if you only speak Cocoa, you'll think that all Apple's speech recognition can do is what NSSpeechRecognizer does: recognize a flat list of words and phrases. You'd assume you can't recognize a phrase from one list followed immediately by a phrase from another list, much less set up a tree of expected phrases. And certainly there's
no way to turn someone's speech into a list of phonemes for further processing, or to record and analyze the actual tones the user uttered. Heck, there are only four attribute set/get methods in NSSpeechRecognizer, so that must be about it for Apple's code, huh?
Well, as you've guessed by now, no. I'm
told, by the original programmers, that all the above functionality is in the Carbon APIs. However, I have no idea, because I simply don't have the patience to learn or program raw Carbon unless I'm forced to, and in this case I wasn't.
There are a ton of neat applications I could make if NSSpeechRecognizer (and its friend, NSSpeechSynthesizer) were fully Cocoa. It's too bad.
--
Ok, let's pick on another Carbon function call, and then invent another Cocoa API that should exist but doesn't.
This time, it's from Keychain. Now, Keychain is massively cool; all your passwords, from all your programs, can be stored securely in a central (encrypted) location, so you don't have to remember them -- all you have to do is remember the password to your keychain. You get to set the policy on which programs can access which passwords and how. Keychain is full-featured and awesome...
...aaaaaaaaaaaaand it's Carbon. So, it brings all that baggage with it, which is really sad.
Let's look at some code I wrote recently to build up a list of FTP sites that are currently stored in the user's keychain, so I can automatically suggest a place to upload a user's Delicious Library in 2.0:
|
#define HANDLE_ERROR(returnCode)if (returnCode != noErr) NSLog(@"keychain error %d encountered on line %d of file %s", returnCode, __LINE__, __FILE__);
- (NSArray *)allFTPSiteDictionariesFromKeychain;
{
NSMutableArray *mutableSites = [NSMutableArray array];
[mutableSites addObjectsFromArray:[self _ftpSitesWithProtocol:kSecProtocolTypeFTPAccount]];
[mutableSites addObjectsFromArray:[self _ftpSitesWithProtocol:kSecProtocolTypeFTP]];
[mutableSites addObjectsFromArray:[self _ftpSitesWithProtocol:kSecProtocolTypeFTPS]];
[mutableSites addObjectsFromArray:[self _ftpSitesWithProtocol:kSecProtocolTypeFTPProxy]];
return mutableSites;
}
- (NSArray *)_ftpSitesWithProtocol:(SecProtocolType)protocol;
{
NSMutableArray *mutableSites = [NSMutableArray array];
SecKeychainAttribute findInternetPasswordsAttributes[] = {
{kSecProtocolItemAttr, sizeof(protocol), &protocol},
};
SecKeychainAttributeList findInternetPasswordsAttributeList = {
sizeof(findInternetPasswordsAttributes) /
sizeof(*findInternetPasswordsAttributes), findInternetPasswordsAttributes
};
SecKeychainSearchRef searchRef = NULL;
OSStatus returnCode = SecKeychainSearchCreateFromAttributes(NULL,
kSecInternetPasswordItemClass, &findInternetPasswordsAttributeList,
&searchRef);
HANDLE_ERROR(returnCode);
if (!searchRef)
return nil;
do {
SecKeychainItemRef internetPasswordKeychainItemRef = NULL;
returnCode = SecKeychainSearchCopyNext(searchRef,
&internetPasswordKeychainItemRef);
if (internetPasswordKeychainItemRef == NULL || returnCode ==
errKCItemNotFound)
break;
HANDLE_ERROR(returnCode);
#define HACK_FOR_LABEL (7)
SecItemAttr itemAttributes[] = {kSecDescriptionItemAttr, kSecTypeItemAttr,
kSecCommentItemAttr, kSecProtocolItemAttr, kSecAccountItemAttr,
kSecServerItemAttr, kSecPathItemAttr, kSecSecurityDomainItemAttr,
kSecCreatorItemAttr, kSecTypeItemAttr, HACK_FOR_LABEL}; // RADAR
3425797 - You can't get the 'kSecLabelItemAttr' using this API
(it crashes), so either we have to use the number '7' or use
SecKeychainItemCopyContent(). I do the former. See
http://lists.apple.com/archives/Apple-cdsa/2006/May/msg00037.html
SecExternalFormat externalFormats[] = {kSecFormatUnknown, kSecFormatUnknown,
kSecFormatUnknown, kSecFormatUnknown, kSecFormatUnknown, kSecFormatUnknown,
kSecFormatUnknown, kSecFormatUnknown, kSecFormatUnknown, kSecFormatUnknown,
kSecFormatUnknown};
NSAssert(sizeof(itemAttributes) / sizeof(*itemAttributes) ==
sizeof(externalFormats) / sizeof(*externalFormats),
@"arrays must have identical counts");
SecKeychainAttributeInfo info = {sizeof(itemAttributes) /
sizeof(*itemAttributes), (void *)&itemAttributes,
(void *)&externalFormats};
SecKeychainAttributeList *internetPasswordAttributeList = NULL;
returnCode = SecKeychainItemCopyAttributesAndData(internetPasswordKeychainItemRef,
&info, NULL, &internetPasswordAttributeList, NULL, NULL);
HANDLE_ERROR(returnCode);
if (internetPasswordAttributeList) {
NSMutableDictionary *siteDictionary = [NSMutableDictionary dictionary];
unsigned int attributeIndex;
for (attributeIndex = 0; attributeIndex <
internetPasswordAttributeList->count; attributeIndex++) {
OSType tag = internetPasswordAttributeList->attr[attributeIndex].tag;
if (tag == HACK_FOR_LABEL)
tag = kSecLabelItemAttr;
NSString *tagString = [NSString stringForOSType:tag];
id value;
if (tag == kSecPortItemAttr)
value = [NSNumber numberWithUnsignedInt:*(unsigned int *)
&(internetPasswordAttributeList->attr[attributeIndex].data)];
else if (tag == kSecCreatorItemAttr || tag == kSecProtocolItemAttr)
value = [NSString stringForOSType:
(OSType)internetPasswordAttributeList->attr[attributeIndex].data];
else
value = [[[NSString alloc] initWithBytes:
internetPasswordAttributeList->attr[attributeIndex].data
length:internetPasswordAttributeList->attr[attributeIndex].length
encoding:NSUTF8StringEncoding] autorelease];
[siteDictionary setObject:value forKey:tagString];
}
if (IsEmpty([siteDictionary objectForKey:[NSString
stringForOSType:kSecLabelItemAttr]]))
[siteDictionary setObject:[siteDictionary objectForKey:[NSString
stringForOSType:kSecServerItemAttr]] forKey:[NSString
stringForOSType:kSecLabelItemAttr]];
[siteDictionary setObject:(id)internetPasswordKeychainItemRef
forKey:@"keychainItem"];
[mutableSites addObject:siteDictionary];
SecKeychainItemFreeAttributesAndData(internetPasswordAttributeList,
NULL);
}
CFRelease(internetPasswordKeychainItemRef);
} while (1);
CFRelease(searchRef);
return mutableSites;
} |
Now, as I said, Keychain doesn't have the worst interface in the world. It's really pretty good for Carbon -- you build the equivalent of an NSEnumerator with SecKeychainSearchCreateFromAttributes() (albeit without the nice generality of the former class), enumerate through it with SecKeychainSearchCopyNext(), and then get the data you want from the item in SecKeychainItemCopyAttributesAndData(). But at this point in this marathon post (aren't you sorry you picked on me for not posting in so long) you're an old Carbon hand, so you can spot the sins Carbon brings with it.
But, just in case, let's go over them:
- We have to build up a bunch of structures by hand so we can build up a "search specifier" in a single parameter in SecKeychainSearchCreateFromAttributes(), which hurts our readability a ton -- we really want for each statement we write to read like a sentence, almost, and if we have to write four lines of "struct this" and "allocate that" before we get to the verb, we've got pretty awkward sentences.
- We loop over the items we find using "SecKeychainSearchCopyNext()" (nice), but we have to manage memory ourselves in Carbon; there's no autorelease (or garbage collection, heaven forfend!) so we have to free every returned value explicitly.
- The returned object is an array of structures of structures of void pointers to things with counters and stuff and things -- ok, I've totally forgotten, honestly. But, my point is, it's not just an NSDictionary full of nice objects (or a CFDictionaryRef), so I have to parse out the values by hand, INCLUDING byte-swapping on Intel machines. Whee! I love it!
- SecKeychainItemCopyAttributesAndData() has a crasher if you ask to retrieve the most common attribute you'd ever ask for from a Keychain item: the label. I should point out that this function is documented to deprecate the older method of getting data from an item, which was more obtuse but didn't crash.
Now, ok, I write crashers too. I don't blame the Apple engineers for that, and it was really nice of them to step up and provide a workaround. Because of their help, this only delayed me by a couple hours instead of a couple days. My point here is, if this whole framework were a lot more easier to use (-cough-Cocoa-cough-) then it'd get used a whole lot more, and this bug would have been found LONG ago. The problem here isn't that a bug was written, it's that an incredibly valuable framework is needlessly crippled by being in Carbon and thus not used enough to be as robust as it should be.
- SecKeychainItemCopyAttributesAndData() isn't actually documented correctly, so the only way to write code for it is to get on the web and look at the source code and read Apple's mailing lists. In particular, the docs say for "attrList": "On input, the list of attributes in this item to get..." which is, frankly, just false. A look at the source code (search for "ItemImpl::getAttributesAndData(") clearly shows "attrList" is ignored on input, which only makes sense, because it's an output structure and you've already passed in the list of attributes to get in "info", so why would you pass it in twice? Note also the docs say that "itemClass" is "A pointer to the item’s class" but don't say if you are supposed to pass it in, or if it gets returned to you? (It turns out to be the latter). And "outData" is described opaquely as "A pointer to a buffer containing the data in this item." Oh, the data, you say? Well, that's certainly specific! I'm always getting and setting data on my items.
It turns out that data in this case means the actual encrypted password. So, in summary, what the documentation for this function doesn't say, but should, is, "You build up a list of parameters you want to fetch from itemRef in info, and this method returns their values in attrList if that's not passed in NULL. The item's class (here there'd be a link to all the classes) is returned by reference in itemClass if it's not passed in NULL, and the password is decrypted from the keychain into outData if that is not passed in NULL."
Ok, we've rewritten the documentation, now let's rewrite my original method to use a hypothetical Cocoa Keychain framework. Note that this rewrite is longer than the last because the base Keychain APIs are more recent than the QuickTime ones I pimped above, so they already use more modern metaphors and are thus already somewhat efficient. But can we do better? (Hint: yes.)
|
- (NSArray *)allFTPSiteDictionariesFromKeychain;
{
NSMutableArray *mutableSites = [NSMutableArray array];
[mutableSites addObjectsFromArray:[self _ftpSitesWithProtocol:NSKeychainSecurityProtocolTypeFTPAccount]];
[mutableSites addObjectsFromArray:[self _ftpSitesWithProtocol:NSKeychainSecurityProtocolTypeFTP]];
[mutableSites addObjectsFromArray:[self _ftpSitesWithProtocol:NSKeychainSecurityProtocolTypeFTPS]];
[mutableSites addObjectsFromArray:[self _ftpSitesWithProtocol:NSKeychainSecurityProtocolTypeFTPProxy]];
return mutableSites;
}
- (NSArray *)_ftpSitesWithProtocol:(NSString *)protocol;
{
NSKeychainFetchRequest *keychainFetchRequest = [[[NSKeychainFetchRequest alloc] init] autorelease];
[keychainFetchRequest setPredicate:[NSComparisonPredicate
predicateWithLeftExpression:[NSExpression expressionForKeyPath:NSKeychainSecurityProtocol]
rightExpression:[NSExpression expressionForConstantValue:protocol]
modifier:NSDirectPredicateModifier type:NSEqualToPredicateOperatorType options:0]];
NSError *error = nil;
NSArray *matchingKeychainItems = [[NSKeychain defaultKeychain]
executeFetchRequest:keychainFetchRequest error:&error];
if (error) {
NSLog(@"%@: Error doing keychainFetchRequest '%@'", keychainFetchRequest);
return nil;
}
return matchingKeychainItems;
} |
_ftpSitesWithProtocol got so tiny because, assuming I didn't have to access the returned keychain results as raw data -- that is, if there were real, full-fledged NSKeychainItem objects that could be queried for their -protocol and -account and -password and -label, then I wouldn't need to do all the complicated object-building I did in the above example, I'd just return the actual keychain items, and even bind to them in my interface directly! This would be pretty rocking... you can imagine feeding an NSArrayController from this list and then binding columns in a tableView to "label" or what-have-you, and you've got a poor man's Keychain Access app in like five lines of code.
Now, again, this might be an oversimplification of Keychain's APIs. There may be subtleties that I'm missing that would require a slightly more complicated set of Cocoa classes. But in general, I think porting Keychain to Cocoa would decrease the amount of effort to program to it (and increase its audience) by an order of magnitude.
--
Let me wrap up, finally, by apologizing profusely to everyone at Apple and elsewhere whom I've insulted tonight. I've been a programmer for 25 years now, and every year I look back at the code I wrote the year before and think, "What the hell? What idiot-monkey wrote this crap?"
And yet, here I sit, on my medium-size pony, and insult code that some of you wrote
ten years ago, viewing it through the lens of modern coding practices that
you guys taught me. Let me stress this: the good programming practices I've learned, such that I've learned them, are from studying the very best of what Apple's done. There are tools and techniques that I love and am in awe of in Apple's code: bindings, enumerators, object-oriented database access, model-view-controller, all those buzzwords. I didn't invent 'em, you guys did.
I know that Apple people exist in the real world, where going back and rewriting blecherous code is not something any engineer wants to do, nor any manager to fund. I know if you meet with Steve in six months and say, "Hey, sure, we didn't add anything cool in Leopard for you to demo, but check it: Keychain is Cocoa now! Huh? Huh?" he'd probably put his foot so far... well, you know where I'm going with this.
And I know that it's a small miracle that things like QuickTime and Keychain and Speech Synthesis and Core Audio exist in the first place. These have truly amazing functionality, using research and math far beyond my ken: if they weren't, I honestly wouldn't give a crap that all their power is frozen in Carbonite right now. (You don't see me complaining about the, uh, Scrap Manager, do you?)
So, I write this
to help you, putative Carbon-coding-Apple-engineer-reader, not
to harm you. I want you to take this to your manager, and say, "Look, see! We've got to get to Cocoa! This has got to be a priority! We can't keep slapping tar on a boat made out of milk cartons!"
Don't do it for me, do it for your team. Do it because you'll love it, and because more programmers will use your stuff, and they'll show it off in ways you never imagined, and suddenly you'll be in all the Stevenotes, and you'll be wealthy and happy and famous.
Labels: code
Monday, July 3
Today's post is a meta-pimp -- this is the summary of some guidance I gave one of my programmers here at Delicious Monster. Most of this isn't new information; it should in fact be standard practice for experienced Cocoa hands. Consider this a remedial pimping; for young pimps and the pimp-at-heart.
Most sheets you'll pop up will be "alert sheets," of the simple "Hey, uh, you're about to do something bad and/or extremely bad, are you sure you want to continue?" variety. (The examples here are all cribbed from
Apple's excellent user interface guidelines, which you should read and re-read.)
Alert sheets are simple to program, although not as simply as alert
panels used to be under Rhapsody (pre-10.0), because panels are application-modal and sheets are document-modal. The relevant difference for users between the two modalities is that with document-modality the user is
still allowed to mess around with other documents, whereas in app-modal mode you put your panel in front of everything and force the user to vote yea or nay before she does anything else, period.
Contrast this with Mac OS 9's largely
system-modal panels, where, for example, if you were printing in one application the print panel would block the user from interacting with any other applications until the print was complete. This, frankly, sucked from the user. Using induction we can deduce that finer-grained modality is preferable, and thus that we should use document-modality over app-modality, even though app-modality still exists in the Cocoa APIs. And, for example, iTunes uses application-modal panels on their info panels instead of, say, document-modal sheets, which is really pretty bletcherous. (Really, they shouldn't use modes at all in an info panel, but that's a separate issue.)
So, before 10.0, you would find yourself asking the user's permission (or forgiveness) using one of the following two panel functions. You can still use them if you come up with a really good excuse to block the entire application -- for example, if the user selects a menu option like, "Delete this application and add my name to a list of people who are never allowed to run it again":
int NSRunInformationalAlertPanel(NSString *title, NSString *msg, NSString *defaultButton, NSString *alternateButton, NSString *otherButton, ...);int NSRunCriticalAlertPanel(NSString *title, NSString *msg, NSString *defaultButton, NSString *alternateButton, NSString *otherButton, ...);Note that there are two nice variants provided here, both stolen from Mac OS 9 by NeXT engineers a long, long time ago -- the "informational" panel and the "critical" panel. For your reference, here's a friendly quote from the currently-current Apple HIG guidelines explaining when to use which: "In rare cases, you may want to display a caution icon in your alert, badged with the application icon as shown in Figure 13-30. A badged alert is appropriate only if the user is performing a task, such as installing software, and a possible side effect of that task would be the inadvertent destruction of data."
And here's an example of a critical alert panel:
Notice that there's a big yellow exclamation point, which is the international symbol for "you are going to lose data." (Not really, I just made that up.) As a programmer, you might not have realized there are two variants of the standard alert panel, because many Cocoa programmers don't use critical alerts when needed (instead they use the generic
NSRunAlertPanel() function, which is admittedly 50% easier to remember). For instance, in TextEdit, when you change a rich-text document to plain-text, and are going to irretrievably lose all your formatting information, you get an
informational sheet, not a modal sheet. Whups.
Update: Mea culpa! I just checked and, in 10.4 at least, TextEdit can undo past a RTF conversion event, and so they were correct to NOT make their sheet be critical. I apologize to the authors of that app for impugning their character in my search for an example.
Invoking either these two panels from your programs is as simple as cake: you just call 'em and check the return code to see which button was pressed. Remember to follow the HIGs when it comes to the "title" and "msg," because a lot of people get this wrong. Specifically, the "title" isn't really so much a
title any more under Aqua, it's more of a
line of text describing the issue succinctly. This is a change from pre-Aqua days that some Cocoa programmers haven't fully grokked, so you'll still see panels with the title "Alert" or with just a title and no text, which
the HIG guys hate.
--
So, after that brief discussion of application-modal panels, we move to document-modal sheets, which you should strive to use whenever it makes sense. First off, let's check out the easy-to-functions for these:
NSBeginCriticalAlertSheet(NSString *title, NSString *defaultButton, NSString *alternateButton, NSString *otherButton, NSWindow *docWindow, id modalDelegate, SEL didEndSelector, SEL didDismissSelector, void *contextInfo, NSString *msg, ...)NSBeginInformationalAlertSheet(NSString *title, NSString *defaultButton, NSString *alternateButton, NSString *otherButton, NSWindow *docWindow, id modalDelegate, SEL didEndSelector, SEL didDismissSelector, void *contextInfo, NSString *msg, ...);Hey, wait a minute... these functions have more parameters! I thought you said sheets were
easier to use? Well yes, they are, for the user. For you: more work. However, the payoff is happy users. And you can't put a price on that. Well, you can, actually: it's about $40, which is pretty sweet.
The big difference in running sheets vs. running panels is that
these sheet functions return immediately, before the user has responded to the panel. Note that they don't return an
int, they return
void: the great unknown. (If only we had Schrödinger's return type, which would represent the all and none of default and alternate and other.)
Now, this immediate return throws a lot of programmers of, and rightly so... I mean, obviously you are throwing up an alert because you want some feedback from the user, and so the _very_ next thing you want to do is going to be based on what the user decides, and yet here you are, unceremoniously dumped back into your code with no decisions in sight. What to do?
Well, there are two choices. One is, just immediately return from whatever method you're in, which should return control flow to the main even loop, and then wait for your
didEndSelector to eventually be called when the user makes up her mind. In that method, you'll be informed of her decision, and can proceed naturally.
The only downside to this is that if you've built up a lot of state information before popping up your alert sheet, you're going to have to regenerate it when you get called back
or you'll have to save it off in your instance variables. Bluck!
One way around this, to be used in extreme cases, is to just process events on your own, such as the following:
|
@implementation ExampleClass
- (IBAction)doSomethingCompletelyAmazingYetKindOfDangerous:(NSButton *)sender;
{
[some code that has a lot of state setup]
userChoice = INT32_MAX;
NSBeginInformationalAlertSheet(NSLocalizedString(@"I'm Going to do Something Big", @"alert title"),
nil, nil, nil, [document windowForSheet], self, selector(_sheetDidEnd:returnCode:contextInfo:),
nil, NULL, NSLocalizedString(@"It's going to be awesome. Seriously.", @"alert message"));
while (userChoice == INT32_MAX) {
[NSApp sendEvent:[NSApp nextEventMatchingMask:NSAnyEventMask
untilDate:[NSDate distantFuture] NSModalPanelRunLoopMode dequeue:YES]];
}
if (userChoice == NSAlertDefaultReturn) {
[some code that does the cool thing with the state we calculated at great expense]
}
}
@end
@implementation ExampleClass (Private)
- (void)_sheetDidEnd:(NSWindow *)sheet returnCode:(int)returnCode contextInfo:(void *)contextInfo
{
userChoice = returnCode;
}
@end |
In this example we assume
userChoice is an instance variable (quick quiz: why can't this be a static variable instead?) and is an int. I'm not particularly fond of having a one-use ivar like this, but, hey, not a lot of options - if the alternative is saving off four or five different pieces of state in ivars, this might be cleaner. Note that MOST of the time you're really going to want to do your actual processing in the
-_sheetDidEnd:..., so you can skip all the NSEvent crap and just return from your
-doSomething... method -- this is just an example of how you might work around a situation where you really don't want to return from your action method before you finish what you're doing.
Update: Pierre Lebeaupin (the pretty pin?) correctly pointed out that my contrived example above is, in fact, wrong in the case of document-modal sheets (see the comments page). I tried to be weaselly in this example by saying, "I'd only use this in extreme cases," to express that I've never used [[sendEvent:...]nextEvent:...] hack in this way, but it'd be something I might try (and then, hopefully, discovered the bug Pierre found).
What I was
trying to accomplish was come up with a concise way to introduce sub-event loops, which are actually a useful tool in some extreme situations. In fact, this post was inspired by one of my programmers not knowing about them when trying to work around some problems with printing, so I tried to blend my discussion of sheets and event subloops into one mélange, but, in fact, my example was flawed. Pierre pointed out a fundamental problem that, in fact, early Mac OS X apps all suffered from, because early on Apple hadn't thought of the case where you pop up two sheets in two different documents. Apple added the asynchronous APIs just for this case.
Here's a real example of when you might use your own event subloop, which
may in fact still have problems of its own (we just started doing this in our code), but I assert it does not due to the inherent limitations of the print system.
Right now, the way we print in Delicious Library 2 is to create a WebView (from Apple's WebKit) and populate it with your books &c, then just tell the WebView to print itself. Clever, no? The only problem is that WebViews will NOT load images synchronously (grrrrrr) even if they are local images and even though it would be completely trivial to do so (eg, NSURLConnection has a +sendSynchronousRequest:... method). So, if you're in the middle of running a print sheet for a document, and you create a WebView based on user input, and then want to print it, you need to spin on the event loop letting all the images load asynchronously before you try to print the WebView. Since there appears to only be one possible print operation per application, I believe this to be a valid use of a event subloop. In our case, the code looked like this:
|
- (void)_setupWebViewThumbnailBasedOnUserSettings;
{
allImagesHaveLoaded = NO;
while (!allImagesHaveLoaded) {
NSEvent *nextEvent = [NSApp nextEventMatchingMask:NSAnyEventMask
untilDate:[NSDate dateWithTimeIntervalSinceNow:0.05] inMode:NSModalPanelRunLoopMode dequeue:YES]
if (nextEvent)
[NSApp sendEvent:nextEvent];
}
// elsewhere, we have code that sets 'allImagesHaveLoaded' when... well, you know
} |
Note that we are, in fact, polling in the above loop, which is normally a no-no, but we are forced to because we're not guaranteed an event will be sent
after the last image loads in our WebView, we don't want the user to have to 'bump the mouse' to get the print panel to finish. (This kind of mistake is very common when writing event loops, and in fact you can see it in a lot of shipping apps -- for instance, some older apps force you to keep moving the mouse to keep autoscrolling; if you stop moving, you stop scrolling, because events aren't being processed and they didn't set a separate timer event like the Apple docs said.)
In this case, we don't mind the polling so much because we know we'll stop once we've loaded a very small number of images from a local disk, and because there's really not much else the user is planning to do in this quarter-of-a-second (switch applications and start and a compile? not likely.) Since the end of the polling is NOT determined by user input, we aren't in a situation where the user could wander away from her computer while it is polling while waiting for her and come back to find her MacBook slagged from overheating.
--
O'Reilly has written a whole article about running alert sheets, which you can read for a lot more detail than I would ever provide you with, ever. Finally, you should check out the (relatively) new
NSAlert class, which is slightly more complex to use (you have to create the object and THEN show it) but provides an objective API onto all this.
--
Ok, now we've handled the case of pulling down a simple yes/no/maybe style sheet with two lines of text and an icon. What if you want to pull down a sheet with radio buttons and tables and all kinds of fun widgets, that performs a relatively complex task for the user (like, say, configuring rules for a smart shelf)? Here's an example sheet from the AppKit, of setting up a fax. (Note that if you want to do
exactly what's on this sheet, you should, you know, just send a fax.)
Well, here's what I consider to be the current best practice for sheets that have a ton of logic and interaction. The overview is, you create an NSWindowController subclass to handle loading and unloading the NIB with the sheet in it (remember that a sheet is just a standard window to Interface Builder), and then provide a single, beautiful class method as its API:
|
@interface LIShelfCreationSheetWindowController : NSWindowController
{
NSDocument *document;
// [add some ivars and outlets and stuff]
}
// Class API
+ (void)runModalSheetlToCreateShelfForDocument:(NSDocument *)document;
// actions
- (IBAction)addShelf:(id)sender;
- (IBAction)cancel:(id)sender;
@end
[...new file...]
@implementation LIShelfCreationSheetWindowController
// Class API
+ (void)runModalSheetlToCreateShelfForDocument:(NSDocument *)document;
{
LIShelfCreationSheetWindowController *shelfCreationSheetWindowController =
[[self alloc] _initWithDocument:document];
[NSApp beginSheet:[shelfCreationSheetWindowController window]
modalForWindow:[document windowForSheet] modalDelegate:shelfCreationSheetWindowController
didEndSelector:@selector(_sheetDidEnd:returnCode:contextInfo:) contextInfo:nil];
[[shelfCreationSheetWindowController window] makeKeyAndOrderFront:nil]; // redundant but cleaner
}
// actions
- (IBAction)addShelf:(id)sender;
{
//
// actually create the shelf, based on the user's settings on the sheet, which are bound to our ivars
//
[NSApp endSheet:[self window] returnCode:0]; // stop the app's modality
}
- (IBAction)cancel:(id)sender;
{
[NSApp endSheet:[self window] returnCode:0]; // stop the app's modality
}
@end
@implementation LIShelfCreationSheetWindowController (Private)
// Init and dealloc
- (id)_initWithDocument:(NSDocument *)aDocument;
{
if (![self initWithWindowNibName:NSStringFromClass([self class])])
return nil;
document = [aDocument retain];
return self;
}
- (void)dealloc;
{
[document release];
[super dealloc];
}
// private callbacks
- (void)_sheetDidEnd:(NSWindow *)sheetWindow returnCode:(int)returnCode
contextInfo:(void *)contextInfo; // [NSApp beginSheet:...]
{
[sheetWindow orderOut:nil]; // hide sheet
[self autorelease];
}
@end |
Things to note here: this class has a backpointer to your NSDocument, so it can get at any relevant state it needs without you having to passed it in explicitly as parameters to the class method (in real code you'd replace the word "NSDocument" with the name of your NSDocument subclass). For instance, we immediately need to figure out which window will sport our shiny new sheet, and we use NSDocument's built-in and conveniently titled
-windowForSheet method.
In NIB you'd hook up buttons to invoke the
-addShelf: and
-cancel: actions, and you'd using bindings to keep your instance variables in sync with the tables and radio buttons and other widgets in the UI, so you can just access your ivars directly in
-addShelf: when the rubber hits the code.
I prefer having a separate class to run the sheet because I've found there can be a lot of cruft code for configuring the UI and performing some action, and it's nice to offload your NSDocument subclass of that kind of code. In NSDocument, your original action method is going to be very small (one line!), which is going to make it very readable.
--
This shell of a class isn't intended to evoke a "Genius!" reaction from the gentle reader: hopefully, in fact, you are muttering, "That seems pretty obvious." Because the correct way to write code should always seem obvious, in retrospect. Years from now, when you're looking at your code, you should say, "Of course I wrote it that way -- there's no other decent way to do it," and not, "What the hell was I thinking?"
Labels: code
Saturday, April 15
While I'm
almost always a cheerleader for Cocoa (and Apple in general) in public, there are, of course, boneheaded things in Cocoa that grind at me every time I run up against them. Some annoyances exist for historical reasons (eg, Why does NSImage have twenty different ways to draw itself, each of which has slightly different semantics?), and some are apparently boundary problems between engineering groups (eg, Why doesn't NSImage have a way to create itself from a CGImageRef? Why are NSRect and CGRect exactly the same yet different? Why can't I ask an NSCachedImageRep for its CGContextRef?).
And, of course, some things are just plain old slap-your-head-duh mistakes...
Bindings, for instance, are my second-favorite feature in all of Cocoa (after CoreData). They're great. I love 'em. So much so that when any little part of them is less-than-perfect, it's incredibly grating.
NSTreeController is a new class designed to drive an NSOutlineView using bindings, which parallels NSArrayController's ability to drive an NSTableView. (Not surprisingly, they are both subclasses of NSController.) However, NSTreeController's design has a number of problems, most of which stem from this very questionable design decision (from Apple docs, emphasis mine):
- (id)arrangedObjects; // opaque root node representation of all displayed objects. This is just for binding to or passing around. At this time, developers should not make any assumption about what methods this object responds to.Let me clarify this: if you hook an NSTreeController up to your object model, and you populate your tree with instances of class, say, "Shelf" (just picking a COMPLETELY random example), then the
-arrangedObjects method will return... any guesses? Maybe, instances of class "Shelf"? Wrong! In fact, it returns objects of type "nothing-you-can-understand."
*Cough*Apoordesignsayswhat*cough*?
What the heck is going on here? Apparently someone at Apple decided it'd be easier to create strange little shadow objects that point to your REAL objects when you put real objects into an NSTreeController, but because that's such an ugly thing to do they decided not to document these little shadow objects, so you can't actually DO anything with them.
This is kind of like not signaling when making an illegal left turn, with the notion that if you don't signal it's not, you know, wrong.
Here are a bunch of problems that fell out from the the opaque-shadow-object design of NSTreeController:
• First, there is no
-[NSTreeController setSelectedObjects:] method, because, of course, the objects YOU have access to are not the same ones that the ones that NSTreeController is messing with. So, uh, good luck setting the selection in code!
Oh, wait, there's
-setSelectionIndexPaths:, which takes an 'NSIndexPath'... except there are two problems with this method, which unfortunately make it almost entirely useless. One is, NSIndexPath is a pretty obtuse class and it's always immutable, meaning if you want to build up a path to a particular object you're going to be writing a ton of code. The other is, if you change the NSTreeController's 'sortDescriptors' or its 'contents', any NSIndexPath you've socked away is invalidated, so you can't, say, store a selection and then re-create that selection later with any certainty.
Nor can you really select a particular object, because, uh, how do you know the path to your object when NSTreeController's opaque arrangedObjects can be in a different order than your source objects? Ok, sure, you can read off the sort order and replicate the sort on your objects, and build up a NSIndexPath to an object by also reading off the method NSTreeController is going to use to discover the children of your objects, as well... does this sound like a pain to you? Well, it is.
• Next, it's almost impossible to figure out how to get an NSTreeController to 'select none' or 'select all'. These should ideally take one line of code, and do with NSArrayController.
But when trying to select nothing on a NSTreeController, one cannot easily construct an NSIndexPath that is the empty set; all NSIndexPath creation methods are similar to
+indexPathWithIndex:, which obviously creates a path that already has one entry (eg, is not empty), so you'd have to do something like
[[NSIndexPath indexPathWithIndex:0] indexPathByRemovingLastIndex], which is fugly beyond all belief. If you call
[NSIndexPath indexPathWithIndexes:NULL length:0] it crashes. (Possibly
[[[NSIndexPath alloc] init] autorelease] works, I have to admit I didn't think of it until just now and it's very late and I've pretty much had it with screwing around with NSIndexPath.)
On the other hand, when trying to select all, you have to manually create an array containing a butt-ton of NSIndexPaths to cover every node in your tree! Ugh! And, since NSTreeController returns an OPAQUE objects, you again have to re-create the sorting and children-getting that NSTreeController is going to use when you're creating your array of indexPaths, which honestly takes a lot of the fun out of having an object in NIB.
• Similarly, almost all NSOutlineView delegate methods are almost useless, which means there's really no easy way to implement drag and drop. Consider the following NSOutlineView callbacks:
- (NSDragOperation)outlineView:(NSOutlineView*)outlineView
validateDrop:(id )info proposedItem:(id)item proposedChildIndex:(int)index;
- (NSDragOperation)outlineView:(NSOutlineView*)outlineView
validateDrop:(id )info proposedItem:(id)item proposedChildIndex:(int)index;
- (BOOL)outlineView:(NSOutlineView*)outlineView acceptDrop:(id )info
item:(id)item childIndex:(int)index;Now consider that the 'item' parameters above are of an opaque class! Wait, what EXACTLY are you proposing? You'd like to drop this pasteboard onto an item in my NSOutlineView whose class is undocumented, so I have no idea where this drop is happening? Uh, let's see... Yes? No? Maybe? 3? Kentucky?
Similarly, it's hard to implement this NSOutlineView delegate method:
- (BOOL)outlineView:(NSOutlineView *)outlineView
shouldEditTableColumn:(NSTableColumn *)tableColumn item:(id)item;When, again, you can't actually look at the item that might be edited. I mean, if you don't actually CARE which PARTICULAR item is going to be edited, then you'd assumedly set the whole damn NSOutlineView to be editable or not, yes? But if you've gone to the trouble to implement this method, I'm guessing you are actually going to want to, you know, QUERY the 'item' a bit, ask about its family, see if it's been tested... make sure that it's really the kind of thing you want to be editing.
--
Ok, phew. I'm sure some engineer at Apple hates me now. (Sorry, overworked engineer dude. You can make fun of my code if you'd like.)
And, in fact, we have a nickname for people at Delicious who complain about stuff and don't
do anything to make the situation better. So, for the record, I did, in fact, file a very detailed bug report with Apple on this in RADAR, so I'm not just complaining to the crowd - I honestly can't stand it when people report bugs in my stuff to the net instead of me, and I wouldn't do that to Apple.
But that's not enough, in my mind. I shouldn't just complain, I should fix this! Why don't I post some code that works around all these ills, so everyone can use NSTreeControllers the way they were intended, until Apple fixes the API in Liger (right, Apple?). Hey, good idea, me. I like the cut of your jib.
I have two categories here, one for NSOutlineView and one for NSTreeController. They contain different methods but some similar code, and I admit that I probably could merge their implementations a bit if I tried, but I also admit that there's only so much cleaning I'm going to do on code I've written to work around API limitations that I'm SURE will go away in the next release. But, please, if you do come up with better versions of these, post 'em in the comments or send 'em to me. They are appreciated!
First off, here's a category of 'NSTreeController' that adds a
-setSelectedObjects: method which takes your REAL objects - not the phony shadow ones - from anywhere in your content tree, and also adds an
-indexPathToObject:: method which will tell you where one of your REAL objects is in the NSTreeController given the controller's current sorting and children-getting-method and all that, so you don't have to get your hands all sticky with NSIndexPath gunk.
|
// NSTreeController-DMExtensions.h
// Library
//
// Created by William Shipley on 3/10/06.
// Copyright 2006 Delicious Monster Software, LLC. Some rights reserved,
// see Creative Commons license on wilshipley.com
#import <Cocoa/Cocoa.h>
@interface NSTreeController (DMExtensions)
- (void)setSelectedObjects:(NSArray *)newSelectedObjects;
- (NSIndexPath *)indexPathToObject:(id)object;
@end
// NSTreeController-DMExtensions.m
// Library
//
// Created by William Shipley on 3/10/06.
// Copyright 2006 Delicious Monster Software, LLC. Some rights reserved,
// see Creative Commons license on wilshipley.com
#import "NSTreeController-DMExtensions.h"
@interface NSTreeController (DMExtensions_Private)
- (NSIndexPath *)_indexPathFromIndexPath:(NSIndexPath *)baseIndexPath inChildren:(NSArray *)children
childCount:(unsigned int)childCount toObject:(id)object;
@end
@implementation NSTreeController (DMExtensions)
- (void)setSelectedObjects:(NSArray *)newSelectedObjects;
{
NSMutableArray *indexPaths = [NSMutableArray array];
unsigned int selectedObjectIndex;
for (selectedObjectIndex = 0; selectedObjectIndex < [newSelectedObjects count];
selectedObjectIndex++) {
id selectedObject = [newSelectedObjects objectAtIndex:selectedObjectIndex];
NSIndexPath *indexPath = [self indexPathToObject:selectedObject];
if (indexPath)
[indexPaths addObject:indexPath];
}
[self setSelectionIndexPaths:indexPaths];
}
- (NSIndexPath *)indexPathToObject:(id)object;
{
NSArray *children = [self content];
return [self _indexPathFromIndexPath:nil inChildren:children childCount:[children count]
toObject:object];
}
@end
@implementation NSTreeController (DMExtensions_Private)
- (NSIndexPath *)_indexPathFromIndexPath:(NSIndexPath *)baseIndexPath inChildren:(NSArray *)children
childCount:(unsigned int)childCount toObject:(id)object;
{
unsigned int childIndex;
for (childIndex = 0; childIndex < childCount; childIndex++) {
id childObject = [children objectAtIndex:childIndex];
NSArray *childsChildren = nil;
unsigned int childsChildrenCount = 0;
NSString *leafKeyPath = [self leafKeyPath];
if (!leafKeyPath || [[childObject valueForKey:leafKeyPath] boolValue] == NO) {
NSString *countKeyPath = [self countKeyPath];
if (countKeyPath)
childsChildrenCount = [[childObject valueForKey:leafKeyPath] unsignedIntValue];
if (!countKeyPath || childsChildrenCount != 0) {
NSString *childrenKeyPath = [self childrenKeyPath];
childsChildren = [childObject valueForKey:childrenKeyPath];
if (!countKeyPath)
childsChildrenCount = [childsChildren count];
}
}
BOOL objectFound = [object isEqual:childObject];
if (!objectFound && childsChildrenCount == 0)
continue;
NSIndexPath *indexPath = (baseIndexPath == nil) ? [NSIndexPath indexPathWithIndex:childIndex]
: [baseIndexPath indexPathByAddingIndex:childIndex];
if (objectFound)
return indexPath;
NSIndexPath *childIndexPath = [self _indexPathFromIndexPath:indexPath inChildren:childsChildren
childCount:childsChildrenCount toObject:object];
if (childIndexPath)
return childIndexPath;
}
return nil;
}
@end |
Believe me,
-setSelectedObjects: is handy as heck. Sewiously.
Next up, an NSOutlineView category that adds one crucial method:
-realItemForOpaqueItem:, so when you're given an item from bizarro-world in an NSOutlineView delegate or datasource callback, you can just turn it into one of your objects, and speak to it in
your language. You can thank me later! (Or, hell, thank me now; honestly, I'm not super-particular about the whole 'thanking' thing.)
|
// NSOutlineView-DMExtensions.h
// Library
//
// Created by William Shipley on 3/10/06.
// Copyright 2006 Delicious Monster Software, LLC. Some rights reserved,
// see Creative Commons license on wilshipley.com
#import <Cocoa/Cocoa.h>
@interface NSOutlineView (DMExtensions)
- (void)setSelectedObjects:(NSArray *)newSelectedObjects;
- (NSIndexPath *)indexPathToObject:(id)object;
@end
// NSOutlineView-DMExtensions.m
// Library
//
// Created by William Shipley on 3/10/06.
// Copyright 2006 Delicious Monster Software, LLC. Some rights reserved,
// see Creative Commons license on wilshipley.com
#import "NSOutlineView-DMExtensions.h"
@interface NSOutlineView (DMExtensions_Private)
- (NSTreeController *)_treeController;
- (id)_realItemForOpaqueItem:(id)findOpaqueItem outlineRowIndex:(int *)outlineRowIndex
items:(NSArray *)items;
@end
@implementation NSOutlineView (DMExtensions)
- (id)realItemForOpaqueItem:(id)opaqueItem;
{
int outlineRowIndex = 0;
return [self _realItemForOpaqueItem:opaqueItem outlineRowIndex:&outlineRowIndex
items:[[self _treeController] content]];
}
@end
@implementation NSOutlineView (DMExtensions)
- (NSTreeController *)_treeController;
{
return (NSTreeController *)[[self infoForBinding:contentAttributeKey]
objectForKey:@"NSObservedObject"];
}
- (id)_realItemForOpaqueItem:(id)findOpaqueItem outlineRowIndex:(int *)outlineRowIndex
items:(NSArray *)items;
{
unsigned int itemIndex;
for (itemIndex = 0; itemIndex < [items count] && *outlineRowIndex < [self numberOfRows];
itemIndex++, (*outlineRowIndex)++) {
id realItem = [items objectAtIndex:itemIndex];
id opaqueItem = [self itemAtRow:*outlineRowIndex];
if (opaqueItem == findOpaqueItem)
return realItem;
if ([self isItemExpanded:opaqueItem]) {
realItem = [self _realItemForOpaqueItem:findOpaqueItem outlineRowIndex:outlineRowIndex
items:[realItem valueForKeyPath:[[self _treeController] childrenKeyPath]]];
if (realItem)
return realItem;
}
}
return nil;
}
@end |
Let me finish by saying that I really don't like being negative towards Apple, both because I actually don't enjoy picking on other people's work unless they ask me to do it, and because Apple engineers have done so much incredible stuff that I would
never have thought of myself, so it seems disingenuous to complain if some little bit of it is non-optimal.
On the other hand, I also don't want to come off as an indiscriminate cheerleader for anything and everything with Apple's imprimatur; that wouldn't help me or Apple or anyone who is considering following my advice. If an API is bad, I need license to say that it's bad, because otherwise it means nothing when I say that something good is good.
But on the OTHER other hand (
the gripping hand, as it were), I know that Apple's detractors tend to seize upon any scrap of criticism that one of the faithful might express towards Apple and blow it out of proportion - witness the recent spate of witless articles about the supposed "backlash" (their term) against Boot Camp, which is really about as tight a product as you can get and is free and IS A FREAKING BETA, AND IF YOU HAVE A PROBLEM WITH THE NOTION OF PUTTING WINDOWS ON YOUR MAC JUST DON'T DO IT, DON'T START A FREAKING PETITION LIKE A WHINEY LITT...
Ok, I'm calm now. At any rate, please don't quote me as saying, "I've had it with Cocoa! Apple sucks!" Cocoa and bindings and CoreData are 90% of what makes Delicious Library 2 so tiggity-tight, and I simply could not have created Library 1 or 2 without them. Period. I won't port to Windows because I can't, not because I hate people who use Windows. It's just too hard. It's not like people TRY to make bad software for Windows. It
ends up bad because if you have to spend all your time just fighting to get the most basic functionality working, of COURSE you're not going to have any time to polish.
So, much love to my homies on the bindings team. Love to
all my Cocoa brothers and sisters.
And, please, fix NSTreeController for Liger. Sewiously!
Labels: code
Saturday, March 25
Today
a fellow blogger asked me to pimp his post. Since it's only two lines, I figure I can take a break from my busy schedule of, uh, drinking and stuff, and help a brother out.
So I did the Challenge problem in Chapter 4 of Cocoa Programming for Mac OS X, Second Edition. I've come up with two different "solutions".
|
- (IBAction) reportCharacterCount: (id)sender
{
NSString *inputString = [inputField stringValue];
[outputField setStringValue:[NSString stringWithFormat:@"%@ has %d letters.",
inputString,[inputString length]]];
} |
|
- (IBAction) reportCharacterCount: (id)sender
{
NSString *inputString = [[NSString alloc] initWithString:[inputField stringValue]];
[outputField setStringValue:[NSString stringWithFormat:@"%@ has %d letters.",
inputString,[inputString length]]];
[inputString release];
} |
For pedagogical reasons, could someone tell me what the difference is between the two? And if possible, which one is better?
(As you can probably guess, my very first solution consisted of version 1 with a release, which brought me to the debugger in a hurry.)
The short answer is the difference between the two is that the second one wastes time and memory to no good effect. There are several problems with the second one: for example, if you really wanted a immutable copy of a string, you should just use
[[inputField stringValue] copy] and not
-initWithString:, because the latter
always allocates a new string, whereas the former will just return the same object with an increased retain count if the original string was already immutable. Now that's fast!
But, in fact, there's no point in doing this copy of the inputField's string, for two reasons. First off, when you call
-setStringValue: on outputField it's really the field's job to make sure it holds on to a immutable copy of the string you've passed in, so it's going to call
-copy or do something similar itself. (It's true there were bugs in early versions of NeXTstep where sometimes mutable strings would be retained or returned instead of immutable ones, but those are mostly ironed out now.)
Secondly, and more importantly, you're not actually passing this string directly to your output NSTextField, you are generating a
new, autoreleased string in your
+stringWithFormat: call, which has the inputField's stringValue as a sub-string. Now, leaving aside the actual implementation details of
+stringWithFormat:, it's a given that it will somehow keep an immutable copy of any strings you pass into it. Otherwise, honestly, nothing in this damn system would work.
Less code is better if it's functionally the same, and the second implementation is absolutely no safer in any way. Even if you were, say, messing with multiple threads at some point, so the value of inputField could change
during your action method, both implementations would be failures, so there's really no conceivable situation in which the second implementation is better.
Also, what's up with that blank line at the beginning of your methods? Seriously, that isn't helping anyone.
Finally, I should point out both implementations are really non-optimal in the post-10.3 world: what you should really do is bind the 'value' inputField to a new instance variable in your controller in Interface Builder (say,
NSString *inputString;), and then bind outputField's 'value' to your controller with the path of, say, 'outputString', then write the following:
|
+ (void)initialize;
{
[self setKeys:[NSArray arrayWithObject:@"inputString"]
triggerChangeNotificationsForDependentKey:@"outputString"];
}
- (NSString *)outputString;
{
return [NSString stringWithFormat:@"%@ has %d letters.",
inputString, [inputString length]]];
} |
Admittedly, this isn't really less code, and in fact it change the semantics of your app: eg, it doesn't require you to send the action to populate the outputField. But bindings are generally the way you should code these days; if you find yourself using target/action, or otherwise manually pushing or pulling values to or from controls, think hard about using bindings instead.
Labels: code
Saturday, March 4
In today's episode, instead of harshing on some poor fellow's code, we trace the short, miserable life of a real, live bug, from commission to detection to squashing, hopefully illustrating some general techniques for would-be exterminators.
--
About a month ago Mike Lee, my bodyguard and majordomo, was trying to replicate our biggest crashing bug in Delicious Library by running it in gdb and replicating a user's actions. This bug actually wasn't even particularly common, but since we don't have very many crashing bugs in Delicious Library (compared to other projects in which I've been involved), and since I wanted to go drinking and not hang out and watch him screw around with Delicious Library, I took an interest in what he was doing.
Mike was loading certain books from Amazon.com to re-create the library of a user who reported that when he loaded or reloaded some items they would crash Delicious Library
every time. Note the emphasis here. Whenever you hear "every time" in a bug report you should breath a sigh of relief, because you'll be able to find the bug. If something is reproducible, it's fixable. Period. It may be a pain, you may have to start cutting out large parts of your program and doing a binary search of your code, but you
can fix it if it's reproducible.
In this bug, loading certain items would always crash DL for the user, while loading other items always worked fine. Other users had different items that would cause a crash, and the vast majority of users had
no items that would cause a crash. We also couldn't get
any items to crash on our machines.
To sum up: we have a crasher bug, that's bad, but it's reproducible for the user, that's good. Different users have different cases that cause crashes, that's bad again, and Mike couldn't reproduce the bug himself, and that's real bad. Also, I want to go drink, which is good in small doses but bad in large doses. What to do?
To his credit, Mike had taken the right first step (try to duplicate the conditions of the crash), but he was starting to spend too long on it (and more time debugging is less time boozing). It was time to move on.
Step two was to examine the exception that was being thrown right before the crash.
NSArchiverArchiveInconsistency - *** NSUnarchiver: inconsistency between written and read data for object 0x0 Hmm, doesn't mean anything to me. Except, let's look up "NSArchiverArchiveInconsistency" on Google. A little reading informed me that it can be thrown when reading in bad data from an archive.
An archive? But I don't use NSUnarchivers in Delicious Library, do I? The next part of step two was to search my project in Xcode for NSUnarchiver and its superclass NSCoder. I found that I didn't use them anywhere NEAR the Amazon book-loading code. This made no sense! Sure, NSUnarchiver is also used in NIB loading, but, again, this wasn't anywhere near that kind of code.
Ok, move on. When you're looking for a bug, the best thing to do is keep an open mind. Play with ideas, try things out, then move on. Don't get hung up on one approach. Just try to get a feel for the whole system and the answer will come to you.
Step three was to look at the backtrace:
|
0x96323f10: _NSExceptionHandlerExceptionRaiser (in ExceptionHandling)
0x92896f3c: +[NSException raise:format:] (in Foundation)
0x92893ff0: __decodeObject_old (in Foundation)
0x928a5060: -[NSURLCache _diskCacheGet:] (in Foundation)
0x928a4b34: -[NSURLCache cachedResponseForRequest:] (in Foundation)
0x928a3f94: -[NSURLConnection(NSURLConnectionInternal) _performLoad] (in Foundation)
0x928a3a00: __resourceLoaderPerform (in Foundation)
0x9074afc4: ___CFRunLoopDoSources0 (in CoreFoundation)
0x9074a43c: ___CFRunLoopRun (in CoreFoundation)
0x90749ebc: _CFRunLoopRunSpecific (in CoreFoundation)
0x928a3760: +[NSURLConnection(NSURLConnectionInternal) _resourceLoadLoop:] (in Foundation)
0x9287c2b4: _forkThreadForFunction (in Foundation)
0x9002c3b4: __pthread_body (in libSystem.B.dylib) |
Oooh, that's ugly. This isn't the main thread. This isn't even a thread I created. This is Apple's thread. I never even called any of these methods, this is something that got called as a side-effect from my code.
Now, let me state something unequivocally: 98% of the time when you think you've found a bug that is not your fault, it really is your fault. The other 2% of the time... well, it's probably your fault as well.
However, in this case... well, damn. We're deep in Apple's code. But I *know* Apple's got lots of code that uses urls and the web and stuff. I mean, hey, Safari, right? They've tested this. I don't believe their dang URL cache stuff has crashers this bad in it, unless I'm doing something odd to trigger them. So I can't just say, "Meh, Apple's fault, too bad, bitches," and be done with it.
Let's look closer at the trace. At the top, we're handling an exception. Big surprise, we knew that. Below that, we're raising the exception, which we also expected. Under that, we're decoding an object. Alright, so now we've confirmed where we suspected our "NSArchiverArchiveInconsistency" exception came from -- Apple was trying to decode an object and hit something it did not like. Check. But what was being decoded, from where did it come, and why was it corrupt? There are a lot of questions we need to answer.
The first interesting line for us is the one that starts
0x928a5060, because it says,
[NSURLCache _diskCacheGet:]. Now, wait a minute. NSURLCache? That's a class I've heard of. I look that up in Apple's documentation and it does what I might expect; it's for caching stuff that is loaded off the web.
Now it's time to let you in on something that you didn't know, so you couldn't have solved this mystery up to this point: in Delicious Library we load book descriptions from Amazon.com. In August of 2005 we released our 1.5.2 version, which disabled Apple's built-in disk caching of data that is downloaded using the Cocoa frameworks. This was done because the disk cache could conceivably grow quite large (with cover images) and server no purpose: there was never any need to look at the cached data, as whenever we reloaded from Amazon.com we really wanted the latest data, not a cached version.
Back up a second. We're getting a crash in NSURLCache, in a method called
_diskCacheGet:, but I just said
that Delicious Library does not use a disk cache. Something's wrong here, right? We've got two assumptions that don't go together. You can smell the cognitive dissonance. This is your best friend. When you smell this smell, you know you're close to solving the whole thing.
What's the next step? Check your assumptions. Never say, "Well, I did blank, I know I did blank, blank is done, and that's the blank story!" Look at the actual code that does blank, and make DAMN SURE it really blanks.
How did we disable the disk cache in 1.5.2? We called
[[NSURLCache sharedURLCache] setDiskCapacity:0].
What
exactly does this do?
Check Apple's documentation. "Sets the receiver’s on-disk cache capacity to diskCapacity, measured in bytes... The on-disk cache will truncate its contents to diskCapacity, if necessary."
Now it's time for a leap of logic. We know that we're not having trouble when we're
writing to the disk cache, we're having trouble
reading from the disk cache (we turned off writing, and the backtrace clearly shows us loading from the disk, not writing to it). And, about the cache limiting call we added in 1.5.2: it doesn't actually prevent us from
reading the disk cache, it just keeps us from
writing any new data to the disk cache.
So, what happens if someone has been using Delicious Library from 1.5 or before (so they already have a disk cache, because it was enabled back then), and then they upgrade to 1.5.2, and reload one of the books they loaded under 1.5?
You wouldn't expect this to crash, because either the book is still in the disk cache, in which case it'd load from the cache (and at worst just have old data), or the cache has been correctly truncated, so the book would
not be in the cache, so the book would load from the net. In neither case would you expect a crash.
But here's where we see a possible case that Apple could have missed, because they probably haven't shipped a program that
originally had caching on, but then turned it off in a later version. So what if they never really tested their cache truncation code, and it turns out when you set the disk capacity to 0 Cocoa corrupts that program's existing URL cache, and makes it so any time you get a cache hit your program crashes? New programs that set their caches to 0 would never have a problem, and neither would old programs that don't mess with their cache size. One might reasonably think that Delicious Library was one of the few programs that had an older version with a disk cache and a new version that truncated it, and thus was the first to hit test this part of Apple's code (and find the bug).
This would explain why Mike couldn't replicate the user's problems -- he was starting on a machine that had never run 1.5 or earlier, so it didn't have any truncated (eg, corrupted) items in the URL cache (or any items at all, since we disabled writing them). It also explains why certain random items crashed for certain users upon reload -- whichever items they happened to have sitting in their URL cache when they upgraded from 1.5 to 1.5.2 would become corrupted for all time, and those would forever crash instead of loading. And it explains why a lot of users never saw this bug; they didn't reload old books or they started with version 1.5.2 or greater, so they never hit Apple's corrupted cache.
--
We have a theory, but how do we test it? Two ways: first, Mike asks some users with this problem to send us their URL cache directories (~/Library/Caches/Delicious Library/), and he copies their directories into his home directory and tries loading the items they say to cause crashes. He is able to replicate their crashes, so we've done a positive test. Mike then asks all the users who have reported this problem to blow away that URL cache directory for Delicious Library and try again. 100% report the problem is gone (yay!), and thus we've done a negative test.
We're now pretty darn sure. All that remains it to write a little code so that future users of Delicious Library don't encounter this problem. Sure, it's Apple's bug, but users don't know that and wouldn't much care even if they did; if my program crashes it's on my head, so I'm going to work around this bug. Here's the code, for your edification:
|
// Turn off NSURLCache (we sort of keep our own cache...)
and remove the existing cache directories so we don't get strange crashers from NSURLCache
(NSArchiverArchiveInconsistency) in Apple's code. (File RADAR!)
NSArray *applicationCacheDirectories = NSSearchPathForDirectoriesInDomains(NSCachesDirectory,
NSUserDomainMask, YES);
unsigned int applicationCacheDirectoryIndex = [applicationCacheDirectories count];
while (applicationCacheDirectoryIndex--)
[[NSFileManager defaultManager] removeFileAtPath:[[applicationCacheDirectories
objectAtIndex:applicationCacheDirectoryIndex] stringByAppendingPathComponent:[[NSProcessInfo processInfo]
processName]] handler:NULL];
[[NSURLCache sharedURLCache] setDiskCapacity:0]; |
From the time I noticed Mike's activity to my writing this workaround code took about a half hour. Mike spent the next day verifying the fix and nursing a hangover.
--
What are the lessons here? First off, when you're tracing down a bug, keep your mind open. Don't get married to a theory or an approach. This isn't like programming, you've got to defocus your mind, read a bunch, look at your code. Don't think "I know I did this," think, "I *thought* I did this, but maybe I'm crazy." Question EVERY LINE around the crash. Does "if" really mean what I think it does? Does "*" bind tighter than "++"? Did I really write the correct variable name there, or did I write in some different variable and I keep reading it as the correct variable because that's what I meant to write?
Second, when you've got a possible theory on the cause of a bug, don't stop until you can prove to yourself how such a bug could exist and go undetected up until now, whether it's your fault or someone else's. It's not good enough to say, for example, "Oh, must be Bob's fault." If you can't explain how and why Bob made such a mistake, and
then devise a workaround or fix, you don't really know what's going on, and thus you don't really know if you've fixed the bug. And, worse, you didn't really learn anything, so you're not going to be any better at it next time.
I never, ever fix a bug, even from another user, unless I can say with some surety that I can see the circumstances that led to that person writing that bug. It may be as simple as "oh, copy and paste error" or "this person was tired and overlooked this edge case, and that didn't come up until now." But if you see a bug and think, "How in the hell did this code ever work?" then you've got a lot more work to do (assuming the code
did ever work), because you aren't allowed to move on until you can answer that question.
Understanding your code, and the underlying code of the environment you're in, is more important than fixing any one bug.
Many mojitos died to bring you this information.Labels: code
Thursday, January 19
Here's a little code-McNugget. Recently someone wrote:
Delicious Library is a great program and I admire the ways you've customized a lot of the controls. It's always the little things that obsess me, specifically the way NSTextField in the details view in EDIT mode loses focus/committs the edit when the user mouses down anywhere it's bounding box. I've managed to produce a similar effect by adding this mouseDown method to the NSBox my NSTextField is held in.
How did you solve it? Here's my code for reference:
|
- (void)mouseDown:(NSEvent *)theEvent {
[self acceptsFirstResponder];
[[self window] makeFirstResponder:self]; // I can make myself accept firstResponder
but I have to get the parent window to make it so.
// Not sure why this works but it made sense when I typed it.
[super mouseDown:theEvent];
} |
Ok, so, restated, the question is, what code did I add to the view that contains my NSTextFields so that, when I click just outside the NSTextField (and in the custom view around them) the NSTextFields stop any editing they may be doing?
Now, I don't get off on being mean, but sometimes I feel like Lisa Simpson as Sacagawea in
Margical History Tour, speaking to the new American settlers who are eating berries, rubbing leaves on themselves, and trying to tie their belts: "These berries are poisonous, these leaves are poisonous, and your belt is a snake... which is also poisonous."
To be fair, this person did write me about this method asking for help, but, seriously, whenever I see a comment that says, "Not sure why this works..." I die a little inside. It happens a lot, which explains why I can't move most of the left side of my body any more (and why I'm still single).
ANYways, there's nothing in the original code that can be saved. First off, don't call [self acceptsFirstResponder]. It doesn't have any side-effects, and it just returns 'YES' or 'NO'. So, essentially this method as written is:
|
- (void)mouseDown:(NSEvent *)theEvent {
1;
[[self window] makeFirstResponder:self]; // I can make myself accept firstResponder
but I have to get the parent window to make it so.
// Not sure why this works but it made sense when I typed it.
[super mouseDown:theEvent];
} |
Also, don't call [super mouseDown:], because it's documented in
NSResponder thusly:
NSResponder’s implementation simply passes this message to the next responder. You don't actually WANT this message to be passed on, because you've already acted on it. In this case, it'll do nothing, but you're wasting time and lines of code for no reason.
So, that leaves us with only one line, the middle one, which is a snake, and poisonous. Ok, it's not a snake. But it's not really correct. What you want to do if you want to de-focus an actively editing NSTextView is tell the
NSWindow to set the first responder to
nil, not
self. In this case, you're just a simple NSView subclass, so it may not really hurt anything for you to be responding to events as they come out, but it's confusing and less efficient and it sure doesn't help. And, it could screw up the focus ring and tab-to-next field editing to set yourself as the first responder.
The other problem with the middle line is that you don't actually check to make sure the first responder IS something you want to de-focus before you steal the focus, which isn't too friendly of you. I mean, if the first responder is just a popup (keyboard navigation is on), it's kind of mean to steal it when it wasn't hurting anyone.
So, here we have the pimped-out method, straight from Delicious Library. Yup, I'm giving away my production code, folks! Get it while it's hot! This crap won awards, you know!
|
// NSResponder subclass
- (void)mouseDown:(NSEvent *)theEvent;
{
NSWindow *window = [self window];
if ([[window firstResponder] isKindOfClass:[NSText class]])
[window makeFirstResponder:nil];
} |
Labels: code
Saturday, December 24
Matt W. submitted this stopwatch display class for pimping:
|
/* CTStopwatchView */
#import
@interface CTStopwatchView : NSView
{
NSString *myCurrentTime;
NSFont *timerFont;
IBOutlet id textDisplay;
NSImage *bgImage;
float fontSize;
}
- (void)updateWithTimeInterval:(NSTimeInterval)time;
- (NSString *)stringFromTimeInterval:(NSTimeInterval)time;
- (void)awakeFromNib;
- (void)doGradient:(NSRect)rect;
- (void)createBackgroundImage;
- (void)viewResized:(NSNotification *)notification;
- (void)viewWillStartLiveResize;
- (void)viewDidEndLiveResize;
- (void)calculateFontSize;
- (NSString *)currentTime;
- (void)setCurrentTime:(NSString *)newTime;
@end |
All right, so we see a class that apparently draws a time interval in the format of HH:MM:SS on a washed background. How do we make it better? Let's go through it bit-by-bit.
This header needs a LOT of work. Why are we storing the
myCurrentTime as an
NSString? That's a warning sign to me right there. Time intervals should be stored as, ahem,
NSTimeIntervals. It's their native format. And we see that there is in fact a method called -updateWithTimeInterval: which takes a time interval, and I think (based on my analysis of the code) that, in fact, this is the only method that ever gets called from outside the class. So this class gets called with a nice, tiny NSTimeInterval (4 bytes) and then it immediately store this as an enormous, inefficient, less-precise NSString? I'm voting no.
Whenever I see a font being cached I also think, "uh, no." Because, seriously, creating NSFonts is practically a no-op. It's not like Cocoa is actually fetching the damn font off the disk every time you ask for it. That shit is cached down to the lowest levels. So
timerFont is toast, too.
I'd toast
textDisplay on general principle, because if you have an
NSView subclass pointing at another NSView, you've probably got some bad architecture. In this case I'm going to leave it, because I don't want to change what this class actually
does, or I'm kind of failing my mission. I am going to require that we point at an
NSCell instead of an 'id', because we certainly call methods on
textDisplay that require it to be of type NSCell. (And, it should be renamed to something like
mirroringCell if it were to stay in real life, which it wouldn't.)
bgImage? Nope. Unless you're doing some impressive-ass drawing of your background, don't cache that crap. First off, because it can be hugely inefficient; the window already caches you once, and if the window is big, then you're just double-caching a huge image, which is gonna be inefficient. And, if the window is small, well, WHAT THE HELL IS WRONG WITH YOU that you're writing background drawing code that's so slow?
bgImage joins the dodo bird and honest politicians on the extinct list.
Surely I'll keep
fontSize, though, right? Hah! You underestimate how much I hate instance variables.
I hate them a lot. They confuse everything because their scope is
global within the class file, even if they are only used in one or two methods. So, you can't easily tell where they are set, used, and modified. You have to search all over for them. Any time you can take them and make them local to a method (or two) instead, you've won. I'm downsizing
fontSize.
But what about the method declarations? Well, we have three major problems here:
- They aren't grouped in any meaningful way,
- Inherited methods are redundantly declared here, and, most egregiously,
- Private methods appear in the header file!
I decide that -updateTimeInterval: was probably the only method that got called from the outside world, but I don't much care for the word "update", so I renamed it to something more standard for a setter method — -setTimeInterval:. Then, because I'm crazy that way, I added a matching accessor method, -timeInterval. That's how
I roll, baby.
Here's my new header class. Notice how it's, like, almost empty?
|
// CTStopwatchView
#import
@interface CTStopwatchView : NSView
{
IBOutlet NSCell *mirroringCell;
NSTimeInterval timeInterval;
}
// API
- (void)setTimeInterval:(NSTimeInterval)newTimeInterval;
- (NSTimeInterval)timeInterval;
@end
|
Ahh. Smell the deleted-code goodness. Smells like... readability.
"But," you exclaim, "you can't just delete code willy-nilly and expect everything to work! How are you going to back these changes up?" And my answer is: "Don't call me willy-nilly."
Here's the original class file:
|
#import "CTStopwatchView.h"
#define FONT_SIZE_RATIO 1.2
#define BASE_FONT_SIZE 30.0
@implementation CTStopwatchView
- (void)awakeFromNib
{
myCurrentTime = @"0:00:00";
[textDisplay setEnabled:NO];
[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(viewResized:)
name:NSViewFrameDidChangeNotification object:self];
[self calculateFontSize];
}
- (id)initWithFrame:(NSRect)frameRect
{
if ((self = [super initWithFrame:frameRect]) != nil) {
// Add initialization code here
[self createBackgroundImage];
}
return self;
}
- (void)drawRect:(NSRect)rect
{
NSRect bounds = [self bounds];
[bgImage compositeToPoint:bounds.origin fromRect:bounds operation:NSCompositeSourceOver
fraction:1.0];
NSShadow *shadow = [NSShadow alloc];
[shadow setShadowOffset:NSMakeSize(2.0,-2.0)];
[shadow setShadowBlurRadius:3.0];
[shadow setShadowColor:[NSColor colorWithDeviceWhite:0.0 alpha:0.7]];
NSDictionary *attrs = [NSDictionary dictionaryWithObjectsAndKeys:
[NSFont fontWithName:@"Lucida Grande" size:fontSize], NSFontAttributeName,
shadow, NSShadowAttributeName,
[NSColor whiteColor], NSForegroundColorAttributeName,
nil];
NSAttributedString *string = [[NSAttributedString alloc] initWithString:myCurrentTime
attributes:attrs];
NSSize size = [string size];
[string drawInRect:NSMakeRect((bounds.size.width - size.width) / 2, ((bounds.size.height
- size.height) / 2) + (size.height / 15), size.width, size.height)];
[string release];
[shadow release];
}
- (NSString *)currentTime;
{
return myCurrentTime;
}
- (void)setCurrentTime:(NSString *)newTime;
{
if (myCurrentTime != newTime) {
[newTime retain];
[myCurrentTime release];
myCurrentTime = newTime;
}
}
- (void)updateWithTimeInterval:(NSTimeInterval)time
{
[self setCurrentTime:[self stringFromTimeInterval:time]];
[self setNeedsDisplay:YES];
[textDisplay setTitle:[self currentTime]];
}
- (NSString *)stringFromTimeInterval:(NSTimeInterval)time
{
int seconds, minutes, hours;
hours = time / 3600;
time = time - (hours * 3600);
minutes = time / 60;
time = time - (minutes * 60);
seconds = time;
return [NSString stringWithFormat:@"%d:%02d:%02d", hours, minutes, seconds];
}
- (BOOL) acceptsFirstResponder;
{
return YES;
}
- (void)createBackgroundImage
{
[bgImage release];
NSRect bounds = [self bounds];
bgImage = [[NSImage alloc] initWithSize:bounds.size];
[bgImage lockFocus];
[self doGradient:bounds];
[bgImage unlockFocus];
}
- (void)doGradient:(NSRect)rect
{
NSColor *lightColor = [NSColor selectedControlColor];
NSColor *darkColor = [NSColor alternateSelectedControlColor];
float height = rect.size.height;
[darkColor set];
[NSBezierPath fillRect:rect];
[NSBezierPath setDefaultLineWidth:0.5];
float i;
for (i = 0; i < height; i++) {
[[lightColor colorWithAlphaComponent:(i / height)] set];
[NSBezierPath strokeLineFromPoint:NSMakePoint(0,i + 0.5)
toPoint:NSMakePoint(rect.size.width, i + 0.5)];
}
for (i = height; i > height / 2; i--) {
[[NSColor colorWithDeviceWhite:1.0 alpha:((height - i) / (height * 1.2))] set];
[NSBezierPath strokeLineFromPoint:NSMakePoint(0, i + 0.5)
toPoint:NSMakePoint(rect.size.width, i + 0.5)];
}
[lightColor release];
[darkColor release];
}
- (void)calculateFontSize
{
NSRect bounds = [self bounds];
NSDictionary *attrs = [NSDictionary dictionaryWithObjectsAndKeys:
[NSFont fontWithName:@"Lucida Grande" size:BASE_FONT_SIZE], NSFontAttributeName,
nil];
NSAttributedString *string = [[NSAttributedString alloc] initWithString:myCurrentTime
attributes:attrs];
NSSize size = [string size];
float newFontSize;
if ((size.width / bounds.size.width) > (size.height / bounds.size.height)) {
// Size text by width
float newWidth = bounds.size.width / FONT_SIZE_RATIO;
newFontSize = floorf(BASE_FONT_SIZE * (newWidth / size.width));
} else {
// Size text by height
float newHeight = bounds.size.height / FONT_SIZE_RATIO;
newFontSize = floorf(BASE_FONT_SIZE * (newHeight / size.height));
}
[string release];
fontSize = newFontSize;
}
- (void)viewResized:(NSNotification *)notification
{
[self createBackgroundImage];
[self calculateFontSize];
[self setNeedsDisplay:YES];
}
- (void)viewWillStartLiveResize
{
[super viewWillStartLiveResize];
}
- (void)viewDidEndLiveResize
{
[self createBackgroundImage];
[self calculateFontSize];
[self setNeedsDisplay:YES];
[super viewDidEndLiveResize];
}
@end |
This is the first time I've actually taken the code and put it into a project and rewritten it to make sure my pimping works, because I made such extensive changes.
Let's pick this apart method-by-method, then I'll show you my rewritten file.
#import "CTStopwatchView.h"
#define FONT_SIZE_RATIO 1.2
#define BASE_FONT_SIZE 30.0 |
Don't define these constants here. They are not important enough to be at the top of the file, and they confuse the reader who sees them before anything else. Putting them here makes me think you're going to use them all over; whereas if you define them right when you use them, the scope is more obvious.
- (void)awakeFromNib
{
myCurrentTime = @"0:00:00";
[textDisplay setEnabled:NO];
[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(viewResized:)
name:NSViewFrameDidChangeNotification object:self];
[self calculateFontSize];
}
|
Don't bother registering for the NSViewFrameDidChangeNotification notification. You get called with -drawRect: whenever your size changes, and that's the correct time to recache any images at a new size, because the mere fact that your frame changed does NOT necessarily mean you're going to be redrawn, and you don't want to do excessive caching of images you aren't going to use. Also, locality is better. Also, less registrations is better. Also, if you're going to register for a notification, you
damn well better call -removeObserver:self in your -dealloc method, which you don't do, which gums up the registration machinery and can cause crashes.
- (id)initWithFrame:(NSRect)frameRect
{
if ((self = [super initWithFrame:frameRect]) != nil) {
// Add initialization code here
[self createBackgroundImage];
}
return self;
} |
Again, you should just create the background image at the last second in -drawRect: if the size has changed since the last time the view was drawn. That is, if you need to create a background image at all, which you usually don't. I'm nuking this whole method.
- (void)drawRect:(NSRect)rect
{
NSRect bounds = [self bounds];
[bgImage compositeToPoint:bounds.origin fromRect:bounds operation:NSCompositeSourceOver
fraction:1.0];
NSShadow *shadow = [NSShadow alloc];
[shadow setShadowOffset:NSMakeSize(2.0,-2.0)];
[shadow setShadowBlurRadius:3.0];
[shadow setShadowColor:[NSColor colorWithDeviceWhite:0.0 alpha:0.7]];
NSDictionary *attrs = [NSDictionary dictionaryWithObjectsAndKeys:
[NSFont fontWithName:@"Lucida Grande" size:fontSize], NSFontAttributeName,
shadow, NSShadowAttributeName,
[NSColor whiteColor], NSForegroundColorAttributeName,
nil];
NSAttributedString *string = [[NSAttributedString alloc] initWithString:myCurrentTime
attributes:attrs];
NSSize size = [string size];
[string drawInRect:NSMakeRect((bounds.size.width - size.width) / 2, ((bounds.size.height
- size.height) / 2) + (size.height / 15), size.width, size.height)];
[string release];
[shadow release];
} |
The NSShadow is the same every time you create it. I'd use a class variable: having these four lines of code in -drawRect: makes the viewer scan them closely to see if anything changes in the shadow from one call to the next, which it doesn't. If something's a constant, make it clear that it's a constant by only doing it once. (Also, we're going to rewrite this method a ton when we get rid of the cached
bgImage and the cached
fontSize instance variables.)
- (NSString *)currentTime;
{
return myCurrentTime;
}
- (void)setCurrentTime:(NSString *)newTime;
{
if (myCurrentTime != newTime) {
[newTime retain];
[myCurrentTime release];
myCurrentTime = newTime;
}
} |
Nothing too wrong with these, except I hate instance variables that start with 'my' (of COURSE it's yours, whose else would it be?) and we're keeping an NSString instead of an NSTimeInterval and your -set... method uses the large if(){} block instead of returning early AND it does not correctly call -setNeedsDisplay: or update the dependent view you defined, yet it appears to be a public method that someone might innocently call. Also, you should use -copy when retaining a string that's been passed to you, not -retain, so that, if
newTime was mutable, you'll make sure it won't mutate out from under you. Note that if
newTime is
not mutable -copy is exactly the same code as -retain, so it's no slower to be safe.
- (void)updateWithTimeInterval:(NSTimeInterval)time
{
[self setCurrentTime:[self stringFromTimeInterval:time]];
[self setNeedsDisplay:YES];
[textDisplay setTitle:[self currentTime]];
} |
This is going to become our new -setTimeInterval: method. It's ok, except for its name.
- (NSString *)stringFromTimeInterval:(NSTimeInterval)time
{
int seconds, minutes, hours;
hours = time / 3600;
time = time - (hours * 3600);
minutes = time / 60;
time = time - (minutes * 60);
seconds = time;
return [NSString stringWithFormat:@"%d:%02d:%02d", hours, minutes, seconds];
} |
Too long for what it does, and it should be a private method, and it should just use the cached NSTimeInterval that's replacing the string.
- (BOOL) acceptsFirstResponder;
{
return YES;
} |
This is a method subclassed from NSView. Really, really ought to say so. And be grouped with other NSView methods.
- (void)createBackgroundImage
{
[bgImage release];
NSRect bounds = [self bounds];
bgImage = [[NSImage alloc] initWithSize:bounds.size];
[bgImage lockFocus];
[self doGradient:bounds];
[bgImage unlockFocus];
} |
Gone in sixty seconds.
- (void)doGradient:(NSRect)rect
{
NSColor *lightColor = [NSColor selectedControlColor];
NSColor *darkColor = [NSColor alternateSelectedControlColor];
float height = rect.size.height;
[darkColor set];
[NSBezierPath fillRect:rect];
[NSBezierPath setDefaultLineWidth:0.5];
float i;
for (i = 0; i < height; i++) {
[[lightColor colorWithAlphaComponent:(i / height)] set];
[NSBezierPath strokeLineFromPoint:NSMakePoint(0,i + 0.5)
toPoint:NSMakePoint(rect.size.width, i + 0.5)];
}
for (i = height; i > height / 2; i--) {
[[NSColor colorWithDeviceWhite:1.0 alpha:((height - i) / (height * 1.2))] set];
[NSBezierPath strokeLineFromPoint:NSMakePoint(0, i + 0.5)
toPoint:NSMakePoint(rect.size.width, i + 0.5)];
}
[lightColor release];
[darkColor release];
} |
This method uses NSBezierPath's -strokeLineFromPoint:toPoint: when it
really wanted to use NSRectFillUsingOperation(), which is optimized for drawing straight lines and thus a zillion times faster. Also, what you should
really, really do is use CGShadingCreateAxial(), but that's a lot more code. Still, should be WAY faster, if you're worried about speed. I was, frankly, too lazy to do this, but I'll (-cough-) leave it as an exercise to the reader. (In my timing tests NSRectFillUsingOperation() was plenty fast enough.) Also, you're releasing
lightColor and
darkColor here, but you don't have a retain on them; methods like +selectedControlColor return a (conceptually) autoreleased instance of the color. In this case you might not be crashing because the class method is actually returning a single, unique color instance that refuses to ever deallocate itself; you lucked out.
- (void)calculateFontSize
{
NSRect bounds = [self bounds];
NSDictionary *attrs = [NSDictionary dictionaryWithObjectsAndKeys:
[NSFont fontWithName:@"Lucida Grande" size:BASE_FONT_SIZE], NSFontAttributeName,
nil];
NSAttributedString *string = [[NSAttributedString alloc] initWithString:myCurrentTime
attributes:attrs];
NSSize size = [string size];
float newFontSize;
if ((size.width / bounds.size.width) > (size.height / bounds.size.height)) {
// Size text by width
float newWidth = bounds.size.width / FONT_SIZE_RATIO;
newFontSize = floorf(BASE_FONT_SIZE * (newWidth / size.width));
} else {
// Size text by height
float newHeight = bounds.size.height / FONT_SIZE_RATIO;
newFontSize = floorf(BASE_FONT_SIZE * (newHeight / size.height));
}
[string release];
fontSize = newFontSize;
} |
Way too many lines for what this does, AND we're caching a size, which I just hate. Look, if we're drawing this less than a million times a second, it's just not going to make a difference, and there's glue code all over this dang class to make sure the font size and background image and string and everything stay updated, and that stuff just makes code harder to read, harder to change, unstable, fragile, and rigid.
- (void)viewResized:(NSNotification *)notification
{
[self createBackgroundImage];
[self calculateFontSize];
[self setNeedsDisplay:YES];
} |
We're nuking this method, because we do all our calculations in -drawRect:.
- (void)viewWillStartLiveResize
{
[super viewWillStartLiveResize];
}
- (void)viewDidEndLiveResize
{
[self createBackgroundImage];
[self calculateFontSize];
[self setNeedsDisplay:YES];
[super viewDidEndLiveResize];
}
@end |
These last two methods actually do nothing right now, and appear to just be vestigial from when the author was trying to get resizing to work correctly. Unsurprisingly, they're nuked.
Ok, so here's the new class. Note how methods got renamed a bit and moved around according to where they come from. Note also that logic lines (like the math to calculate font sizes from the bounds) tend to be a little bit shorter. It's very important to write "logic" parts of your program in as few lines as possible, because these are the lines that are hardest to apprehend at a glance, and lead to the most errors.
|
#import "CTStopwatchView.h"
@interface CTStopwatchView (Private)
- (NSString *)_stringFromTimeInterval;
- (void)_drawGradientInRect:(NSRect)bounds;
@end;
@implementation CTStopwatchView
static NSShadow *CTStopwatchViewTextShadow = nil;
// NSObject (class)
+ (void)initialize;
{
CTStopwatchViewTextShadow = [[NSShadow alloc] init];
[CTStopwatchViewTextShadow setShadowOffset:NSMakeSize(2.0,-2.0)];
[CTStopwatchViewTextShadow setShadowBlurRadius:3.0];
[CTStopwatchViewTextShadow setShadowColor:[NSColor colorWithDeviceWhite:0.0 alpha:0.7]];
}
// NSObject (NSNibAwaking)
- (void)awakeFromNib
{
[self setTimeInterval:2 * 3600 + 18 * 60 + 43];
[mirroringCell setEnabled:NO];
}
// NSView
- (BOOL)acceptsFirstResponder;
{
return YES;
}
- (void)drawRect:(NSRect)rect
{
#define FONT_SIZE_RATIO (1.2)
NSRect bounds = [self bounds];
// draw background wash
[self _drawGradientInRect:bounds];
// calculate font size for stopwatch text
NSString *timeIntervalString = [self _stringFromTimeInterval];
const float baseFontSize = 30.0;
NSString *fontName = @"Lucida Grande";
NSAttributedString *attributedString = [[NSAttributedString alloc]
initWithString:timeIntervalString attributes:[NSDictionary dictionaryWithObjectsAndKeys:
[NSFont fontWithName:fontName size:baseFontSize], NSFontAttributeName, nil]];
NSSize stringSize = [attributedString size];
[attributedString release];
float biggestRatio = MAX(stringSize.width / NSWidth(bounds),
stringSize.height / NSHeight(bounds));
float fontSize = (baseFontSize / biggestRatio) / FONT_SIZE_RATIO;
// draw stopwatch text
attributedString = [[NSAttributedString alloc] initWithString:timeIntervalString
attributes:[NSDictionary dictionaryWithObjectsAndKeys:[NSFont fontWithName:fontName
size:fontSize], NSFontAttributeName, CTStopwatchViewTextShadow, NSShadowAttributeName,
[NSColor whiteColor], NSForegroundColorAttributeName, nil]];
stringSize = [attributedString size];
[attributedString drawInRect:NSMakeRect((NSWidth(bounds) - stringSize.width) / 2,
(NSHeight(bounds) - stringSize.height) / 2 + stringSize.height / 15, stringSize.width,
stringSize.height)];
[attributedString release];
}
// API
- (void)setTimeInterval:(NSTimeInterval)newTimeInterval;
{
if (ABS(timeInterval - newTimeInterval) < 0.1)
return;
timeInterval = newTimeInterval;
[self setNeedsDisplay:YES];
[mirroringCell setTitle:[self _stringFromTimeInterval]];
}
- (NSTimeInterval)timeInterval;
{
return timeInterval;
}
@end
@implementation CTStopwatchView (Private)
- (NSString *)_stringFromTimeInterval;
{
return [NSString stringWithFormat:@"%d:%02d:%02d", (int)(timeInterval / 3600),
((int)timeInterval % 3600) / 60, (int)timeInterval % 60];
}
- (void)_drawGradientInRect:(NSRect)bounds;
{
[[NSColor alternateSelectedControlColor] set];
NSRectFill(bounds);
NSColor *lightColor = [NSColor selectedControlColor];
float height = NSHeight(bounds);
unsigned int row;
for (row = 0; row < height; row++) {
[[lightColor colorWithAlphaComponent:((float)row / height)] set];
NSRectFillUsingOperation(NSMakeRect(0, row, NSWidth(bounds), 1), NSCompositeSourceOver);
}
for (row = height; row > height / 2; row--) {
[[NSColor colorWithDeviceWhite:1.0 alpha:((height - row) / (height * 1.2))] set];
NSRectFillUsingOperation(NSMakeRect(0, row, NSWidth(bounds), 1), NSCompositeSourceOver);
}
}
@end |
This rewrite is a lot fewer actual lines of code and a lot more whitespace and comments. I think it's easier to read and modify without sacrificing speed.
Thanks to Matt submitting his code to such cruelty. You're a braver man than me, sir.
Can you do better on this pimping? I only had so much time, and I think this could be a little pimpier. What's your input?
Labels: code
Monday, November 21
abstractApple's shared frameworks are very useful for sharing executable code and its associated resources among multiple applications, but were historically not designed to be created by authors of consumer applications. I discourage developers from creating frameworks as a method of sharing code, because they encourage code bloat, increase launch times, complicate the developer's fix/compile/link/debug cycle, and require extra effort in setting up correct and useful developer and deployment builds.
articleThe Origins of Shared Frameworks on NeXTstepIt will help us to understand the limitations of shared frameworks if we examine the prevailing conditions for the time when they were created on NeXTstep. The user-base of NeXT machines was largely businesses and schools; institutions which generally had a knowledgeable system administrator in charge of clusters of machines and which typically ran suites of custom applications; consumer software for the NeXT was not as common. The net was not as broadly adopted, so application patches were largely unheard of; bug fixes and new versions were relegated to major releases.
Application installation in those days was done by the Installer, which could install resources in different directories throughout the disk, and left a record ("receipt") of what it had done so the installations could be reversed. Many sites also had a server machine vending a shared disk of applications and resources, so application installation only had to be done once.
Disk space was much more expensive, as was RAM. Any way to minimize the use of both was a huge win.
Shared frameworks were created by NeXT (and many others at the time) to allow multiple applications that need to use common code (eg, window handling or graphics drawing code) to link against a single binary, and thus even when, say, ten applications are running that use graphics, only a
single version of NeXT's graphics drawing code has be to physically in memory.
NeXT's take on shared frameworks was more clever than most, in that frameworks took the form of directories, containing the code and its associated resources: localization files, images, sounds, and interface descriptions (
NIBs). This provided a very convenient encapsulation of shared system resources (although there weren't provisions for multiple applications sharing a single copy of these).
Initially NeXT kept shared framework creation to themselves, but in a later release they allowed third-party developers to create them as well. For developers of custom applications for businesses, this was a godsend, as typically custom applications come in suites, and have a ton of shared code. For instance, at AT&T Wireless (an early NeXT adopter), they might have an application where they can look up the last call a subscriber made, and another application where they can change a subscriber's rate plan. The basic code to look up and display subscribers could now be shared between the two applications, which
saves on disk space and physical memory when the program is run, and
provided an easy way to re-use code and resources in multiple applications for developers.
The distinction between the two functions is important to make, because almost all third-party developers now take advantage of only the second benefit when they create their own shared frameworks, and the crux of my argument is that shared frameworks are a relatively inefficient way to accomplish this one result.
In order to benefit from the savings in disk space and physical memory in an installation, an end-user need two critical things: she needs to have installed two applications from a single vendor, and she needs to have installed the frameworks associated with those applications in a shared folder (typically, /Library/Frameworks/), so that all the applications know where to look for them.
This was not a problem when applications were installed with the Installer, because the Installer could, as stated, put files in multiple directories throughout the system. However, the trend in consumer applications (started by Omni, and rewarded with one of our first Apple Design Awards) became to have simple drag-and-drop installation of the application to wherever the user chose, which precluded installing frameworks in a well-known shared folder.
But not only did application creators who had a suite of applications that all used the same frameworks not know
where the user might install those applications, they also didn't know
which applications might be installed, so all "shared" frameworks had to be bundled inside even the smallest of applications.
This is the case today with Omni's applications; if you look inside the application wrapper (control-click on an application, and select "Show package contents") you'll find a Contents/Frameworks folder with copies of all relevant Omni frameworks in
every Omni application you own.
Thus, there is no space savings from third-party frameworks, either in RAM or disk space.
Difficulties for DevelopersBut, the situation is worse than this. Frameworks are made of position-independent code, so they can be relocated in memory when linked against any arbitrary client application. This code is larger and slower than non-position-independent code, AND requires a quick fix-up at run-time when the client application launches. This fix-up time can become quite lengthy if there are lots of frameworks with lots of symbols used by any given application; in the old days Omni applications would spend many seconds loading their frameworks on launch.
[I don't mean to keep picking on Omni, but since I had a big hand in creating their open source frameworks, which are one of the largest and most popular collections of open source Cocoa code out there, I know details of the problems we encountered, and that I solved by giving up on frameworks.]
This can be ameliorated by creating frameworks at a fixed location, but you have to (a) read a bunch to understand how to do this, (b) make sure no two of your frameworks overlap, and you don't overlap anyone else's third-party frameworks that you might use, and (c) set up your build system to do it. This was actually a huge hassle at Omni; I was on the team that tried to streamline the process and it's still not something that is easily grasped by newcomers to coding.
But the situation is worse than that, because even after an application loads a framework into the right place in memory, the application has to adjust all its own symbols to point to the correct places in the framework. Luckily, Cocoa handles all of this for you, but unluckily, it takes time.
Again, this can be ameliorated by prebinding against the frameworks to which you've already assigned a fixed address (from above). Again, this is kind of a complicated process, and you spend a bunch of time learning nm and other tools trying to figure out if your application is "correctly prebound" with the frameworks.
But, it gets worse. When developing, your application probably does NOT want to prebind against the frameworks, because prebinding takes extra time in the link cycle. So, you have more setup in Xcode to sometimes prebind and sometimes not.
These are just the problems and slowdowns with loading frameworks at run time. There are also myriad problems in having your source code split up into several projects in Xcode (eg, a client application project and a framework project).
For instance, you probably have some build settings you want to enforce on your project; you like to debug with optimization level -O1, but you like to deploy with -Os like Apple recommends this week. Well, you'll have to copy those settings to every Xcode project for every framework you have. And make sure they stay updated.
Does this sound like duplicated code? The thing I hate most in the world? Yes, yes it does.
Also, you need to make sure when you're doing a development build that you link against the development versions of the framework you built in your development directory (because it's a pain to copy in the enormous with-symbols versions of the frameworks into your application every time you do a debug build), but you also need to make sure your deployment rules correctly
do copy the frameworks into the client application's wrapper, or the app won't run. So, you get to write a bunch more Xcode configuration code, and, hey, you get to copy it into every application you write. And, oh boy, if you update it one place, be sure to remember to update it everywhere. (I believe the very latest Xcode has shared settings to help this situation, but you still have to write the darn configuration code.)
And, you need to make darn sure you don't confuse which versions of the frameworks you are linked against at any one time; eg, if you make a special debug build of the frameworks with some odd options, then you debug one of your other applications which use the frameworks, you'll suddenly have some very odd behaviors. (This happens more than one might imagine.)
And, when you do a deployment build, you want to make sure you strip the headers from your frameworks, as well as any subversion files. Yes, you get to write more Xcode configuration code. And copy it around to all your applications. Oh, boy.
Difficulties for End-UsersNow, why are you creating shared frameworks again? Because you want to re-use code in multiple projects. This is a noble goal. However, frameworks make it
too easy to just throw every dang class that
might be useful into them, so the developer forgets to ever prune old files that are no longer used at all. Because, if you have multiple framework projects and multiple application projects which use them, and you wonder if any particular framework class is used, you have to search through
all of these projects for references. At Omni, this was forty-something projects to open; just opening them all took minutes, let alone searching them.
So you end up with a lot of extra code in your application that none of you applications still use. How do I feel about extra code?
But also, even worse, you end up linking in a lot of extra code that some applications use, but
not this application. Because, unlike a standard library, there's no such thing as partially including a shared framework; you either grab all of its functionality or none.
How bad of a problem is this? Well, consider
OmniDictionary. It connects to the network, downloads a definition from a dictionary server, displays the HTML. Simple. It uses some of the classes from the Omni frameworks to do this, but not nearly all of them.
Still, it has to include ALL of the code from any framework it touches, and ALL of the associated resources (image files, NIBs, etc), and ALL of the frameworks which that framework uses, as well.
With OmniDictionary, the frameworks it bundles with take up 1.6 megabytes. The actual application takes up 448
kilobytes. The application takes up
four times as much space because of the frameworks.
The Alternatives for Code Re-useDownload and install
subversion, and store all your source code in it, religiously.
Create a folder in your source code repository for each of your applications, and a folder for shared code and resources. In each project, in Xcode, drag
only the files you really need into your project, and tell Xcode
not to copy them into the project directory, and instead refer to them relative to the source directory.
Removing dead code from your shared folder is easy; at the worst you can just delete the files and do a build of your applications, and if the build succeeds you're not using them. Or you can open
only your applications' projects (because what were your 'frameworks' now don't have projects associated with them) and search for the code, because your source files will
never have any header imports of the form "#import
"; they will have each file imported separately, and thus listed out for you to review and remove. (Conversely, if you decide to sunset a particular piece of code, it's very easy to find where it is referenced and rewrite the code. This happened recently to me when 10.4's NSXMLNode replaced my custom XML reading and writing classes.)
You don't spend any time configuring the frameworks' build environment because there isn't one. You don't spend any time setting up your frameworks' header file (eg, OmniAppKit.h) because there isn't one. You don't worry about whether your framework is pre-bound or position-independed because it doesn't exist.
What are the drawbacks? Well, when you include a source file in your application from your shared directory, you'll have to pay attention to which NIBs and images the files requires from the shared directory, and drag those into your application yourself. I think this is actually kind of good, because it also makes it possible to sunset images and NIBs. And, if you forget to drag in an image or NIB initially, it's an error you'll find immediately, the fix is easy, and it only needs to be done once. Thus, this isn't a problem that needs a complex solution.
Of course, if you have multiple frameworks, you might create a separate shared source directory for each one just to conceptually organize them. Or, keep them in subfolders. Whatever you like.
Note that subversion makes it very easy to modify your shared code for one project and not worry about affecting other projects; you can branch your top-level directory for any given project in subversion, and it effectively creates a copy-on-write version of that project, so you don't burn your entire source disk with a thousand versions of all your frameworks. When you're happy with a project, you can merge it back to the head (with the shared code changes) and check at _that time_ that your other applications like your newer and hopefully improved shared classes.
This brings up another issue, which is that with only one project per application it's much easier to associate a single tag with any given build, and then if a user or beta-tester encounters bugs you can go back to that version of the your source code with a single command. The problem with frameworks is it is too easy to ship different versions of the frameworks with any given build, so you end up reporting all your framework versions to the user to mention in bug reports, which is just another thing for the developer to have to check, and another avenue for error.
To get around this, developers with frameworks will write scripts to update their frameworks in a clean place and do a brand new build, but then those scripts have to be maintained by someone, and developers have to be educated about a particular site's practices. For instance, at Omni, only a few people in know how to do a beta build of a particular piece of software (not including many of the developers, but including the support personnel), so the beta process can be entirely gated by the availability of the support team.
In my alternative system, creating a beta build is as easy as opening the Xcode project, updating (inside Xcode), changing the target to "deployment", cleaning, and building. These steps are well-known to any developer, as they are all the same (except one) as during development.
Who Should Create Frameworks
Apple should, for one; because their frameworks have static base addresses, and because we all link against them at compile time, we don't suffer the same overhead by default that we do for third-party frameworks. Further, because every application is going to use, say, AppKit, and AppKit is installed in a known location, Apple's frameworks are correctly shared and result in saved RAM and saved disk space.
Companies writing suites of custom software, where they can use an Installer package and write the frameworks in a standard location, are also encouraged to use frameworks for the same reasons that Apple is.
summary
Creating shared frameworks is a lot of hassle for third-party developers of consumer software, introduces instability into the development process, and encourages slower and larger applications. Code sharing is better accomplished through creating new directories for shared code in subversion and judiciously including only the files and resources needed by any application in its Xcode project.
finis
Irony and sarcasm do not translate well onto the web. The title of this article and the previous Unit testing is teh suck, Urr. (sic) are references to the teachings of the Mooninites Ignignokt (who speaks in a sarcastic singsong and flips off the entire earth as hard as he can; "The pain is excruciating, but it's worth it") and Err (who is the Beavis to Ignignokt's Butthead).
The Mooninites are know-it-alls who come from a land where everything is better than here on earth, and they are far superior to us in every way. They pity us and our pathetic three dimensions, because on the moon they have five...
Err: THOUSAND!
Yes, five THOUSAND dimensions. Don't question it.Labels: code
Sunday, October 9
An anonymous comment on my last post got me thinking about my favorite macro (which I'd posted), and how it could be improved.
Here it is again, for reference:
|
static inline BOOL IsEmpty(id thing) {
return thing == nil
|| ([thing respondsToSelector:@selector(length)]
&& [(NSData *)thing length] == 0)
|| ([thing respondsToSelector:@selector(count)]
&& [(NSArray *)thing count] == 0);
} |
Now, I like this macro because I don't have to worry about the class of the Cocoa object I pass into it; it pretty much works for any primitive type that has an 'empty' state. (Notably not NSScanner, though.)
Some people didn't like it because it does an extra compare with nil, which is unnecessary under old versions of Cocoa because sending any method to nil returns nil, which is synonymous with the integer 0 or the value FALSE on all current implementations of Cocoa (but NOT synonymous with the float 0.0, DO NOT BE FOOLED).
However, Apple is attempting to deprecate this shortcut, because they can make the runtime more efficient if they know that no receivers are nil (c.f. the new GCC 4.0 compiler flags: GCC_NO_NIL_RECEIVERS or -fno-nil-receivers).
Since a nil comparison is REALLY quick: like, you can do a billion a second, since a zero comparison should be one instruction for a in-register variable, and you're going to have to load the variable up for the Objective-C call to -length or -count if you're not nil anyways. Also, if the receiver *is* nil, you never have to send the obj-c message at all, which saves you a function call, so this is actually faster.
So, I think the explicit comparison with nil is great. But, what about the -respondsToSelector:? It struck me that GNU C has had (for a LONG time) the ability to tell the type of a variable passed into a macro. Now, I'm not really good with this stuff (I've only used it once or twice), so I don't know if this is possible, but it seems like it MIGHT be possible to rewrite the macro so that if 'thing' is of a known type (NSArray or NSString or whatever) then we immediately check it for nil and then send it the CORRECT method (-count or -length or whatever, respectively), and we only fall back on the code I have if we pass in a variable that is truly of type 'id', which is pretty rare.
Possible? I'm not sure. But there's a handmade, one-of-a-kind
Delicious Monster stuffed sock monster for the first person who does it and posts the (non-copyrighted) code here for everyone to use. (You can see an example of this artist's other work
on the DrunkenBlog, where drunkenbatman has snaps of the cow she did for his logo.) Unlike the 'virus' post a couple days ago, this is not a trial balloon; contest starts RIGHT NOW and you get the sock monster if your code works, biggity-bam.
Labels: code
Saturday, October 8
Ok, I wanted to share a file from my shared source directory, because I don't have very many things in my shared source directory. I'm very picky this time about what I consider "shared" -- I have to actually USE code in two different projects to consider it shared, not just think "Hmm, someday somebody may want to re-use this." Because, in truth, most of the crap people put into shared code directories is either too specific to really be shared, OR (more commonly) it's written in a general way but the particular app it was written for only tested some of the pathways, so it is essentially a bunch of buggy code that's not actually being used and is waiting to trip you up.
And, yes, if you'd like you can compound this mess by writing test cases for the code you don't use, to prove that it does what you think it might do even though nobody actually cares. And, yes, I've been paid to do this for people.
All that said, here's the single-most used file in my shared repository:
|
static inline BOOL IsEmpty(id thing) {
return thing == nil
|| ([thing respondsToSelector:@selector(length)]
&& [(NSData *)thing length] == 0)
|| ([thing respondsToSelector:@selector(count)]
&& [(NSArray *)thing count] == 0);
} |
Yup, that's the whole file. You WOULD NOT BELIEVE how often I use this macro. At least once per method, often twice or three times.
Essentially, if you're wondering if an NSString or NSData or NSAttributedString or NSArray or NSSet has actual useful data in it, this is your macro. Instead of checking things like "if (inputString == nil || [inputString length] == 0)" you just say, "if (IsEmpty(inputString))".
Seriously, this code may seem butt-obvious but you're going to find a million places in your code to use it. Pretty much every time I was just checking for nil, I now call IsEmpty(), because, hey, if someone just passed me in an empty stub of a string or array or whatever, that's still empty. I probably want to do the same thing.
This is especially important in Cocoa, because it's Apple's convention to return empty objects instead of 'nil' if the set of things you are looking for is empty; for instance, if you call -selectedObjects on a Cocoa instance that has no selection, you can expect to get an empty NSArray, not nil. (A glaring exception to this is the IOBluetooth toolkit, which sometimes returns nil instead of an empty array when you ask for a list of devices, which can break your code silently and badly if you're not expecting it. I filed a bug on this (RADAR #4093573) but, sadly, the engineers decided their behavior was correct, even though it Cocoa's stated conventions.)
Apple has also (mostly) cracked down on passing in 'nil' to Cocoa when you should pass in some empty object -- most of the time you'll get an exception thrown nowadays if you, say, set something to have a string value of 'nil' instead of @"" (c.f. NSMutableString documentation).
However, I use 'nil' all the time in my code, so at any given time, for any given method, I'm never really sure if empty input will be some empty object or 'nil'. Hence, IsEmpty(), which lets my code function perfectly without worrying what convention the caller is using.
Labels: code
Monday, October 3
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.
|
/* 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.
|
// 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...
|
- (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:
|
- (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: code
Thursday, September 22
Recently an innocent reader inquired the following of me:
I'm curious to know how you approach product testing in such a small company. Do you have an established process? Do you use unit testing? Other structured testing? [...] Do you have suggestions specific to solo testing and debugging?
My answer, perhaps more politely phrased, was, like another famous Wil(l), "AW HELL NAW!"
I've certainly known companies that do "unit testing" and other crap they've read in books. Now, you can argue this point if you'd like, because I don't have hard data; all I have is my general intuition built up over my paltry 21 years of being a professional programmer.
But, seriously, unit testing is
teh suck. System testing is teh suck. Structured testing in general is, let's sing it together, TEH SUCK.
"What?!!" you may ask, incredulously, even though you're reading this on an LCD screen and it can't possibly respond to you? "How can I possibly ship a bug-free program and thus make enough money to feed my tribe if I don't test my shiznit?"
The answer is, you can't. You should test. Test and test and test. But I've NEVER, EVER seen a structured test program that (a) didn't take like 100 man-hours of setup time, (b) didn't suck down a ton of engineering resources, and (c) actually found any particularly relevant bugs. Unit testing is a great way to pay a bunch of engineers to be bored out of their minds and find not much of anything. [I know -- one of my first jobs was writing unit test code for Lighthouse Design, for the now-president of Sun Microsystems.] You'd be MUCH, MUCH better offer hiring beta testers (or, better yet, offering bug bounties to the general public).
Let me be blunt: YOU NEED TO TEST YOUR DAMN PROGRAM. Run it. Use it. Try odd things. Whack keys. Add too many items. Paste in a 2MB text file. FIND OUT HOW IT FAILS. I'M YELLING BECAUSE THIS SHIT IS IMPORTANT.
Most programmers don't know how to test their own stuff, and so when they approach testing they approach it using their programming minds: "Oh, if I just write a program to do the testing for me, it'll save me tons of time and effort."
There's only three major flaws with this: (1) Essentially, to write a program that fully tests your program, you need to encapsulate all of your functionality in the test program, which means you're writing ALL THE CODE you wrote for the original program plus some more test stuff, (2) YOUR PROGRAM IS NOT GOING TO BE USED BY OTHER PROGRAMS, it's going to be used by people, and (3) It's actually provably impossible to test your program with every conceivable type of input programmatically, but if you test by hand you can change the input in ways that you, the programmer, know might be prone to error.
So, listen closely, because this is the method I've used since the dawn of time, and it works great:
1) When you modify your program, test it yourself. Your goal should be to break it, NOT to verify your code. That is, you should turn your huge intellect to "if I hated this code, how could I break it" as SOON as you get a working build, and you should document all the ways you break it. [Sure, maybe you don't want to bother fixing the bug where if you enter 20,000 lines of text into the "item description" your program gets slow. But you should test it, document that there is a problem, and then move on.] You KNOW that if you hated someone and it was your job to break their program, you could find some way to do it. Do it to every change you make.
When I test Delicious Library, I'm like, "Hey, to make sure my new queue works, I'm going to take a Library of 2,000 items and tell them all to reload themselves simultaneously." Now, honestly, in version 1.x, this is really slow. But, dang it, it works. And since our goal for version 1.x was to have it work with between 1K-2K items, I'm OK with this.
Too often I see engineers make a change, run the program quickly, try a single test case, see it not crash, and decide they are done. THAT IS TEH SUCK! You've got to TRY to break that shit! What happens when you add 10 more points to that line?
2) When you get the program working to the point where it does something and loads and saves data, find some people who love it and DO A BETA TEST. The beta test is often maligned, but the most stable programs I've ever written are stable because of beta testing. Essentially, beta testing is Nature's Way (TM) of making systems stable. You think nature creates unit tests and system tests every time it mutates a gene? Aw hell nah. It puts it out in the wild, and if it seems better it sticks around. If not, it's dead.
Now, if you're a pure-CS kind of person, you're thinking, "But, with beta testing, I might miss some really deep, conceptual bugs that unit testing would find, because there's only certain pathways people will test..." Well, there's really only one rational response to this:
WHO CARES!
Like in nature, any system is going to have bugs. And, again like nature, most of the time, in most situations, you want your system to work. But occasionally, under some circumstances, it could fail. This isn't great, but it is acceptable. The nice thing about beta testing is it lets you focus YOUR precious time as a programmer on the pathways that fail most often under normal use patterns, instead of just finding bugs at random that may or may not ever be triggered, and sucking up your time fixing them. (Hey, if you want to try to fix every bug in your program before you ship, be my guest, but at least fix them in order of how often they are reported, OK?) Unit testing doesn't give you information on how common a bug is, which is the single-most important piece of information about a bug, followed very closely by its severity.
Look, I know it's heretical to say "software is going to have flaws." But
everything is going to have flaws. Pilots screw up. Surgeons screw up. People die. Denying this doesn't make it go away, it makes it worse. We need to say not "never screw up," but "make sure when you screw up, you recover nicely."
Make sure that the flaws your program ships with are the least-common ones possible, given the time you have to fix bugs. Honestly, crashers are more important than, say, drawing bugs, but if 1,000 users report a drawing glitch, and one reports a crasher, which one should you fix? THE DRAWING BUG. DO THE MOST GOOD WITH THE TIME YOU HAVE. It's your duty on this earth, and it's also the fast route to success.
Now, all this is NOT an excuse to write crappy subroutines and say, "Well, it'll got caught in the beta test." You, as a programmer, should be programming EVERY LINE as defensively as possible. You should assume that every other method in the program is out to kill you, and they are intentionally passing you corrupted buffers and pointers, and it's your job to handle all that crap and make the best of it. (And, if you can repair it, do so, and if not, raise an exception so you can catch it in beta testing, instead of silently failing and/or corrupting data.)
Let's sum up: Write good code to start with. Write every routine assuming that it'll be called by chuckle-heads. When you modify your program, test it yourself, and TRY to get it to fail, don't try to get it to pass. Then set up a real beta program, with a good pathway for sending in feedback and categorizing it, and let the beta-testers do the rest.
Testing is hugely important. Much too important to trust to machines. Test your program with actual users who have actual data, and you'll get actual results.
--
November 20, 2005 followup: My comments here have been widely reported, and sometimes misconstrued because of my own failure to disclose the boundaries of when and why I don't use Unit Tests. I was thinking of writing a follow-up, but
bbum has done it for me, and I agree completely with what he says. So, please consider his post as the continuation of mine, only, you know, written by him.
Labels: code
Sunday, September 18
[October 29, 2007 - Note this post was written for 10.4. Under 10.5, our tests indicate JPEG2000 is now actually faster than JPEG to decode at any size; sometimes a TON faster. This is awesome news. Most of the following post is now just plain wrong, but I'm leaving it for posterity.]
--
As research for a project (-cough-
Delicious Library 2 -cough-) on which I'm slaving away, night and day, I did some timing tests to see how Apple's implementation of JPEG2000 performed, especially when creating a thumbnail image from a larger image (as compared to JPEG, which I've used a lot).
As background, I should mention that
Delicious Library (1.x) currently stores three separate JPEGs each cover image it downloads from Amazon, pre-rendered (with gloss) and downsampled into three different sizes (small, medium, large). (I store the downsampled images because anti-aliased downsampling turned out to be very expensive.) Every time we draw a cover, we actually draw directly from its appropriately-sized JPEG data, instead of keeping a cached image around, because caching the unpacked images was blowing up memory. (A library of 2,000 items with uncompressed covers would easily take up more than a 1GB of RAM, and we'd start thrashing like mad.) The real beauty to this scheme was that since the images on-disk were JPEGs and we were drawing directly from the data on disk (without changing it in any way), if we ran out of physical memory and some of our textures got paged out, they didn't have to be written to the swap space on the disk; Mach is smart enough to realize that if it has a page that it's mapped directly from a file on disk, there's no point in writing that page out to the swapfile; it can always read the page from the original file again when it needs to. Since writing to disk is the single-slowest thing a program can do, this is a HUGE speed win (if you've got more textures than you have memory).
However, keeping all those fiddly images (small, medium, and large) also increase our memory footprint, not to mention taking up a fair extra chunk of disk space. Wouldn't it be nice if we could just store ONE, full-size image, and then just decompress the first part of it for small images, and more for medium, and the whole thing for large images? (Progressive refinement, as it were.)
Well, as it happens, this is part of what
JPEG2000 was born to do (much like Kris and
"warming it up"). Not only is JPEG2000 more efficient than JPEG (eg, prettier files are smaller), but you can also create a beautiful low-res version of a JPEG2000 file by reading only its first few bytes, and as you read more you can construct larger and larger images, until, when you've read the last byte, you can reconstruct the full-size image. This would be really convenient with Mac OS X's mapped files ([NSData dataWithContentsOfMappedFile:]), because you can map in the entire JPEG2000 file safe with the knowledge that only the first couple of pages are going to be physically read off the disk if you just want a thumbnail (because mapped files lazily demand-page themselves into memory as they are accessed). (And, remember, disk access is the slowest thing your program can do, so less disk access is better.)
And, conveniently, Mac OS X 10.4 ("Tiger") has a new framework for reading and writing images, and it understands JPEG2000 natively (based on the commercial, C++
Kakadu library). And, in this new Apple framework, there's a function CGImageSourceCreateThumbnailAtIndex() where you can create a thumbnail of an image you haven't read in yet, by specifying a maximum side length (kCGImageSourceThumbnailMaxPixelSize). This would be EXACTLY the kind of call in which one would implement the partial-reading of JPEG2000s in order to quickly read in a low-rez versions of high-rez files.
At least, that's the theory. Because I'm a skeptic, I ran timing tests, and it turns out JPEG2000 is, well... you've already read the title of this post. It's slow. 10x slower than JPEG for a standard decode.
But, that's not all. It turns out that exactly the OPPOSITE of the optimization I thought would happen is happening; with JPEG files smaller thumbnails are actually MUCH faster to create than large ones, whereas with JPEG2000 files creating a thumbnail takes about the same amount of time no matter what size it is. That is, Apple has optimized their JPEG decoder to create thumbnails (and done a GREAT job), but they haven't done so with their JPEG2000 decoder (or they did, but did a REALLY poor job; I prefer to think they just haven't tried yet).
The timing tests below were done with a 1292x1319 RGB image. JPEG2000 and JPEG files were created at full resolution with varying levels of compression to try to be comparable in file size (final file sizes given below). Images were unpacked using CGImageSourceCreateThumbnailAtIndex() with the specified maximum side length (kCGImageSourceThumbnailMaxPixelSize) and then CFReleas()ed 100 times for each test.So, good news, bad news. The good news is, with the new 10.4 image reading framework (CGImageSource...), I can read JPEGs and make beautiful, anti-aliased smaller versions of them EXTREMELY quickly compared to 10.3. Under 10.3, I had to read in the whole JPEG, decompress the whole thing into a full-size buffer, create a small thumbnail buffer, and downsample the whole thing. Obviously, there are a lot of slow parts in that. 10.4's CGImageSourceCreateThumbnailAtIndex() is a godsend in this case -- I can store one large JPEG per book/DVD/whatever and use only the bit of the JPEG that I need for a given size.
The bad news is, right now, JPEG2000 is just not fast enough to use in real time. I've heard that, in general, it's just a slow format, so I'm not bashing Apple here; their implementation may be 40x faster than anyone else's, for all I know. But it's still 10-20x as slow as their JPEG decompression for thumbnails, and that's what matters to me in this case. (But, for example, if I were distributing an online game and were going to decompress my graphic assets only once, I'd use JPEG2000, since it's higher quality and smaller and has full alpha.)
I ran some various tests on JPEG2000s to see if the data I got was anomalous, but apparently it is not. One interesting note for me was that more compressed JPEG2000s don't decompress faster than less compressed JPEG2000s at the same image size. Who knew? Now
you do.
--
Followup (9/19): The news gets even better, as it turns out 10.4's CGImageSourceCreateThumbnailAtIndex() is 3-7x
faster at creating JPEG thumbnails than the method I had to use under 10.3: creating a CGImageRef using CGImageCreateWithJPEGDataProvider() and then drawing it (with antialiasing turned on) into a smaller window.
How often do you get to speed up your most time-critical portion of code by 3-7x? Go Apple!
Labels: code
Sunday, August 21
First off, I apologize for how long it's been since my last post. It's been a bit nutty around
Delicious Monster these last couple weeks, and I had to get a 1.5.2 release out as well. Also, my main machine lost its hard drive, which took two weeks to restore and repair. Also, I got the flu really bad. Also, I started dating again, which is one of those things you'd think I'd learn to stop doing, since by definition every relationship, no matter how short or how long, ends in pain.
Anyways, if you've read this series from the beginning, you'll know that, besides being perenially single, I also insist on returning early from functions/methods/what-have-you, instead of nesting ifs inside ifs inside ifs. To me, this makes the code much quicker to understand at a glance -- the normal path is the one down the left column, and any if statements I see are just dealing with error conditions. I demonstrate this in an earlier column, so I won't paste it again here.
Though I will say, again, that the goal of good coding is
to make your code apprehensible at a glance. That's
the goal,
the as in
only. I've spent a 25 years learning this rule, and I'll keep harping on it until St. Peter hands me a different harp to play. (Then, I plan to play "Stairway to Heaven".)
Now, several people have asked a thorny question, including one perceptive man named Tobias, who I will quote, in part, here:
I do have a problem with your solution though, and I can't figure out a way to solve it, so I thought I might ask you and see if you have an answer. What if you want some specific code that's executed when bailing? Or say, some code that should always be executed after some statements to clean up? I can't use return; then to bail, because I need that code to be executed before returning.
Ah, young Tobias. Condescending comment. Witticism!
Actually, to be honest, I don't have a really good solution to this one. I have solutions I use, depending on the situation, but none of them are particularly pleasing. I still like them better than nested-ifs, and I've usually found that when unrolling nested-ifs, there were lots of ways the code could have failed and not been handled correctly; that is, nesting ifs is actually not a particularly robust way to handle errors.
So, here are some possible situations, and possible solutions to them:
Situation 1 is if you've got a couple of lines of code you'de like to execute before you return from several different places, but you don't need to put the return statement itself in those lines; eg, the return is simple enough that it's not a hardship to copy and paste it. For this we use an inline function. It's a good fit because it's scoped right (won't be visible from anywhere but inside this method), and it inherits the scope above it, so you can use "self" and access your instance variables.
This particular snippet is from the code I use to recognize whether a keypress that's coming in to the program is part of a scanner's input or if it's being pressed by a human, since some scanners ("wedge-type") just send USB keypresses as if they were just another keyboard, and it's freaking hard to figure out where a keypress came from in AppKit. Oh, sure, they claim there's a way, but I will literally give $200 to the person who can show me how to take a keypress NSEvent and get the device that it came from out of it. No B.S., cash money. (Or an interview!)
|
inline void _sendAllEventsIncludingThisOne() {
[self _sendNormallyStoredNumericKeypressEvents:nil];
[super sendEvent:theEvent];
}
unsigned int modifierFlags = [theEvent modifierFlags];
// scanners never press the shift or command keys...
if ((modifierFlags & 0xffff0000) != 0) {
_sendAllEventsIncludingThisOne();
return;
}
unsigned int length = [[theEvent characters] length];
if (length != 1) { // WJS: pretty much all keydown and keyup events
have exactly one character these days
_sendAllEventsIncludingThisOne();
return;
}
|
Well, it's not pretty, but it does the job. One nice thing to note about this kind of approach is you can have different little "deconstructor" inline functions for dissassembling different pieces of a whole that you are assembling, so you could stack them intelligently if you get further along and fail, and thus require more unrolling actions. If you need to return some complex value when you bail, have your inline method return it, and then just call something like "return _sendAllEventsIncludingThisOne();"
In Situation 2 we see a situation where we have one exit point, and we pretty much always want to do the same thing in front of it.
|
{
autoreleasePool = [[NSAutoreleasePool alloc] init];
if (![self _drinkIsFrosty]) // this can allocate a lot of memory
goto bail;
if ([self _drinkType] != wantedDrinkType)
goto bail;
// ingest drink here
bail:
[autoreleasePool release];
}
|
Remember that this technique isn't a panacea: if you set up some other state in this method that needs to be unrolled, you're going to have to deal with it.
Here are some other real-life macros I've written. These could have easily been inline function, but I wrote them as macros because when I wrote them I wasn't sure if I was going to insert return statements into them; if you return from an inline function you just pop back to the method that had the error, but if you return from a macro you are actually returning from the containing method. This is sometimes a good thing, and sometimes a bad thing, and you have to decide which you want.
Note that these are the macros I use when dealing with QuickTime, so I can be notified if an error occurs and then decide if it's critical enough that I want to throw an exception. The problem with QuickTime's error model is there are a lot of errors that aren't critical and won't affect whether the program will continue correctly, so you have to sort your way through the values by trial and error at run time to figure out what errors you'll see in practice and what they mean. Oh, man, I sure DO NOT MISS this kind of programming. Bring on the Cocoa QuickTime!
|
#define NoteErr(x) { if ((x) != noErr) NSLog(@"QuickTime note error %d in file %s
on line %d", (x), __FILE__, __LINE__-1);}
#define PedanticErrorCheck() { OSErr _err = GetMoviesError(); if (_err != noErr) NSLog(@"QuickTime warning error %d in file %s
on line %d", _err, __FILE__, __LINE__-1);}
#define BailErr(x) { if (x != noErr) [NSException raise:[NSString stringWithFormat:@"%d",
x] format:@"Raised Carbon error %d in file %s on
line %d", x, __FILE__, __LINE__-1];}
...
{
...
sequenceGrabberComponent = OpenDefaultComponent(
SeqGrabComponentType, 0);
if (sequenceGrabberComponent == nil)
BailErr(-1);
err = SGInitialize(sequenceGrabberComponent);
BailErr(err);
// specify the destination data reference for a record
operation -- tell it we're not making a movie
// if the flag seqGrabDontMakeMovie is used, the sequence
grabber still calls your data function, but does not write
any data to the movie file
// writeType will always be set to seqGrabWriteAppend
err = SGSetDataRef(sequenceGrabberComponent, 0, 0,
seqGrabDontMakeMovie);
BailErr(err);
// create a new sequence grabber video channel
err = SGNewChannel(sequenceGrabberComponent,
VideoMediaType, &sequenceGrabberChannel);
BailErr(err);
...
}
|
As I said, this is work-in-progress. None of these techniques are super-beautiful, but when I compare my sequence-grabbing code to the samples on the websites, I certainly think this is a HUGE improvement. Especially since usually the examples just say, "/* if there's an error, you should handle it. */" Yah, thanks, guys. We CERTAINLY would NOT want an example of how to tear down these structures that are in a primordial state.
This brings me to a side rant. Seriously, if you're going to post sample code, SHOW HOW TO HANDLE ALL CONDITIONS! In the real world, windows resize. Users hit the wrong buttons. Cameras get unplugged. We need to know how to handle these things! There are still things that the iChat team can do with the iSight camera that I cannot replicate, and I think that stinks.
In summary: here's a situation where I'm actually INVITING you to flame me and post a better way of doing things. Considering the response on my last couple "pimp" articles, where I specifically told people not to, I'm expecting an avalanche of fun on this one.
Labels: code
Sunday, July 31
This week, I received mail from a padawan learner (aka, "youngling") who has written a tableView class which draws the cool gradient background behind selected cells, like you see in the left-hand column of Mail 1.0 or iPhoto or iTunes or the Finder.
Yes, some of you may be saying, "Well, if gradient backgrounds are the standard when you're selecting a new data source to drive other UI elements, why doesn't Apple just build them into NSTableView?" Hah! Impatient, you are. Much to learn, you have.
Sadly, the AppKit guys aren't that "big" on adding features just because every program that "ships" with the Mac implements them. For instance, you may also notice that in Mail when you drag a message to a mailbox (or in Finder when you drag a file onto the shelf-thing at the left) you get a nice, rounded-corner blue border around the selected row, whereas in YOUR Cocoa programs you get an ugly black square. No rounded-corners for you!
So, without much further ado, here is the NSTableView subclass, as submitted:
|
@implementation ACSourceListOutlineView
- (void)awakeFromNib
{
// source lists have different cell spacing
[self setIntercellSpacing:NSMakeSize(0.0, 1.0)];
// use custom gradiented text cell
[[self outlineTableColumn] setDataCell:
[[[ACSourceListTextCell alloc] init] autorelease]];
}
- (id)_highlightColorForCell:(NSCell *)cell
{
return nil;
}
- (void)highlightSelectionInClipRect:(NSRect)clipRect
{
// don't do anything when we're not selected
// this check may be a bit superfluous, but it's still a good idea
to keep it here -- the documentation doesn't say that when this
method is called, the outline view will have a row selected
// also notice that multiple selection is disallowed; we can safely
use [self selectedRow]
int selectedRow = [self selectedRow];
if(selectedRow == -1) return;
[self lockFocus];
// get the gradient image
// we should only draw a blue gradient when our view is active
NSImage *gradient;
if(([[self window] firstResponder] == self) && [[self window]
isMainWindow] && [[self window] isKeyWindow])
{
gradient = [NSImage imageNamed:@"highlight_blue.tiff"];
}
else
{
gradient = [NSImage imageNamed:@"highlight_grey.tiff"];
}
[gradient setScalesWhenResized:YES];
[gradient setFlipped:YES];
// get the rect we're drawing in
NSRect drawingRect = [self rectOfRow:selectedRow];
// get gradient size
NSSize gradientSize = [gradient size];
// get image rect
NSRect imageRect;
imageRect.origin = NSZeroPoint;
imageRect.size = gradientSize;
// draw
if(drawingRect.size.width != 0 && drawingRect.size.height != 0)
{
[gradient drawInRect:drawingRect
fromRect:imageRect
operation:NSCompositeSourceOver
fraction:1.];
}
[self unlockFocus];
}
@end
|
First off, note that there's some funny spacing going on here, which I strongly discourage. Don't try to line up your "=", and don't split up a single method onto multiple lines. I know it
seems easy to read the parameters, but, really, when you're looking at the code your biggest problem is getting the
gestalt of what's going on, not reading each parameter. When you glance at a method that's broken up
it
makes
reading
slower
like
this
because
we're
used
to
having
multiple words
on a
single line.
Annoying, isn't it? Don't write code like you're ee cummings. You aren't. (
That is the deepest secret nobody knows.)
The code itself is generally pretty reasonably written, except for other spacing-type nits, and the fact that it's not broken up by what is being subclassed, which makes it hard for newbies to figure out why certain methods exist at all. "Hey, why do we implement -_highlightColorForCell:? We never call it." Yes, but it's a private method that gets called by NSTableView, and if you don't override it you'll get drawing glitches when the NSTableView tries to highlight the cell AND your code draws its gradient highlight.
So, here's the code as I might rewrite it, knowing nothing about what it does. Note that I'm assuming line wrap is ON in your editor, and you have indent on wrap set to four spaces. The multi-line statements you see below are me simulating line wrapping (on a VERY narrow window!):
|
@implementation ACSourceListOutlineView
// NSObject (NSNibAwaking)
- (void)awakeFromNib
{
// source lists have different cell spacing
[self setIntercellSpacing:NSMakeSize(0.0, 1.0)];
// use custom gradiented text cell
[[self outlineTableColumn] setDataCell:
[[[ACSourceListTextCell alloc] init] autorelease]];
}
// NSTableView
- (void)highlightSelectionInClipRect:(NSRect)clipRect
{
// don't do anything when we're not selected
// this check may be a bit superfluous, but it's still a good idea
to keep it here -- the documentation doesn't say that when this
method is called, the outline view will have a row selected
// also notice that multiple selection is disallowed; we can safely
use [self selectedRow]
int selectedRow = [self selectedRow];
if (selectedRow == -1) return;
[self lockFocus]; {
// we should only draw a blue gradient when our view is active
NSImage *gradient;
if (([[self window] firstResponder] == self) && [[self window]
isMainWindow] && [[self window] isKeyWindow])
gradient = [NSImage imageNamed:@"highlight_blue.tiff"];
else
gradient = [NSImage imageNamed:@"highlight_grey.tiff"];
[gradient setScalesWhenResized:YES];
[gradient setFlipped:YES];
NSRect drawingRect = [self rectOfRow:selectedRow];
NSSize gradientSize = [gradient size];
// draw
if(!NSIsEmptyRect(drawingRect))
[gradient drawInRect:drawingRect fromRect:(NSRect){NSZeroPoint,
gradientSize} operation:NSCompositeSourceOver fraction:1.0];
} [self unlockFocus];
}
// NSTableView (private)
- (id)_highlightColorForCell:(NSCell *)cell
{
return nil;
}
@end
|
Ok, I changed some more stuff. I don't like extra braces around one-line if statements. I know, that way you can add extra lines any time you want and you don't have to think blah blah blah. But we're trying to make our code more READABLE. That's the key. You can spend 10 seconds adding braces if you end up writing another line. It's like you're slicing bread before every meal just in case you decide to make a sandwich, and then 90% of the time you just throw the bread away. Seriously, people, pretend that each line of code you write represents another tiny portion of W's shrunken soul that you are throwing away. We're talking about a precious commodity! Conserve it!
Note that I use NSIsEmptyRect() now. First off, it's almost always a bad idea to compare a float to zero using "myFloat == 0.0". Floats often have tiny round-off errors. Look it up in your C books. For example, is "(1.0 - 0.5 - 0.5) == 0.0" true? NOT NECESSARILY. You should use "myFloat < 0.0001" or some such. Or, in this case, use NSIsEmptyRect, which saves you like five words.
I got rid of the pedantic comments, but kept the pithy ones. Seriously, "get gradient size"? That's what "gradientSize = [gradient size]" means? Because, I'm thinking, if you can't figure that out, you not only can't write Objective-C, you can't read english.
Also, I toasted "imageRect" altogether. Not only did I save three lines (W's soul, people! His essence!), but, also, I closed off the question of "is imageRect going to be used in multiple places?" that the old code begged. And, as an added bonus, my code runs a TEENY BIT faster in some cases (left as exercise to reader to figure that out). I know I told you not to optimize while you code, but, honestly, if you can make smaller, clearer, AND faster code all at once... you go, girlfriend.
--
Now, many would be content to stop there, but as it happens I'm the author of one of the first gradient tableViews out there, and mine is open source, courtesy of The Omni Group. Please see their
source license on how this can be used:
|
@interface OAGradientTableView (Private)
- (void)_windowDidChangeKeyNotification:(NSNotification *)notification;
@end
// CoreGraphics gradient helpers
typedef struct {
float red1, green1, blue1, alpha1;
float red2, green2, blue2, alpha2;
} _twoColorsType;
void _linearColorBlendFunction(void *info, const float *in, float *out)
{
_twoColorsType *twoColors = info;
out[0] = (1.0 - *in) * twoColors->red1 + *in * twoColors->red2;
out[1] = (1.0 - *in) * twoColors->green1 + *in * twoColors->green2;
out[2] = (1.0 - *in) * twoColors->blue1 + *in * twoColors->blue2;
out[3] = (1.0 - *in) * twoColors->alpha1 + *in * twoColors->alpha2;
}
void _linearColorReleaseInfoFunction(void *info)
{
free(info);
}
static const CGFunctionCallbacks linearFunctionCallbacks = {0,
&_linearColorBlendFunction, &_linearColorReleaseInfoFunction};
@implementation OAGradientTableView
// NSObject
- (void)dealloc;
{
[[NSNotificationCenter defaultCenter] removeObserver:self];
[super dealloc];
}
// NSView
- (void)viewWillMoveToWindow:(NSWindow *)newWindow;
{
[[NSNotificationCenter defaultCenter] removeObserver:self
name:NSWindowDidResignKeyNotification object:nil];
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(_windowDidChangeKeyNotification:)
name:NSWindowDidResignKeyNotification object:newWindow];
[[NSNotificationCenter defaultCenter] removeObserver:self
name:NSWindowDidBecomeKeyNotification object:nil];
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(_windowDidChangeKeyNotification:)
name:NSWindowDidBecomeKeyNotification object:newWindow];
}
// NSTableView
- (void)highlightSelectionInClipRect:(NSRect)rect;
{
// Take the color apart
NSColor *alternateSelectedControlColor = [NSColor
alternateSelectedControlColor];
float hue, saturation, brightness, alpha;
[[alternateSelectedControlColor
colorUsingColorSpaceName:NSDeviceRGBColorSpace] getHue:&hue
saturation:&saturation brightness:&brightness alpha:&alpha];
// Create synthetic darker and lighter versions
NSColor *lighterColor = [NSColor colorWithDeviceHue:hue
saturation:MAX(0.0, saturation-.12) brightness:MIN(1.0,
brightness+0.30) alpha:alpha];
NSColor *darkerColor = [NSColor colorWithDeviceHue:hue
saturation:MIN(1.0, (saturation > .04) ? saturation+0.12 :
0.0) brightness:MAX(0.0, brightness-0.045) alpha:alpha];
// If this view isn't key, use the gray version of the dark color.
Note that this varies from the standard gray version that NSCell
returns as its highlightColorWithFrame: when the cell is not in a
key view, in that this is a lot darker. Mike and I think this is
justified for this kind of view -- if you're using the dark
selection color to show the selected status, it makes sense to
leave it dark.
NSResponder *firstResponder = [[self window] firstResponder];
if (![firstResponder isKindOfClass:[NSView class]] ||
![(NSView *)firstResponder isDescendantOf:self] || ![[self
window] isKeyWindow]) {
alternateSelectedControlColor = [[alternateSelectedControlColor
colorUsingColorSpaceName:NSDeviceWhiteColorSpace]
colorUsingColorSpaceName:NSDeviceRGBColorSpace];
lighterColor = [[lighterColor
colorUsingColorSpaceName:NSDeviceWhiteColorSpace]
colorUsingColorSpaceName:NSDeviceRGBColorSpace];
darkerColor = [[darkerColor
colorUsingColorSpaceName:NSDeviceWhiteColorSpace]
colorUsingColorSpaceName:NSDeviceRGBColorSpace];
}
// Set up the helper function for drawing washes
CGColorSpaceRef colorSpace = CGColorSpaceCreateDeviceRGB();
_twoColorsType *twoColors = malloc(sizeof(_twoColorsType)); // We
malloc() the helper data because we may draw this wash during
printing, in which case it won't necessarily be evaluated
immediately. We need for all the data the shading function needs
to draw to potentially outlive us.
[lighterColor getRed:&twoColors->red1 green:&twoColors->green1
blue:&twoColors->blue1 alpha:&twoColors->alpha1];
[darkerColor getRed:&twoColors->red2 green:&twoColors->green2
blue:&twoColors->blue2 alpha:&twoColors->alpha2];
static const float domainAndRange[8] = {0.0, 1.0, 0.0, 1.0, 0.0, 1.0,
0.0, 1.0};
CGFunctionRef linearBlendFunctionRef = CGFunctionCreate(twoColors, 1,
domainAndRange, 4, domainAndRange, &linearFunctionCallbacks);
NSIndexSet *selectedRowIndexes = [self selectedRowIndexes];
unsigned int rowIndex = [selectedRowIndexes
indexGreaterThanOrEqualToIndex:0];
while (rowIndex != NSNotFound) {
unsigned int endOfCurrentRunRowIndex, newRowIndex = rowIndex;
do {
endOfCurrentRunRowIndex = newRowIndex;
newRowIndex = [selectedRowIndexes
indexGreaterThanIndex:endOfCurrentRunRowIndex];
} while (newRowIndex == endOfCurrentRunRowIndex + 1);
NSRect rowRect = NSUnionRect([self rectOfRow:rowIndex],
[self rectOfRow:endOfCurrentRunRowIndex]);
NSRect topBar, washRect;
NSDivideRect(rowRect, &topBar, &washRect, 1.0, NSMinYEdge);
// Draw the top line of pixels of the selected row in the
alternateSelectedControlColor
[alternateSelectedControlColor set];
NSRectFill(topBar);
// Draw a soft wash underneath it
CGContextRef context = [[NSGraphicsContext currentContext]
graphicsPort];
CGContextSaveGState(context); {
CGContextClipToRect(context, (CGRect){{NSMinX(washRect),
NSMinY(washRect)}, {NSWidth(washRect),
NSHeight(washRect)}});
CGShadingRef cgShading = CGShadingCreateAxial(colorSpace,
CGPointMake(0, NSMinY(washRect)), CGPointMake(0,
NSMaxY(washRect)), linearBlendFunctionRef, NO, NO);
CGContextDrawShading(context, cgShading);
CGShadingRelease(cgShading);
} CGContextRestoreGState(context);
rowIndex = newRowIndex;
}
CGFunctionRelease(linearBlendFunctionRef);
CGColorSpaceRelease(colorSpace);
}
- (void)selectRow:(int)row byExtendingSelection:(BOOL)extend;
{
[super selectRow:row byExtendingSelection:extend];
[self setNeedsDisplay:YES]; // we display extra because we draw
multiple contiguous selected rows differently, so changing
one row's selection can change how others draw.
}
- (void)deselectRow:(int)row;
{
[super deselectRow:row];
[self setNeedsDisplay:YES]; // we display extra because we draw
multiple contiguous selected rows differently, so changing
one row's selection can change how others draw.
}
// NSTableView (Private)
- (id)_highlightColorForCell:(NSCell *)cell;
{
return nil;
}
@end
@implementation OAGradientTableView (Private)
- (void)_windowDidChangeKeyNotification:(NSNotification
*)notification;
{
[self setNeedsDisplay:YES];
}
@end
|
Ok, right off the bat you probably are saying, "I may be a padawan learner (or
youngling, as it were), but even
I can see that your rewritten code is more lines of code than the code you were tasked with pimping.
Ah, no patience, you have. Always Objective-C, code is not! The source code I was sent requires two external images. The source code re-imagined requires NO external resources. (And it runs faster, has a smaller memory footprint, and prints faster, but, again, we're not thinking of speed yet, right?) Those external resources need to be maintained. They need to be installed in the right place. And, most importantly, they need to be
understood. If you don't know what they look like, you have to pull them up in your editor, which means interrupting your flow.
Which leads me to an important rule:
In general, if I can replace an image with code, I do so. This is not something that's intuitive, and it took me many years to decide that this is the best policy. Code is easier to change and understand than images are. And having fewer files in your project is EVEN BETTER than having fewer lines of code in a single file. Think of each file as a project-complexity multiplier, and each line of code as a project-complexity adder. Every file in the project has to be understood. (The most confusing projects I've seen are the ones that have 400 files, each of which has like one or two routines. That way leads madness.)
Also, note that the images are static -- they come in two colors, blue and grey. My code actually looks at the highlight color you set in preferences, and generates a dark and light version of that color. So, if you like to highlight in pink, my tableView draws in two shades of it. In contrast, the Finder and iPhoto and iTunes all use blue, even if your highlight color is yellow. Whaaaa? How is that clear?
Also note my tableView registers for notifications when the window becomes and loses "key" state, which is when your highlighted items are supposed to switch to grey instead of colored. The code I was given just
hopes that the tableView will be asked to redraw itself in this case, which is often the case but
not always. My code also checks to see if the firstResponder is a
subview of this tableView, so if you're editing text inside one of the cells of this tableView, mine will still draw the colorful gradient (his goes gray in this case).
And, mine does a really cool effect where it does a gentle was across ALL the selected cells in a tableView,
even if they are discontiguous. He documents that his won't work in this case (which is a fine way to save on coding; the first version of mine did the same thing), but it's cooler to handle multiples, because forcing single selection is almost always teh suck.
Finally, I just think it's cooler to use CG to do gradients (they call them "shadings") than to use images. For one thing, it's hella fast (then again, drawing a stretched image is pretty fast, too). More importantly, it's resolution-independent, which will be a Big Deal in 10.5 ("Liger"). But even right now, you could print my tableView on a super-hi-res printer and the gradients would look boo-ty-full (and the PDF files would be small).
My tableView is actually pretty old code, and I didn't want to modify it right before publishing it, because that's like adding "one last feature" before doing a major release. (Eg, invitation to disaster.) Can you make this code more efficient? Maybe there are newer APIs on CGShading which require less crap... I wrote this about three years ago, and I was young and foolish, then.
Now, I'm just old and foolish.
Youngling.Labels: code
Wednesday, July 20
Ok, much as these things tend to, my last post caused a flurry of controversy amongst a select few, who felt that the more traditional method of writing -init methods, eg:
|
- (id)init;
{
if ((self = [super init]) == nil)
return nil;
[...initialize my stuff...]
return self;
}
|
Is somehow better than my recommended:
|
- (id)init;
{
if (![super init])
return nil;
[...initialize my stuff...]
return self;
}
|
I kept posting reasons why my way was valid, and posters kept arguing, so I threw down the gauntlet and offered $20 to
anyone who could come up with
one Cocoa class that you can subclass and which won't be initialized correctly unless you use "self = [super init]" in your subclass's -init method.
And, this week, we have a winner! Well, sort of. We have someone who did such a good job researching this that I agreed to give him the money, even though he didn't disprove my point, although he
did point out some interesting gotchas in Cocoa.
Ken Ferry was intrepid enough to write a program that
dynamically runs through every class and subclasses them, instantiates the subclasses, and sees if [super init] doesn't return self.
Which, in fact, several classes don't always do. Mr. Ferry mentions that NSColorPanel, NSFontManager, and NSDocumentController all only have a single instance, so the second time you call -init on them, you get the first object again.
However, this wasn't actually the question. Because if you allocate a second NSColorPanel,
you've done wrong. Your code is not correct. FURTHER, if you just blithely go on with your init method after the superclass has returned a different object, you are likely to leak objects at best and crash at worst. That is, imagine I have the following code:
|
- (id)init;
{
if ((self = [super init]) == nil)
return nil;
myColorList = [[isa _generateColorList] retain];
return self;
}
|
Well, the second time you init your subclass of NSColorPanel, you're going to overwrite the
original value for myColorList with a new value, and leak the original value. You can probably imagine how to have even worse effects.
What you need to do with these classes is
detect that you've had a different value returned, and then handle that case. You can't just blithely reset 'self.' I would recommend raising an exception if you've attempted to create two of a unique class, so you can catch this and fix your dang code, but if you're subclassing a class where Apple's machinery requires their (dubious) silently-replace-with-unique-instance behavior, at least you should do something like:
|
- (id)init;
{
id superInitReturn = [super init];
if (!superInitReturn || self != superInitReturn)
return nil;
myColorList = [[isa _generateColorList] retain];
return self;
}
|
Mr. Ferry also pointed out that NSIndexPath uniquefies paths, so in this example:
|
unsigned indexes[3] = {1,2,3};
NSIndexPath *indexPath1 = [[NSIndexPath alloc] initWithIndexes:indexes length:3];
NSIndexPath *indexPath2 = [[NSIndexPath alloc] initWithIndexes:indexes length:3];
|
indexPath1 and indexPath2 are the exact same pointer. However, this is essentially the same case as above — you do
not necessarily want to re-run your -init method on the same object twice, so you have to detect if you've been uniquefied after a [super init],
not simply re-assign self.
So, my original statement was, "If you write code that says, 'self = [super init]', YOU DONE WRONG." This still stands. Mr. Ferry showed us some cases where you have to be aware of what your superclass is doing, and you have to handle some classes in a special way. And these cases underscore the point I made in the discussion after my last blog post: don't write your subclasses without
knowing how your superclass works. Don't pretend you can write some platonic ideal class whose superclass doesn't matter. It
does matter. It effects what valid variables can be named. It effects what method names you can use. And it effects how (and even if) you'll write your - init method. Heck, sometimes you don't even want to subclass - init; you'll want to call the designated initializer for the superclass, which by convention is usually the
longest - init... method, and - init is, by definition, always the shortest.
Note: Mr. Ferry asked if he could exchange his cash prize (which totalled $100, since I said I'd give $20 for each instance found) for a job interview at Delicious Monster, which I will happily give him.Update April, 2009:There's been hints from Apple that they might modify the standard -[NSObject init] method to try to re-use old object's memory, since it turns out that a very common usage pattern is for programs to keep creating and deallocating, say, 12 objects of the same class, over and over. Re-using the exact same memory ends up being a big win (and this is a trick the iPhone already does with its UITableViewCell class, and that is a HUGE win if you do it yourself on the iPhone).
So, from now on, I recommend everyone uses:
|
- (id)init;
{
if (!(self = [super init]))
return nil;
// other stuff
return self;
}
|
I do.
Labels: code
Wednesday, July 6
Our first "lucky" contestant is up! He's sent me a class whose instances apparently represent files on the disk, possibly to be put into a tree to mirror parts of the filesystem. Then, STUFF can be done to them. None of this really matters for our porpoises (Eeee! Eee!), we're just looking at the
way he wrote the code.
We otter dive right in (as a porpoise would):
|
@interface FileSystemNode (PrivateMethods)
- (BDAlias *) nodeAlias;
- (void) setNodeAlias:(BDAlias *)newVal;
// Keep track of the node mod date
- (void) setNodeModifiedDate:(NSDate*)newDate;
- (NSDate*) nodeModifiedDate;
// Keep track of where the alias last pointed to...
- (BOOL) setNodePath:(NSString*)newVal;
@end; |
|
|
@interface FileSystemNode (Private)
- (BDAlias *)_nodeAlias;
- (void)_setNodeAlias:(BDAlias *)newNodeAlias;
- (NSDate *)_nodeModifiedDate;
- (void)_setNodeModifiedDate:(NSDate *)newModifiedDate;
- (BOOL)_setNodePath:(NSString *)newNodePath;
@end; |
Note first that "PrivateMethods" category name becomes "Private". Obviously, these are "methods" — what else would they be? Every word counts.
All private methods gained underscore prefixes. This convention is
really handy for telling, at a glance, whether you're calling a public method or not on your code. It aids in deleting methods in a timely manner — whenever you delete a place where you call a method that starts with an underscore, you search for that method in the same file, and if you don't invoke it then you can delete the whole method, as well. Yay! Less code.
The accessors were rearranged so that they are in a consistent order: access, set, access, set, etc. This is important; the brain loves patterns. Redundant comments were removed. I know what -_setNodeModifiedDate: is going to do just by reading the (well-written) method name. Extra text in front of it is HARDER for me to read. Assume I speak code better than I speak English. Assume that if I'm thinking in code terms I don't want to have to keep switching my English parser on and off. Assume I'm familiar with the concept of "accessor" methods if I've programmed Cocoa at all, ever.
If there are side-effects in your set/get accessor methods, document them in the method. If the side-effects are really severe, the accessor method should be retitled as well. Almost never should you have to comment your method definitions; if the titles alone aren't clear enough, YOU ARE ALREADY WRONG. (In this case, they are.) For example, if the method were to, say, set the node path AND remove the old file, then it should be called something like -_removeOldFileAndSetNodePath:.
Finally, spacing was changed to be consistent with my exacting standards (no spaces after braces except in an "if ()" statement) and method parameter names we changed to be more descriptive.
|
+ (id)nodeWithPath:(NSString *)aPath
{
FileSystemNode* newNode = [[[FileSystemNode alloc] initWithPath:aPath] autorelease];
return newNode;
}
|
|
|
+ (id)nodeWithPath:(NSString *)aPath
{
return [[[FileSystemNode alloc] initWithPath:aPath] autorelease];
}
|
Don't allocate a variable you only use once — every time I see a variable, I expect it to, you know...
vary. More lines is
worse, not better. I want to be able to glance at the routine and know what it does.
|
- (id)initWithPath:(NSString *)aPath
{
// We don't support symbolic links because aliases made to them get pointed at the original, not the
// point of the symbolic... this messes up all our hierarchy management with alias resolution, etc.
if ([[NSFileManager defaultManager] pathContentOfSymbolicLinkAtPath:aPath] == nil)
{
if (self = [super init])
{
// Set the initial path (this creates an FSRef, records mod date, etc)
if ([self setNodePath:aPath] == NO)
{
// NSLog(@"FileSystemNode creation failed with path %@.", aPath);
[self release];
self = nil;
}
}
}
else
{
[self release];
self = nil;
}
return self;
} |
|
|
- (id)initWithPath:(NSString *)aPath
{
if (![super init])
return nil;
// We don't support symbolic links because aliases made to them get pointed at the original, not the
point of the symbolic... this messes up all our hierarchy management with alias resolution, etc.
if ([[NSFileManager defaultManager] pathContentOfSymbolicLinkAtPath:aPath] != nil) {
[self release];
return nil;
}
// Set the initial path (this creates an FSRef, records mod date, etc)
if ([self _setNodePath:aPath] == NO) {
[self release];
return nil;
}
return self;
}
|
Multi-line comments should be collapsed to just one line; let your editor wrap lines for you, it's good at it. There is no magical 80-column limit! If my editor window is narrower than yours, I'll see double-wrapping (as in the left column, above) if you try to manually wrap lines. [Set Xcode to wrap and indent lines in its preferences.]
Don't leave dead logs in your code. Rewrite 'em if you need them again. They junk things up and make code hard to read. Also, they make me think you're not really sure if your code works or not.
Don't indent and indent and indent for the main flow of the method. This is huge. Most people learn the exact opposite way from what's really proper — they test for a correct condition, and if it's true, they continue with the real code inside the "if".
What you should really do is write "if" statements that check for
improper conditions, and if you find them, bail. This cleans your code immensely, in two important ways: (a) the main, normal execution path is all at the top level, so if the programmer is just trying to get a feel for the routine, all she needs to read is the top level statements, instead of trying to trace through indention levels figuring out what the "normal" case is, and (b) it puts the "bail" code right next to the correctness check, which is good because the "bail" code is usually very short and belongs with the correctness check.
When you plan out a method in your head, you're thinking, "I should do blank, and if blank fails I bail, but if not I go on to do foo, and if foo fails I should bail, but if not i should do bar, and if that fails I should bail, otherwise I succeed," but the way most people write it is, "I should do blank, and if that's good I should do foo, and if that's good I should do do bar, but if blank was bad I should bail, and if foo was bad I should bail, and if bar was bad I should bail, otherwise I succeed." You've spread your thinking out: why are we mentioning blank again after we went on to foo and bar? We're SO DONE with blank. It's SO two statements ago.
Don't assign "self = [super init]" in your init statements, ever. If you ever type "self =" YOU'VE DONE WRONG. I know there's some book out there that says to do it. It's wrong as well, so there. 'self' is assigned by the Obj-C machinery when -init is called. Re-assigning it should really be a compiler error, but it wasn't made so because back in 1989, when we started all this, we were using +new instead of +alloc, -init, and +new both allocated the memory AND initialized it, and so we assigned 'self' by hand in our +new methods. Nowadays, 'self' is set for you, and it's ugly and potentially hazardous and redundant to set it again.'
Don't do anything before calling "[super init]" unless you have a really, REALLY good reason. In this case, the tiny optimization of the unlikely case of the file being a symlink is not a good enough reason, because [super init] is SO cheap compared to everything else that it really doesn't matter. I expect the first two lines and the last line of every -init... method to be the same unless something REALLY different is going on. Don't disappoint me. It makes kittens cry.
I do like the way the coder calls his own -_setNodePath: to set the path instead of writing the code AGAIN in -init. Kudos.
|
- (void) setNodeAlias:(BDAlias *)newVal
{
[newVal retain];
[_nodeAlias autorelease];
_nodeAlias = newVal;
}
[... other methods were here ...]
- (BDAlias *) nodeAlias
{
return _nodeAlias;
} |
|
|
@implementation FileSystemNode (Private)
- (BDAlias *)_nodeAlias;
{
return _nodeAlias;
}
- (void)_setNodeAlias:(BDAlias *)newNodeAlias;
{
if (_nodeAlias == newNodeAlias)
return;
[_nodeAlias release];
_nodeAlias = [newNodeAlias retain];
}
@end |
If you declare your "Private" methods to be in a category, put 'em in a category! In that way, if you miss some, you'll get a nice compiler warning. Don't mix 'em in with your public methods! That's just ugly.
End the definition lines on your method implementations with a semicolon, so you can copy-n-paste them to or from your header (or the "Private" category at the top of your file) as needed when they change. Semicolons are required in the "interface" section, but don't hurt anything in the "implementation" section.
Group accessors together. -_nodeAlias should be right next to -_setNodeAlias:. Purr.
Ok, now some people are going to call me a hypocrite here, because my _setNodeAlias: method is ONE LINE LONGER than his. Well, it's the exception that makes the rule, smart guy. In this case, I happen to know three things:
(1) These -_set...: methods tend to be the primitives upon which everything else is based, and so they can get called a LOT. It's not uncommon to have tens of thousands called for a single user action in real applications. Now, if you use the old (and once-recommended) -autorelease, -retain technique for -set...: methods, you will end up with TENS OF THOUSANDS of dead objects sitting in your autorelease pool. Remember that all these objects are using up memory, and it won't be freed until the autorelease pool is popped, which is at the end of the current event (unless you've made your own, inner pool, which is often a good idea inside big operations). Remember also that allocating and releasing memory is expensive.
Now, if you do it my way, these objects never hit the autorelease pool. They're deallocated immediately, so their memory can be re-used immediately. This keeps your stack size nice and small, and you expand your VM like a blowfish with a water allergy.
(2) These -_set...: methods often register their undo contrapositives, as they otter. Your typical -set...: method should look like this:
|
- (void)_setNodeAlias:(BDAlias *)newNodeAlias;
{
if (_nodeAlias == newNodeAlias)
return;
[[[self undoManager] prepareWithInvocationTarget:self] _setNodeAlias:_nodeAlias];
[_nodeAlias release];
_nodeAlias = [newNodeAlias retain];
} |
Now, it doesn't take a genius to see that it's going to be a TON more efficient to only register undo events when the 'newNodeAlias' is actually different.
(3) -set...: methods often have side-effects, and those can be expensive. See point #2.
Ok, you may be saying, "How often am I really going to call -set...: with the value it already has?" The answer is: a lot. It happens a lot. There are lots of reasons for this, but it boils down to this simple rule: it happens a whole lot in code. This is a good place to nip off this redundancy — don't try to handle it at a higher or lower level. Handle it right here.
Note that the -autorelease, -retain way of doing -set...: methods was once recommended by Apple (or NeXT — I can't remember), and may still be recommended by books. Ignore that crap.
|
- (NSComparisonResult) sortByUserVisibleName:(id)otherObject
{
// Just compare the user visible names
return [[self userVisibleName] caseInsensitiveCompare:[otherObject userVisibleName]];
}
|
|
|
- (NSComparisonResult)compareUserVisibleNameToObject:(FileSystemNode *)otherFileSystemNode;
{
NSString *ourName = [self userVisibleName];
NSString *otherName = [otherObject userVisibleName];
if (ourName != nil && otherName != nil)
return [ourName caseInsensitiveCompare:otherName];
else if (ourName == nil && otherName == nil)
return NSOrderedSame;
else if (ourName == nil)
return NSOrderedAscending;
else
return NSOrderedDescending;
}
|
Note the method name change. This method doesn't sort! It compares. Name it to match other -compare...: methods, like, say, NSString's -compare:.
I deleted the comment. Seriously, I can figure out this method compares the visible names. Especially now that I named the method "compareVisibleNameToObject:". My
mom could figure that one out.
I handle the case for -userVisibleName being 'nil' explicitly, because it's much safer — -caseInsensitiveCompare: would otherwise raise an exception if 'otherName' is nil, which is essentially like crashing, and if 'ourName' were nil it would return NSOrderedSame no matter what 'otherName' was. Note that if you
know that -userVisibleName can NEVER return 'nil' (like, it'd be a program error if it did so) then his method was really written correctly; I just wrote mine to show what you'd do if there's a possibility you'll get a 'nil'. Note that if you can't
ever have a nil value for the userVisibleName and it doesn't change, I urge you to
not have a -setUserVisibleName: method, and instead pass it as an argument to your -init...: method, so there's no way to create one of these objects without a name.
I do like that the programmer called his own accessor here instead of accessing his ivar directly. My rule is, if you have an accessor method, it's good to use it internally in the class, because there may be side effects in the accessor method. (If you don't have an accessor method, I recommend against using it, because your app will not compile or run.)
--
Now, as an exercise to the reader, I'll paste in another method from this file, which you should now have the power to write correctly. Go for it! Make it beautiful! You can do it!
|
- (NSString*) nodePathResolvingAliases
{
// Assume we will get the path of the base FSRef
NSString* finalPath = nil;
// If it's not a dir, it might be an alias
if (_isDir == NO)
{
BOOL isAlias;
BOOL isDir;
if ((FSIsAliasFile(&_nodeFSRef, (Boolean *)&isAlias, (Boolean *)&isDir) == noErr)
&& (isAlias == YES))
{
BOOL isAliasToDir;
FSRef thisFileRef = _nodeFSRef;
// ¥[Initials]¥ should cache alias resolution?
if (FSResolveAliasFileWithMountFlags(&thisFileRef, YES, (Boolean *)&isAliasToDir,
(Boolean *)&isAlias, kResolveAliasFileNoUI) == noErr)
{
UInt8 pathChars[PATH_MAX];
if (FSRefMakePath(&thisFileRef, pathChars, PATH_MAX) == noErr)
{
finalPath = [[NSString stringWithCString:(const char *)pathChars]
stringByStandardizingPath];
}
}
}
}
// If we didn't get an alias resolution, just use the base path
if (finalPath == nil)
{
finalPath = [self nodePath];
}
return finalPath;
} |
Special thanks to my guinea pig this time, who was first to send me code. Thanks for being a good sport and not coming after me with a hunting rifle. I thought there was a lot to like in this code; in particular the author clearly was following
some style guidelines, they just weren't mine. Still, big ups for trying to be clean.
Labels: code
Tuesday, July 5
I'm mentoring an innocent man who I met at WWDC (we call him Dean Cain) in the finer points of programming Cocoa (he's an ex-Java guy), and he suggested that my mentoring might serve a larger audience. I thought about it, and realized (a) I like talking about how right I am, and (b) I like insulting people, so it seemed like a natural fit.
So, I thought I'd try an experiment. I don't know if this'll work, but, hey, nothing ventured, nothing chopped off and sold on the black market in New Guinea.
Send me a snippet of your code Objective-C Cocoa code -- from a single method to an entire file -- something that works, at least sort of, but maybe isn't as clean as you like. I will mercilessly tear it apart in my blog, and from this you will learn my style.
Obviously, if you don't have any faith in my style, this offer holds no allure for you, and I urge you to move on. I'm going to spend zero time justifying why you might want to listen to me, so if there's nobody out there that wants to learn things my way already, then this will be a very short experiment.
This isn't about me debugging your code, this is about me trying to teach the techniques of writing readable, beautiful, maintainable, minimal code, such as I've learned them.
I must warn you that it may seem like I'm kind of mean, because I don't say a lot of "this is interesting, but maybe..." It'll be more like, "NO! Don't do this! DO THIS! BECAUSE I AM THE MOMMY!" (Let me know if you want me to use your name or not. By default, I will not, but if you seek a perverse kind of fame, let me know.)
The e-mail address I'll use for this is my initials (William J Shipley) at Apple's mail destination -- mac.com.
Labels: code
Friday, February 25
Sometimes people say to me, "Hey, Wil, you've been programming since dinosaurs roamed the earth... do you have any advice for young whippersnappers like us?"
And I always respond, "Hey, you kids, GET THE HECK OUT OF MY YARD!"
No, no, I usually demur with, "Oh, gosh, I don't know," as I look down shyly and shuffle my feet.
But, I've thought about it a lot recently, after writing so much solo code for Delicious Library (for the first time in many years), and then taking on a new programmer and trying to impart my style on him. And what I've come up with is a style I call:
* The Way of the Code Samurai *
Now, I don't actually know much about real samurai, but the basic thing I've heard is they stand and stare at each other for hours, and then suddenly BAM strike once and the other guy is down.
That's how you should code.
- Think first. Think some more. Think about the whole problem. Think about a little part of the problem you're going to start with. Think about the whole thing again, in relation to your idea on the starting point.
Don't write code until you know what you're doing. Now, you may not be able to "know what you are doing" just from thinking, in which case you should start a TEST project and write a bunch of ugly code to make sure your ideas are correct.
Then, start again, in a new directory. We had seven or so different test project directories during the making of Delicious Library -- one for the video barcode reader, one for the store, one for amazon XML parsing, one for talking to Bluetooth scanners, etc. We learned how to do what we were going to do BEFORE we uglied up the main project with a bunch of code.
Then we copied the good parts out of the test project, and left the cruft. This let us observe rule #2...
- Write all your code "clean," the first time you write it. Make classes for everything. Use enumerated types. Don't take shortcuts. Don't have any part of the code where you say, "Oh, yah, I just glossed over that for now." You are NOT going to go back and fix it. Seriously, how often do you say to yourself, "I think I'll dive into this messy code today and try to make it nice and pretty without adding any functionality?" Nobody is going to pay you for that. In fact, I got called on the carpet for cleaning code during a major update to a piece of software at a previous job -- "What are you doing spending time modifying code that already works? Just add your new features and be done." Never mind that I couldn't understand the code, or that clean code is stable, maintainable, extensible code.
Don't gloss over anything. Write every line to be bulletproof. Write every method as if every other method was out to get your code and try to make it crash, and your job was to make sure it wasn't going to happen in YOUR code. Assume the user is going to hit keys randomly. Assume the network is going to get disconnected. Assume the hardware will fail.
Explain your routines aloud. Do you find yourself saying, "Well, this isn't totally correct, but it works because I know that at this point..." STOP. Redo it. Write it the correct way.
And, if you're in code anyways to extend it or fix a bug, CLEAN IT. Clean as you go, always. Don't consider code to be static. Turn those constants into an enumerated type like you always meant. Take those hard-coded strings and make class variables for them. Replace english phrases with NSLocalized versions. Clean, clean, clean as you go.
- Less source code is better. This is almost always true. The exceptions to this are so rare that every time you think you've found one you should REALLY doubt yourself. Less lines of source code almost always means less code that new programmers have to understand when they come on the project. It means less stuff for you to remember next year when you are in the middle of another version. It means fewer places for you to have mistyped something. It means fewer instructions to execute. It means fewer things to change when you re-architect.
There are some interesting corollaries here. For instance, if you're writing a class to display some text in red (for some reason), don't add a bunch of methods "for the future" that allow you to draw the text in blue or green or purple. Because that's more code than you need right now, and "less code is better."
But, if you suddenly realize you want to draw purple text, you could write the red code again, except put in the color purple. But, "less code is better," so you really need to abstract out the text-drawing code and make it take a color parameter, so you can re-use the same code.
The lesson I'm getting at is, don't try to make code general until you actually need it in more than one place. The worst libraries in the world are the ones people write without actually writing any code that uses them to do actual work for actual users.
And don't write longer, more obtuse code because you think it's faster. Remember, hardware gets faster. MUCH faster, every year. But code has to be maintained by programmers, and there's a shortage of good programmers out there. So, if I write a program that's incredibly maintainable and extensible and it's a bit too slow, next year I'm going have a huge hit on my hands. And the year after that, and the year after that.
If you write code with more code that's fast now, you're going to have a hit on your hands. And next year, you're going to have a giant mess to maintain, and it's going to slow you down adding features and fixing bugs, and someone's going to come along and eat your lunch.
I'm not saying slow code is good. There's a time and a place for optimizations...
- The time and place are AFTER YOU ARE DONE. Optimize methods ONLY after they work and are bulletproof AND you've done testing in Shark and discovered the method is critical.
Don't optimize as you write. Why not? Because, in all probability, you're wasting your time. The VAST majority of the code programmers optimize is executed so rarely that its total speedup to the program can be measured only in nanoseconds. Also, you're probably not very good at it. No insult, but machines and libraries are complex systems, and your guesses as to what's slow are probably pretty bad. You need to run testing tools to find out what is ACTUALLY slow. The results are often surprising. (For instance, in the Mac OS 10.3, having lots of tooltips in a view that you remove and add back to a window a lot is EXTREMELY slow. This is not something you can possibly know to optimize without doing testing in Shark.)
The next time you go to make something a little harder to read but a little faster, ask yourself, "How often will this REALLY get called?" Is the answer less than a hundred times a second? Because processors can now process several BILLION operations a second.
Now, Delicious Library isn't the zippiest program in the world on all hardware, but, actually, it's a LOT faster than it was initially. Those huge, beautiful covers really suck down memory. When we first wrote Library, if you loaded more than about 400 items, the cover images would suck up all your main memory and the program would just crawl. (It was like iPhoto 1.)
I re-architected the entire cover caching and compositing system, and got it so that we could comfortably handle several thousand items, and, if you use the table mode, possibly tens of thousands (I've never had that many items). This took several weeks to do. Now, I'm not as zippy as iPhoto 4, but I'm actually pretty close to the performance of iPhoto 2, and considering I'm just one guy and this was Delicious Library 1, I'm pretty proud of that.
What's the point? The point is, if I'd spent a bunch of time optimizing other parts of the program as I wrote it, I would not have had those weeks at the end to fix the imaging path, which was the slowest part. I would have had to have shipped with a program that could only handle a couple hundred items, and then I would have immediately had to patch it when people started scanning in thousands.
--
[Parlez-vous Français?]Labels: code
Delicious Monster
