Saturday, December 24

Pimp My Code, Part 6: The Pimp Before Christmas

Matt W. submitted this stopwatch display class for pimping:

Original CTStopwatchView header

/* 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?

PIMPED CTStopwatchView header

// 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:

Original CTStopwatchView class

#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.

PIMPED CTStopwatchView class

#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

39 Comments:

JKP said...

Will -

In the past I've taken on board many of your comments because they just make sense (I'm a particular fan of early exit conditions rather than huge nested if statements etc...). But looking at how you approached this I cant help but feel its a little short sighted.

I recently had to optimise several cells in my application because they were far too slow when I was using them in drag and drop operations. Profiling with shark showed that they spend a significant amount of time within their drawing methods calculating layout etc. Things like sizeWithAttributes are extremely costly, and if you rely on these for positioning other elements you _are_ going to see a performace hit.

I changed my (cell and view) implementations so that they cached the last size used (something you said you hated), and called into a seperate method to calculate the layout required for each element. This was a huge speed booster for me since if you are just dragging over a cell and its redrawing god knows how many times a second, it is _not_ changing size, and in reality, it is changing nothing from the last time drawRect: was called. I went from a pretty unusable set of cells / views to classes that are speedy and efficient.

Also, caching images - sometimes this too is useful as scaling images can be a costly operation...where do you see this benefit being negated?

Lastly, I'm interested in your take on caching fonts too - whilst I dont cache them as ivars, I do hold fonts as static class variables, simply because a: it saves repeating the same code to manipulate the font via NSFontManager to get it to the state you need it at, and b: each and every instance of your view is likely to need to share the same font configurations.

So much as I too _hate_ ivars i cant see another more efficient way to go about these things. I was looking forward to learning something new here in your latest article, but I have to say that you just seem to have gone against all conventional wisdom (see the Apple docs about improving the efficiency of drawRect:). I'd love to hear your thoughts on what i have said, as i say, I hate excessive ivars, and if I can find a better way I always will, but as of yet in the case of improving view efficiency I haven't found one.

Jamie

December 24, 2005 7:27 AM

Anonymous said...

Why the difference in how you define your two constants in the method
- (void)drawRect:(NSRect)rect:

#define FONT_SIZE_RATIO (1.2)

const float baseFontSize = 30.0;

Is there a particular benefit or reason for one style vs the other?

December 24, 2005 7:29 AM

Jonathan said...

Willy-nilly, I hope that posting to Blogger messed up some of the formatting, as I noticed in the implementation file that there were some things that weren't tabbed and neat.

December 24, 2005 9:35 AM

Seth Willits said...

Do you always add comments to label groups of methods as coming from different protocols or classes? I've never seen this done before, though it kinda sorta looks like a good idea. :)

December 24, 2005 9:53 AM

Anonymous said...

>const float baseFontSize = 30.0;
>
>
>Is there a particular benefit or >reason for one style vs the other?

In my experience, the typed declaration is preferred because, well, it's typed. Also, some debuggers will let you change constants like that during runtime. Which is invaluable for "tweaky" constants.

screw The Man! http://www.screwtheman.com

December 24, 2005 1:55 PM

Wil Shipley said...

Jamie:

You make some very valid points, but I think your situation is different than this one.

There's a time and a place for optimization. The time and place is after your code is beautiful. You took the right approach with your code; write it, then profile, then optimize just the slow parts. It seemed clear to me that this code had been "pre-optimized," which I ripped out.

Looking at the particulars of this class, we see that it's a stopwatch view, not a cell. That means there really will only be one of these on the screen at once. It's not a cell, that can appear on-screen a hundred or a thousand times. That would certainly be a different situation.

We see that it's a stopwatch, which makes us think it should get drawn no more than once a second. We don't know about the code that calls this class, but we hope it is architected not to call this class excessively. (Note that if it calls us more than 10x a second, I've got the -set... method to just return rather than redraw, so it'll be a total no-op.)

So, we have a unique view that gets drawn once per second. This is, I would like to guess, not a place where we should spend our time optimizing. Of course, we don't know, because we don't know what the rest of the program does.

--

I agree that -sizeWithAttributes: is costly, but there are several mitigating factors here. One is, almost every time we're drawn we expect to have a new string to draw, since we're a stopwatch. Time marches on, and if the string is different every time we draw, then we're not really getting anything from caching our width from the last time the string was drawn. (Although we could make the assumption that hours will always only be double-digits and get some value from the caching, if we wanted.)

One assumes that, pretty much, if the time isn't changing, then the view isn't being redrawn much, since the window caches the view itself. The only other time the view needs to redraw (in general) is during a window resize, which just isn't that common. As I said, I put this in a project and tested it, and window resizes were very snappy.

The other point is that we're talking about a string that's about 8 characters long. -sizeWithAttributes: should behave reasonably for this length of string.

--

I would, in fact, cache fonts in the class if the fonts were constant, but since ours grow and shrink I don't bother.

In the old days I'd cache images that I'd scale, but with the new Image input framework in 10.4, I don't bother any more; see my previous post. It's wicked fast at drawing scaled-down versions of JPEGs from the original JPEG.

--

I think possibly you are disappointed with this pimping because you expected me to find massive speed inefficiencies, and point out ways to solve them. That's traditionally what people think of when they think about "improving" code, but it's something I am very much against.

If I can make code 10x as readable and only slightly slower, well, I'm going to do it. Machines are very fast. I know, this is counter to what a lot of people say, but remember that I'm not advocating just pissing away cycles; I'm saying, find ways to write clean and minimal code. Very often that will turn out to be the fastest code.

My larger point is, there are several possible areas of optimization here, but lacking data, none of them should be done unless they result in cleaner code. If you drop this code into a project (go ahead, just instantiate a view in a NIB and it'll draw) and do timing tests and find that something is dragging it down, then, by all means, figure out some optimizations.

Personally, I'm guessing the wash-drawing code is a bottleneck, and I'd guess that switching to the CG shading stuff would be a huge win and be pretty readable.

But, remember, the nicest thing about clean code is that it is very easy to optimize at a very high level. For instance, we would redraw all the time with the old class; if you called it 100x a second it'd redraw 100x a second. If you called it with the exact same time interval, it'd still redraw.

So, even if I made my -drawRect: slightly slower (and the jury is out on that), the fact that I don't draw at all can make mine infinitely faster. It's these kinds of gains you should seek in writing any large program -- the gains that come from having a clean architecture where it's easy to track changes and do absolutely minimal updating.

December 24, 2005 3:49 PM

Abhi Beckert said...

// NSObject (class)

- (void)initialize;
{

I don't have time to test your code (it's christmas morning...), but isn't that a typo? Shouldn't it be:

+ (void)initialize
{

Or am I just showing how little I've used Obj-C?

December 24, 2005 3:55 PM

Abhi Beckert said...

Oh and I forgot, please keep up the pimp my code posts! I learn tons from every single one, thank you!

December 24, 2005 3:57 PM

Wil Shipley said...

abhi:

Nice catch! I'm changing this in the post so I don't confuse anyone else. You are right, my original one was a no-op.

December 24, 2005 3:59 PM

Matt said...

Thanks Will! I'll, uh, be working your changes back into my stopwatch ASAP. Yay for other people writing my code for me ;)

December 24, 2005 9:39 PM

Anonymous said...

Come to think of it, rather than creating the NSShadow in +initialize, I would probably create it lazily the first time I hit -drawRect:

-jcr

December 25, 2005 12:21 AM

Carsten Pedersen said...

I wouldn't want to make the shadow a class attribute - when polishing this stuff you'd probably realize that the shadow parameters should vary with size so in my mind it should just be set up in drawRect just like the font size.

Apart from that most of your pimping makes sense except for one thing that we will never agree about:

Usage of the return statement anywhere outside the last line of a method should at least be flagged with a "You've asked for it!" warning - if I wrote the compiler it would simply be illegal right along with all usage of break and goto!

Maybe if you stopped using single character indents the structured alternative would be easier to understand :)

December 25, 2005 5:27 AM

macFanDave said...

Wil,

I am really enjoying this "Pimp My Code" series. All of the other Cocoa resources I've seen show you how to build code "right" (i.e., to the best of the author's ability), but this approach of taking code that is nominally functional and improving it is very thought-provoking and, OK, dammit, I'll say it: FUN!

Perhaps this is the idea with re-factoring and extreme programming, but these are done in languages other than Objective-C and frameworks other than Cocoa.

My humble improvement to your improvement is to use #pragma mark statements to categorize the methods instead of comments. In Xcode, they have a similar delimiting function as comments, but they also make useful separators in the function dropdown.

Thanks, again, and feel free to choose any or all from the following list: Merry Christmas, Happy Hanukkah, Happy Festivus, Happy Kwanzaa, Happy New Year!

December 25, 2005 12:55 PM

Anonymous said...

WIl,

Could you post a full project file with this completed? I'm interested seeing it run after reading through it however without much Cocoa ability I"m having difficulty (mostly with the NIB view/nscell binding).

Thanks!
-MW

December 25, 2005 5:08 PM

Anonymous said...

Is there any way you could add an excerpt RSS feed or change your existing one? Like many NetNewsWire users, I hit spacebar to go from unread feed to feed, and hitting it 30 times just to scroll to the bottom of your very long articles is a bit troublesome.

-Paul

December 25, 2005 6:49 PM

Anonymous said...

Paul,

I think you should file a bug report with the NetNewsWire developers. Asking a blogger to accommodate the particular way you choose to read his articles is a bit much, don't you think?

-jcr

December 26, 2005 12:25 AM

Abhi Beckert said...

I'd ask the NNW guys to let you hit cmd-space to scroll to the end of the article.

December 26, 2005 2:47 PM

Anonymous said...

> I'd ask the NNW guys to let you hit cmd-space to ...

...pop up the Spotlight search field. ;-)

December 26, 2005 5:57 PM

Wil Shipley said...

I heard NetNewsWire was written by total chumps!

(No, not really, just teasing my homies.)

-W

December 26, 2005 6:41 PM

keith ray said...

The refactored version looks very clean. The only things left to do that I can see would be to refactor a few of the longer methods ... I would "extract method" the chunks of code following the comments "draw background wash", "calculate font size for stopwatch text", and "draw stopwatch text".

With good naming for the extracted bits of code, those comments would not necessary, and we might be able to find homes for some of those chunks of code elsewhere: perhaps the "calculate font size" method belongs to a font-related class, if on a larger project, that code would otherwise be repeated in more than one class.

December 27, 2005 11:44 AM

Anonymous said...

I dont know why i read your "pimp my code" posts. I haven't used Obj-C nor mac os x in my life.

i guess its simply good written.

December 28, 2005 4:46 AM

jcburns said...

Good written, indeed! Easily the best written I've read. Also, in some indefinable way gets you in the mindset of "do a good job with this" that works whether you're coding in objc or, uh, HyperTalk.

December 28, 2005 3:12 PM

alxknt said...

will + all,
first off i am still very much learning cocoa + obj-c.

after a few minutes hacking around with your code, i am wondering in your pimped
- (void)_drawGradientInRect:(NSRect)bounds;

aren't these two lines doing the same thing:

NSRectFill(bounds);
[NSBezierPath fillRect:bounds];

?

alex.

December 29, 2005 4:37 PM

Wil Shipley said...

Alex:

You're right, I had a typo and left in the Beizier version. Fixed now. Nice catch!

December 29, 2005 4:47 PM

Anonymous said...

where's the "pimp my code" before new year's eve?

December 30, 2005 8:10 AM

Anonymous said...

I wonder if these kinds of "changes" are things that are done in Delicious Library as well. If so, it would explain the extremely slow performance of it. Who cares how good the code looks if 1). No one but you or the company ever sees the code and 2). If it creates a massive performance penalty that many if not all users will see?

January 02, 2006 4:59 AM

Anonymous said...

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?

Not that it changes anything, but an NSTimeInterval is a double, which is 8 bytes.

January 02, 2006 6:17 AM

Wil Shipley said...

Anonymous snarked: Who cares how good the code looks if 1). No one but you or the company ever sees the code and 2). If it creates a massive performance penalty that many if not all users will see?

This is actually a good question, although I admit I don't accept the premise that Delicious Library is particularly slow.

The thing is, I've answered this question before, again and again. It seems like a disservice to regular readers to reiterate the same point in every "Pimp My Code" episode.

But the answer remains: every user cares how pretty the code is. Because pretty code is code that doesn't have bugs.

Most programmers start with the premise "I should spend my limited time making this program as fast and featureful as possible."

My argument is and has been that this approach is fundamentally flawed. The biggest problem facing programmers is failing to understand their own code, which leads to bugs, general instability, and brittleness.

I'm not saying they don't understand what it's supposed to do, I'm saying they don't understand what it actually does. Less code is much easier to apprehend and much less likely to have bugs, in my experience.

You can complain about Delicious Library's speed if you'd like. My assessment is that if I were a sloppy coder there's no way I could have written DL in 7 months, much LESS have it be lightning fast. If you want to argue that I shouldn't have released it without spending more time on speed, I'll certainly be willing to listen, but I'd have to have actual examples of where it's slow.

It's a pretty much universal law of programming that the time spent writing code is diametrically opposed to how stable, featureful, and fast that code is. I think DL is very stable and reasonably fast and featureful, and the market appears to pretty much agrees with me, based on my sales data. That's pretty amazing for 7 months, I think.

If you don't like my style, my programs, or simply disagree with my premise, though, I invite you to just ignore me. I only am trying to give advice to anyone who wants to emulate my mode of success, I am NOT trying to create the cult of Wil Shipley. You are perfectly welcome to keep your beliefs.

January 02, 2006 8:49 AM

Wil Shipley said...

Also, about the "massive performance penalty" you mention, i'm going to guess you didn't build this code up and actually test it, as I did. Because its speed is great. I explained in the comments that I've actually made it more efficient under actual use conditions.

January 02, 2006 8:51 AM

Anonymous said...

Could you add a download link to the project file in the future by any chance?

January 02, 2006 9:12 AM

Zed said...

Wil, this is great stuff. My friends and coworkers have consistently given me crap because I tend to refactor their stuff similarly (even when they dont' ask for it).

I think this is the difference between someone that develops quick and dirty vs. someone that is more of a software craftsman. :-)

January 04, 2006 11:44 AM

Anonymous said...

Wil, I don't agree with everything you write, but your Pimp My Code stuff is legit. As you said, it's easier to optimize code when you know exactly what it's doing - and if you have to mangle clean code with optimizations, be good about it and try to wrap it up into an extracted method or extracted class, rather than let the optimizations pollute the rest of the class. (And much respect to anyone who recognizes when instance variables are being abused as globals. Return values and parameters are good for you.)

January 05, 2006 4:12 PM

Andy Bons said...

Hi Will,
I really enjoy your 'Pimp My Code' series, and I've been able to find parts 3-6, but I can't seem to find parts 1 and 2. Could you post links to those?

Thanks
Bons

January 13, 2006 1:33 PM

Fjölnir Ásgeirsson said...

Regarding the moving of the constants closer to/into the method they're used in, You know.. I have to disagree with you. I think constants should be gathered at the top of the header.
I just think it's more comfy to know exactly where all of the constants are defined.

February 02, 2006 6:21 AM

Anonymous said...

Mr. Late Guy again… your example of +initialize has a problem: if the class is subclassed, and the subclass does not implement +initialize, your implementation will be called twice. Given your defensive attitude, and the fact that this sort of article gets read by bright-eyed, newbies, this should at least be mentioned, although in this particular case it would only be a small leak.

July 27, 2006 8:11 AM

Ben said...

[macFanDave]
My humble improvement to your improvement is to use #pragma mark statements to categorize the methods instead of comments. In Xcode, they have a similar delimiting function as comments, but they also make useful separators in the function dropdown.

I also use #pragma mark to group related methods. Apple doesn't seem to use this much, but one example is the NSDocument header. You can also write #pragma mark - (with a hyphen or minus), which will create a menu separator in the navigation bar.

August 11, 2006 7:38 PM

Anonymous said...

You removed this code anyway but you missed that

if (myCurrentTime != newTime) {

is buggy for a string ( unless he means a pointer compare)

November 03, 2006 6:29 AM

Anonymous said...

Wil,

You never send CTStopwatchViewTextShadow a release message -- is there something I'm missing?

June 03, 2007 11:30 AM

Wil Shipley said...

Yes... CTStopwatchViewTextShadow is a shared instance that it used (potentially) throughout the life of the application, so we keep it around until we quit, and it is magically freed by the system.

This of it as being like the NSFontManaged or NSFileManager singletons.

June 03, 2007 1:12 PM

Call Me Fishmeal.

Categories

Recent Posts

Pimp My Code

Other Blogs

Arch-Nemesis

Pimp My Code, Part 6: The Pimp Before Christmas

39 Comments: