Monday, June 28, 2010

Code As If…

Sorry for how slow things have been here lately; I'm still suffering from WWDC work backlog, plus I've been spending a lot of time on the new book. This is the first book I've tried to write while also doing full-time client work and it's taking a bit of a toll on me.

I've had a blog post floating around my head since writing Beginning iPhone Development that has never become completely formed. It's about how the process of writing about code has changed the way I write code. I think the thoughts are finally ready to congeal into solid form. So, here goes.

The other day, I saw somebody wearing a T-shirt that said "Dance Like Nobody's Watching". I'm not much of a dancer, but I like the sentiment. However, dancing is not coding. The worst thing you can do is code like nobody's watching. You should code like everybody's watching you.

One thing I noticed after I'd been writing code for public consumption for a while is that I've become much more critical of my own code than I was before I started putting it out there for the world to see. A lot of that comes from pure vanity - I want to minimize the number of stupid mistakes that people are going to call me out on. There are a lot of eyes on the code I write for books and for this blog, and some of those eyes belong to people who are, quite frankly, smarter than me.

It's impossible to always put out perfect code, but I do like to avoid giving the impression of being a sloppy coder or, worse, of being a complete hack. The process I go through with code that goes into a book or blog post goes something like this:
  1. Make it work
  2. Make it work well
  3. Make it read well
The process isn't quite as linear as that makes it look, but that works as a very basic statement of the process. The last step includes the idea of refactoring, but it includes more than that. It's proofreading my code after I'm happy with the way it functions in order to look for ways the code could be written better. I'm not talking about performance here, that goes under #2. This is purely about maintainability and readability. Are my methods and variables using logical names? Is my code easy to read? Is the formatting consistent? Was I being lazy anywhere and doing quick copy and paste coding. Step 3 is about trying to be my own worst critic. You wouldn't send a proposal or other written document out without proofreading it. Your code shouldn't be any different.

I thought that I would revert back to my old ways when I wasn't writing code for public consumption, but it turns out that once acquired, this habit is hard to shake. Frankly, like most developers, I had always assumed that the third step, or at least the more picayune aspects of it, were a waste of time when doing production work.

That hasn't turned out to be true in my experience. In fact, it's been quite the opposite. In almost every case, I spend less time writing code because my code is now more readable, more precise, and generally quite a bit shorter than the way I used to write code. Every time I have to change, fix, or extend existing code, I spend less time doing it, and those little bits add up over the course of even a short job. In the log run, all those little things pay huge dividends.

Here's an example I was working on this weekend for OpenGL ES 2.0 for iOS 4. I'm putting together an Objective-C class to make creating and managing multiple programs and shaders in OpenGL easier. One piece of functionality I wanted to include was the ability to get the program and shader logs out of OpenGL. Without that information, it's awfully hard to identify why a shader won't compile or a program won't link. The end result of my first two steps ("Make it work", "Make it work well"), was this:

- (NSString *)vertexShaderLog
{
GLint logLength = 0, charsWritten = 0;

glGetShaderiv(vertShader, GL_INFO_LOG_LENGTH, &logLength);

if (logLength > 0)
{
char *log = malloc(logLength);
glGetShaderInfoLog(vertShader, logLength, &charsWritten, log);
NSString *ret = [[NSString alloc] initWithBytes:log
length:logLength
encoding:NSUTF8StringEncoding
]
;
free(log);
return ret;
}


return @"No Log Data";
}

- (NSString *)fragmentShaderLog
{
GLint logLength = 0, charsWritten = 0;

glGetShaderiv(fragShader, GL_INFO_LOG_LENGTH, &logLength);

if (logLength > 0)
{
char *log = malloc(logLength);
glGetShaderInfoLog(fragShader, logLength, &charsWritten, log);
NSString *ret = [[NSString alloc] initWithBytes:log
length:logLength
encoding:NSUTF8StringEncoding
]
;
free(log);
return ret;
}


return @"No Log Data";
}

- (NSString *)programLog
{
GLint logLength = 0, charsWritten = 0;

glGetProgramiv(program, GL_INFO_LOG_LENGTH, &logLength);

if (logLength > 0)
{
char *log = malloc(logLength);
glGetProgramInfoLog(program, logLength, &charsWritten, log);
NSString *ret = [[NSString alloc] initWithBytes:log
length:logLength
encoding:NSUTF8StringEncoding
]
;
free(log);
return ret;
}


return @"No Log Data";
}

It was obvious, even as I was writing this code that I wasn't going to leave it like this. You can get away with this kind of unnecessary duplication of code in most projects because it functions fine and most clients and many development managers don't have the technical chops to read your code, but it violates the DRY principle something fierce. This is hardly the worse example of copy-and-paste coding I've ever seen, but code like this shouldn't ever go into production.

I tested this code to make sure the methods worked, then I immediately refactored the two shader log methods by adding a private method declared in an extension. I then changed the two existing methods to simply call the private method, so I was then using the same code for both scenarios.

- (NSString *)shaderLogForShader:(GLuint)shader
{
GLint logLength = 0, charsWritten = 0;

glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &logLength);

if (logLength > 0)
{
char *log = malloc(logLength);
glGetShaderInfoLog(shader, logLength, &charsWritten, log);
NSString *ret = [[NSString alloc] initWithBytes:log
length:logLength
encoding:NSUTF8StringEncoding
]
;
free(log);
return ret;
}


return @"No Log Data";
}

- (NSString *)vertexShaderLog
{
return [self shaderLogForShader:vertShader];
}

- (NSString *)fragmentShaderLog
{
return [self shaderLogForShader:fragShader];
}

- (NSString *)programLog
{
GLint logLength = 0, charsWritten = 0;

glGetProgramiv(program, GL_INFO_LOG_LENGTH, &logLength);

if (logLength > 0)
{
char *log = malloc(logLength);
glGetProgramInfoLog(program, logLength, &charsWritten, log);
NSString *ret = [[NSString alloc] initWithBytes:log
length:logLength
encoding:NSUTF8StringEncoding
]
;
free(log);
return ret;
}


return @"No Log Data";
}

That's definitely better than the original version. I got rid of the obvious duplication between the two nearly identical shader log methods. The only difference between the original two methods was the GLuint (unsigned integer) value passed into the two OpenGL ES function calls.

This code still bothered me, though. Look how similar the shader and program log functionality is. It's identical except for the two function calls and, I realized, those two function calls take exactly the same parameters. It had been a while since I had used them, so it took my brain a few seconds to drudge up how C function pointer worked, but once it did, the path for refactoring this code became obvious. A single method could be used for all three of these cases if I took function pointers to the two OpenGL API calls as method parameters.

Now, this second refactoring is one a lot of people would probably say isn't worth the hassle. It's too much. It doesn't pass a "cost-benefit analysis". The result is not going to be that much shorter, so how can you justify the time? I used to think exactly the same way before spending so much time writing code for public consumption, but now I've become convinced that refactoring like this does pass the cost-benefit analysis. I wasn't putting enough weight on the value of good code when doing that analysis. When you come back to code weeks, months, or years later, you won't remember all the little details. With well-written code, you won't have to.

The truth (in my experience, at least) is that refactored code is enough easier to maintain that it's worth taking the time to do it. Like everything, the more you do it, the better you get at it, and the faster you can do it. Eventually, it becomes second nature. You end up writing better code in roughly the same amount of time that you used to write sloppy-but-functional code. Plus, every time you visit that code in the future, you have a little less code to search through. Whenever you have to debug that code, you have a little less code to step through. Every time you fix the code, you have less code to fix.

If I had an error in the original version of these log functions, I would almost certainly have the same exact error in three places. When I discovered such an error, do you think I would remember to fix it in all three places? Maybe, if I wrote the code recently enough to remember. If somebody else were fixing my code, do you think they'd know they had to fix it in three places? Maybe… but more likely they wouldn't.

Here's the final version of the method I settled on for the book¹:

typedef void (*GLInfoFunction)(GLuint program, GLenum pname, GLint* params);
typedef void (*GLLogFunction) (GLuint program, GLsizei bufsize, GLsizei* length, GLchar* infolog);

- (NSString *)logForOpenGLObject:(GLuint)object
infoCallback:(GLInfoFunction)infoFunc
logFunc:(GLLogFunction)logFunc
{
GLint logLength = 0, charsWritten = 0;

infoFunc(object, GL_INFO_LOG_LENGTH, &logLength);

if (logLength > 0)
{
char *log = malloc(logLength);
logFunc(object, logLength, &charsWritten, log);
NSString *ret = [[NSString alloc] initWithBytes:log
length:logLength
encoding:NSUTF8StringEncoding
]
;
free(log);
return ret;
}


return @"No Log Data";
}

- (NSString *)vertexShaderLog
{
return [self logForOpenGLObject:vertShader
infoCallback:(GLInfoFunction)&glGetShaderiv
logFunc:(GLLogFunction)&glGetShaderInfoLog
]
;

}

- (NSString *)fragmentShaderLog
{
return [self logForOpenGLObject:fragShader
infoCallback:(GLInfoFunction)&glGetShaderiv
logFunc:(GLLogFunction)&glGetShaderInfoLog
]
;
}

- (NSString *)programLog
{
return [self logForOpenGLObject:program
infoCallback:(GLInfoFunction)&glGetProgramiv
logFunc:(GLLogFunction)&glGetProgramInfoLog
]
;
}


