Three Points On Error Handling
I’ve been ranting on Twitter lately about how to do error checking the wrong way. Coincidentally, the latest topic on Mac Developer Roundtable was about just that, error handling.
I don’t think this episode of MDR was very interesting. It was essentially an overview of the three common error handling strategies (exceptions, enum returns, pass-by-ref error object), and some general C++/Java likeage from Uli (which is not a compliment :P). However, it got me writing a real blog entry, which is a good thing :)
So: Three things kept repeating in my head while I listened to MDR#3, hoping that someone’d mention it so that we can rid the world of more bad code.
First off, stay far far away from nested ifs. I don’t remember which Mac dev blog I read it on, maybe Wil Shipley’s or ridiculous_fish, but whoever it was recommended to always try to keep the code as far to the left as possible. Use the outermost block for the common, correct case, and use inner blocks for error cases. What I mean by this is, check for the error condition and treat it in-place, don’t check for the not-error case and treat the error in an else far far away. An example is in order…
// Don't:
void collectPhazon() {
PhazonDetector *detector = findNearestPhazonDetector();
if(detector) {
Phazon *nearestPhazon = detector->scanForPhazon();
if(nearestPhazon) {
sendPhazonCollectorDrone(nearestPhazon->position());
} else {
beep(kNoPhazonFoundBeep);
}
} else {
beep(kMajorlyCatastrophicErrorBeep);
}
}
// Do:
void collectPhazon() {
PhazonDetector *detector = findNearestPhazonDetector();
if(!detector) { beep(kMajorlyCatastrophicErrorBeep); return; }
Phazon *nearestPhazon = detector->scanForPhazon();
if(!nearestPhazon) { beep(kNoPhazonFoundBeep); return; }
sendPhazonCollectorDrone(nearestPhazon->position());
}
This also applies to smaller scopes, such as for and while loops. Instead of having a big if block inside the loop, say if(error_condition) continue;. Feel free to use gotos as well, eg:
Baz *a() {
...
if(error) goto a_cleanup;
return myBaz;
a_cleanup:
free(myBaz);
fclose(bazHandle);
return NULL;
}
The same effect can be achieved with try-catch-finally, so use whichever makes your code the easiest to understand.
Secondly, never use just a boolean NO or a nil return value as an error return for a method or function that can fail in more than one way. If you do, the user of your library (or yourself, if it’s in your app!) can’t know what actually went wrong.
This leads to error dialogs such as “Couldn’t connect to iPod” — why not? Because one isn’t connected? Because it’s the wrong model? Because I hit it with a hammer? The sentence is lacking a ‘because’ because the reason is hidden behind bad abstractions.
This is where NSError is your friend, and I think that this is the answer to Scotty’s burning question: ‘When should I use NSError’? The answer is simple: whenver there’s more than one way to fail; whenver a NO/nil can mean more than one kind of failure.
Third, and this was actually mentioned, if you’re checking for errors, make sure you understand /why/ you’re checking for that error, and what the logical response is.
This snippet from Apple’s sample “ComplexPlayThru” is a perfect example of how not to do it (ComplexPlayThru.cpp, line 353-354):
comp = FindNextComponent(NULL, &desc);
if (comp == NULL) exit (-1);
Nice! My app suddenly disappeared with no explanation whatsoever! And I’m sure no other part of the app uses the error code ‘-1’! Awesome. Also, the component that they’re looking for is an Apple default, must-be-there-or-any-sound-app-will-crash component. It’s safe to assume that it will always be there, and just skip the error check.
This also goes against Gus’ advice: please don’t just copy any sample code you find, even if it’s from Apple.
