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, July 1
So, my WWDC slides have been downloaded 26,000 times since I put them up a week ago. Yipes!
A number of people asked me to post a podcast of the talk, as well, so last night I stayed up for an extra hour and a half and tried to replicate what I said when I originally presented the slides, so if you're a glutton for punishment you can now have an extra sense filled with Wil goodness. [This is actually an expanded version of the talk, since I had lots of time.]
Here's a smaller version of the slides (same content, exactly) that a nice reader sent me, to save my bandwidth:
Slides of the talkAnd here's the exciting podcast:
Podcast, to go with the slidesLabels: business, stories
Delicious Monster