Now, if you work for somebody else, especially if you work in a large corporation, you may be measured on a series of metrics including something like "Lines of Code Written" (LOC). While I understand the need for accountability, most corporate metrics are (to be blunt) a bullshit abuse of statistics. The absolute worst metric ever devised is LOC. If you're working somewhere that's still in the coding dark ages and measuring you by LOC, run like hell, or at least lobby for a change in that policy. Measuring performance based on LOC encourages even good engineers to write sloppy, horrible code.

In my example here, I spent some of my time improving my code by removing 25 lines of code. I spent time taking it from 64 lines of code down to 39 lines of code. As an developer, I shouldn't be punished for doing that!

In real-life situations, we often convince ourselves that there's not time for refactoring while we code. After all, we have to make that deadline, and there's always a deadline! We can always come back and refactor the code later when we have more time, right?

Only you won't. You won't come back until you absolutely need to, and by that point, it'd probably be faster to rewrite it. If you work for somebody else, they're unlikely to let you go back and spend time "fixing" something that works perfectly well. That's especially true if you have a non-technical manager. Besides, you'll probably be onto your next deadline already and feeling time pressure all over again.

In the long run, good code is worth the time it takes. Often, it's worth it in the short run as well.

Make time to refactor, and not after the fact make it part of your development process. Do it up front, before it goes to test, because you won't come back. Proofread your code before you ever send out a build.

Or, in other words, code as if everyone was able to see and understand your code.

Addendum


There's another point I want to make. Not only should you code as if there were lots of eyes on your code, you should actually get lots of eyes on your code if you can. You will miss stuff even if you make proofreading and refactoring part of your development process. You will sometimes even miss stupid stuff. Case in point, I got an e-mail shortly after posting this from Brent Simmons of NetNewsWire fame. Brent very kindly took the time to proofread the code and pointed out one bug and several improvements that could be made.

Here are his suggestions:
  1. Return early if logLength < 1, so that the main functionality doesn't have to be indented.
  2. Return an autoreleased string, per Apple's coding conventions
  3. Return nil in the case of no log data, rather than a string that would have to be compared to determine if there was log data or not.
  4. Change the name of the char * and NSString variables to something slightly more meaningful. (Since the method is logForSomething, it returns a variable named log.)
