if-statementerror-handlingcoding-stylecode-complete

How do you write the error checking part of a method to make it readable and error prone?


I had a disagreement with another programmer on how to write a method with a lot of error checking:

public void performAction() {
  if (test1) {
    if (test2) {
      if (test3) {
        // DO STUFF
      } else {
        return "error 3";
      }
    } else {
      return "error 2";
    }
  } else {
    return "error 1";
  }
}

-

public void performAction() {     
  if (!test1) {
    return "error 1";
  }
  if (!test2) {
    return "error 1";
  }
  if (!test3) {
    return "error 1";
  }
  // DO STUFF
}

To me, the deep nesting of if statements makes the first example hard to read.
The second one, despite having three returns, is more readable.

I checked by curiosity what Code Complete was saying about that and it left me less sure about the way to handle this:

The stack of error conditions at the bottom of the nest is a sign of well-written error-processing code.

but then

Indenting the main body of the routine inside four if statements is aesthetically ugly, especially if there’s much code inside the innermost if statement.

and considering using guard clauses, as in the second example

Minimize the number of returns in each routine. It’s harder to understand a routine when, reading it at the bottom, you’re unaware of the possibility that it returned some-where above.

How do you write the error checking part of a method to make it readable and error-prone?


Solution

  • Nothing gets programmers into a fight faster than stylistic debates (Is it ever advantageous to use 'goto' in a language that supports loops and functions? If so, why?). So the short answer is "Whatever style you and your team decide is best for your project/language."

    That being said, I would like to add my 2 cents to Code Complete's comment about multiple returns. You should make a distinction between multiple successful returns, and multiple returns. If I need to investigate 5 returns that are not due to errors being generated, the function probably needs to be rewritten. If you gracefully exit your function immediately after an error is detected, then a maintenance programmer (i.e. you in 6 months) should have no more trouble following the main logic of your function than if you had nested all of those error checks.

    So, my personal opinion is that your second code example is the better choice.