I absolutely agree with every single one of these suggestions and am a little embarrassed about leaking the string, to be perfectly honest. Here's Brent's rewrite of my function that I plan to use in the book (with Brent's permission):

- (NSString *)logForOpenGLObject:(GLuint)object 
infoCallback:(GLInfoFunction)infoFunc
logFunc:(GLLogFunction)logFunc
{
GLint logLength = 0, charsWritten = 0;

infoFunc(object, GL_INFO_LOG_LENGTH, &logLength);
if (logLength < 1)
return nil;

char *logBytes = malloc(logLength);
logFunc(object, logLength, &charsWritten, logBytes);
NSString *log = [[[NSString alloc] initWithBytes:logBytes
length:logLength
encoding:NSUTF8StringEncoding
]
autorelease]
;
free(logBytes);
return log;
}


Thanks, Brent!


1 This may, of course, change if my tech reviewer finds an issue with it



14 comments:

Simon Wolf (@sgaw) said...

Great article. I've been writing code for one of the Cocoa luminaries recently and it has made me sharpen things up a lot of take more care (and pride) in my own code. It can be scary letting others see, let alone critique, your code but it is worth it. When I was in the first steps of my Cocoa journey someone very kindly spent a couple of hours on iChat with me talking through some code I'd sent him and he not only gave freely of his time but also he gave me some great advice and his words that getting rid of bad habits early on was good still rings in my ears.

ScottYelich said...

Along the same lines, if you really refactor all the way down... won't you just end up with little more than basically a single control statement and a jump table? At some point, the code becomes more obfuscated than clear.

For instance, OTBS?

if (logLength < 1)
return nil;

if (logLength < 1) {
return nil;
}

The second would not add any new vertical space to your listing. It allows for "safe" inserting of (debug?) lines after the if, etc. It may not bite you or everyone. It might not bite you today -- but it will bite someone eventually (single statement "for" or "while" loops seem to be particularly susceptible). It can be simple things such as striving to use iterators whenever possible.


Anyway, then there's the whole consistency, why do things like this occur:

if (blah) {
something1;
} else
something2;

... where something1/something2 are single lines/statements.

I guess I'm an advocate of code boring, but code correct. It's going to be amusing to see the havoc that blocks is going to inflict, at least initially -- not so much just with memory management considerations, but just coding style and readability.

If something is known to work or be a best practice/implementation -- stick with it... trying to be cute or refactor too much might actually introduce new bugs. I think LLVM with build and analyze is really a huge win for all coders.

Scott

Wil Shipley said...

Good ideas. Here's my rewrite of Brent's rewrite of your rewrite:

- (NSString *)logForOpenGLObject:(GLuint)objectNumber infoFunction:(GLInfoFunction)infoFunction logFunction:(GLLogFunction)logFunction;
{
  GLint logLength = 0;
  infoFunction(objectNumber, GL_INFO_LOG_LENGTH, &logLength);
  if (logLength < 1)
    return nil;

  char *logUTF8Bytes[logLength];
  logFunction(object, logLength, NULL, logUTF8Bytes);
  return [NSString stringWithUTF8String:logUTF8Bytes];
}

This is shorter and doesn't have an unused variable (charsWritten). Also, it's really easy for the reader/programmer if you name parameters the same way you name your methods (eg, "logFunction:logFunction"), as that way you only have one word to remember instead of two.

Also, never abbreviate. If you do, other people have to learn which things you think are ok to abbreviate or not, and your code is harder to use.

-Wil

rectalogic said...

You could also use:

NSString *log = [[[NSString alloc] initWithBytesNoCopy:logBytes
length:logLength
encoding:NSUTF8StringEncoding
freeWhenDone:YES] autorelease];

to avoid copying, and remove the free(logBytes)

Jeff LaMarche said...

Simon:

As you can see from the comments that follow yours, I'm still learning from Cocoa Luminaries. I think the threshold of becoming a good programmer is learning to not just accept, but appreciate criticism.

Scott:

There's no doubt that personal preference plays a role. Personally, I hate curly braces on the same line, and proper indentation has always been enough of an indicator to me, but I certainly can see your argument. I do agree that if refactoring is making your code less readable, then you're going to far, but I don't think there's any consensus on what's "too far".

Wil:

I'm flattered. Seriously. Thank you for taking the time to read and offer suggestions. You make good points and make me pine for another issue of your "Pimp my Code" series.

Rectalogic:

That's a great suggestion, thanks!

Wil Shipley said...

@rectalogic I considered the no copy thing, but you're optimizing before testing, which we have to learn to resist. I'd rather go with a much shorter (and conceptually simpler) call, and then switch if there's actually any performance problem (which I don't expect, because copying a couple hundred characters is like 20 nanoseconds).

g2webhost said...

Thank you. code cool
.....
Discount Wheels | Buy Laptops

rob said...

Excellent article - should be required reading for anyone who writes code in a team setting.

Les Cop's de Sam said...

Excellent article
Thanks for sharing!

ScottYelich said...

Jeff --

Exactly. One of the big issues I see, though, is simply many multiple different styles within one file. I don't really care what's done (within reason) -- as long as there's consistency within at least the current single file. Jeff, also notice how braces on the next line affect code block folding within Xcode (ie: there's a lot less vertical space used with OTBS.) With this aside, though, I find naming variables and parameters a much more important issue than text formatting. Hell, I even like code colorization now, whereas in the past I did not (of course, color forth is taking it too far), but Xcode's RED for text is a great indication that you've forgotten to match a closing quote.

... but off to comment on magic synth with modern runtime.

Wil --

I agree. In fact, I have just recently found myself naming the variables the same as naming the function. I find where it really helps is the human tracing aspect -- you call a function that takes "someThing" and in the function, you don't have to keep a mental map that "someThing" is now "_myX" ... it's still just "someThing". This allows for more focus on the algorithm and logic... and every little bit helps.

WALKER said...

Hi Mr.La Marche, I'm a long time reader; first time commenter. I'm an iOS developer based in Tokyo. Just wanted to say that Im a huge fan of your blog, and I love what this post is saying.

I'm the only person at my company that understands coding, let alone English so no one ever looks at my code, but I hate looking back through my code for optimizations and then finding that my code is hard to understand so Im always looking at others sample code in an attempt to familiarize myself with better coding practices.

Anyhow, I love the insight on your blog, it's my favorite RSS feed. I hope that someday if you ever come to Japan I could have the honor of buying you q cup of coffee and sounding you out on programmng.

Take care.

SEO Services Consultants said...

Nice information, many thanks to the author. It is incomprehensible to me now, but in general, the usefulness and significance is overwhelming. Thanks again and good luck! Web Design Company

Hire iphone developer said...

Hi
Nice blog ipad development and application
as the programmer developer for the same are highly qualified and experienced in developing different iPhone mobile applications for different types of customers.

hire iphone developer

caseywatson said...

For the developing an application for an Iphone is really an tough job you really required good and update knowledge regarding the Programming your code would be helps lots of us...


ipad wholesale