Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

From the Code Complete book comes the following quote:

"Put the normal case after the if rather than after the else"

Which means that exceptions/deviations from the standard path should be put in the else case.

But The Pragmatic Programmer teaches us to "crash early" (p. 120).

Which rule should I follow?

share|improve this question
13  
@gnat not duplicate –  BЈовић May 20 at 9:36
14  
the two are not mutually exclusive, there is no ambiguity. –  jwenting May 20 at 10:14
5  
I'm not so sure the code-complete quote is such great advice. Presumably it's an attempt to enhance readability, but there are certainly situations where you'd want uncommon cases to be textually first. Note that this isn't an answer; the existing answers already explain the confusion your question stems from well. –  Eamon Nerbonne May 20 at 14:15
11  
Crash early, crash hard! If one of your if branches returns, use it first. And avoid the else for the rest of the code, you've already returned if pre-conditions failed. Code is easier to read, less indentation... –  CodeAngry May 21 at 16:23
4  
Is it just me who thinks this has nothing to do with readability, but instead was probably just a misguided attempt at optimizing for static branch prediction? –  Mehrdad May 22 at 8:06

7 Answers 7

up vote 171 down vote accepted

"Crash early" is not about which line of code comes earlier textually. It tells you to detect errors in the earliest possible step of processing, so that you don't inadvertently make decisions and computations based on already faulty state.

In an if/else construct, only one of the blocks is executed, so neither can be said to constitute an "earlier" or "later" step. How to order them is therefore a question of readability, and "fail early" doesn't enter into the decision.

share|improve this answer
2  
Your general point is great but I see one issue. If you have (if / else if / else) then the expression evaluated in the "else if" is evaluated after the expression in the "if" statement. The important bit, as you point out, is readability vs conceptual unit of processing. Only one code block is executed but multiple conditions can be evaluated. –  Encaitar May 20 at 15:08
7  
@Encaitar, that level of granularity is far smaller than what is generally intended when the phrase "fail early" is used. –  Stargazer712 May 20 at 19:49
2  
@Encaitar That is the art of programming. When faced with multiple checks, the idea is to try the one most likely to be true first. However, that information may not be known at design time but at the optimization stage, but beware the premature optimization. –  BPugh May 20 at 19:49
    
Fair comments. This was a good answer and I only wanted to try to help make it better for future reference –  Encaitar May 20 at 21:58

If your else statement contains only failure code then, most likely, it should not be there.

Instead of doing this:

if file.exists() :
  if validate(file) :
    # do stuff with file...
  else :
    throw foodAtMummy
else :
  throw toysOutOfPram

do this

if not file.exists() :
  throw toysOutOfPram

if not validate(file) :
  throw foodAtMummy

# do stuff with file...

You don't want to deeply nest your code simply to include error checking.

And, as everyone else has already stated, the two pieces of advice are not contradictory. One is about execution order, the other is about code order.

share|improve this answer
8  
+1 for throw foodAtMummy XD –  jzx May 23 at 20:39
2  
Worth being explicit that advice to put normal flow in the block after if and exceptional flow in the block after else does not apply if you do not have an else! Guard statements like this are the preferred form for handling error conditions in most coding styles. –  Jules May 24 at 5:44
    
+1 this is a good point and actually gives something of an answer to the real question of how to order stuff with error conditions. –  ashes999 May 26 at 3:17

You should follow both of them.

The "Crash early"/fail early advice means that you should test your inputs for possible errors as soon as possible.
For example, if your method accepts a size or count that is supposed to be positive (> 0), then the fail early advice means that you test for that condition right at the beginning of your method rather than waiting for the algorithm to produce nonsense results.

The advice for putting the normal case first means that if you test for a condition, then the most likely path should come first. This helps in performance (as the branch prediction of the processor will be right more often) and readability, because you don't have to skip over blocks of code when trying to figure out what the function is doing in the normal case.
This advice doesn't really apply when you test for a precondition and immediately bail out (by using asserts or if (!precondition) throw constructs), because there is no error handling to skip while reading the code.

share|improve this answer
    
Can you elaborate on the branch prediction part? I wouldn't expect code which is more likely to go to the if case to run faster than code which is more likely to go to the else case. I mean, that's the whole point of branch prediction, isn't it? –  Roman Reiner Jun 15 at 19:55
    
@user136712: In modern (fast) processors, instructions are fetched before the previous instruction has finished processing. Branch prediction is used to increase the likelihood that the instructions fetched while executing a conditional branch are also the right instructions to execute. –  Bart van Ingen Schenau Jun 16 at 6:49
    
I know what branch prediction is. If I read your post correctly you say that if(cond){/*more likely code*/}else{/*less likely code*/} runs faster than if(!cond){/*less likely code*/}else{/*more likely code*/} because of branch prediction. I would think that branch prediction is not biased to either the if or the else statement and only takes history into account. So if else is more likely to happen it should be able to predict that just as fine. Is this assumption false? –  Roman Reiner Jun 16 at 7:01

I think @JackAidley said the gist of it already, but let me formulate it like this:

without Exceptions (e.g. C)

In regular code flow, you have:

if (condition) {
    statement;
} else if (less_likely_condition) {
    less_likely_statement;
} else {
    least_likely_statement;
}
more_statements;

In the “error out early” case, your code suddenly reads:

/* demonstration example, do NOT code like this */
if (condition) {
    statement;
} else {
    error_handling;
    return;
}

If you spot this pattern – a return in an else (or even if) block, rework it immediately so the code in question does not have an else block:

/* only code like this at University, to please structured programming professors */
function foo {
    if (condition) {
        lots_of_statements;
    }
    return;
}

In the real world…

/* code like this instead */
if (!condition) {
    error_handling;
    return;
}
lots_of_statements;

This avoids nesting too deep and fulfils the “break out early” case (helps to keep the mind – and the code flow – clean) and doesn’t violate the “put the more likely thing into the if part” because there simply is no else part.

C and cleanup

Inspired by an answer on a similar question (which got this wrong), here's how you do cleanup with C. You can use one or two exit points there, here's one for two exit points:

struct foo *
alloc_and_init(size_t arg1, int arg2)
{
    struct foo *res;

    if (!(res = calloc(sizeof(struct foo), 1)))
        return (NULL);

    if (foo_init1(res, arg1))
        goto err;
    res.arg1_inited = true;
    if (foo_init2(&(res->blah), arg2))
        goto err;
    foo_init_complete(res);
    return (res);

 err:
    /* safe because we use calloc and false == 0 */
    if (res.arg1_inited)
        foo_dispose1(res);
    free(res);
    return (NULL);
}

You can collapse them into one exit point if there is less cleanup to do:

char *
NULL_safe_strdup(const char *arg)
{
    char *res = NULL;

    if (arg == NULL)
        goto out;

    /* imagine more lines here */
    res = strdup(arg);

 out:
    return (res);
}

This use of goto is perfectly fine, if you can deal with it; the advice to stay off using goto is directed at people who cannot yet decide by themselves whether a use is good, acceptable, bad, spaghetti code, or something else.

Exceptions

The above talks about languages without exceptions, which I highly prefer myself (I can use explicit error handling much better, and with much less surprise). To quote igli:

<igli> exceptions: a truly awful implementation of quite a nice idea.
<igli> just about the worst way you could do something like that, afaic.
<igli> it's like anti-design.
<mirabilos> that too… may I quote you on that?
<igli> sure, tho i doubt anyone will listen ;)

But here's a suggestion how you do it well in a language with exceptions, and when you want to use them well:

error return in the face of exceptions

You can replace most of the early returns with throwing an exception. However, your normal program flow, i.e. any code flow in which the program did not encounter, well, an exception… an error condition or somesuch, shall not raise any exceptions.

This means that…

# this page is only available to logged-in users
if not isLoggedIn():
    # this is Python 2.5 style; insert your favourite raise/throw here
    raise "eh?"

… is okay, but…

/* do not code like this! */
try {
    openFile(xyz, "rw");
} catch (LockedException e) {
    return "file is locked";
}
closeFile(xyz);
return "file is not locked";

… is not. Basically, an exception is not a control flow element. This also makes Operations look weird at you (“those Java™ programmers always tell us that these exceptions are normal”) and can hinder debugging (e.g. tell the IDE to just break on any exception). Exceptions often require the run-time environment to unwind the stack to produce tracebacks, etc. There are probably more reasons against it.

This boils down to: in a language supporting exceptions, use whatever matches the existing logic and style and feels natural. If writing something from scratch, get this agreed on early. If writing a library from scratch, think of your consumers. (Do not, ever, use abort() in a library either…) But whatever you do, do not, as a rule of thumb, have an exception thrown if operation continues (more or less) normally after it.

general advice wrt. Exceptions

Try to get all in-program use of Exceptions agreed on by the whole dev team first. Basically, plan them. Do not use them in abundance. Sometimes, even in C++, Java™, Python, an error return is better. Sometimes it is not; use them with thought.

share|improve this answer
    
In general, I view early returns as code smell. I'd throw an exception instead, if the following code would fail because a precondition wasn't met. Just sayin' –  DanMan May 24 at 10:40
1  
@DanMan: my article was written with C in mind… I normally do not use Exceptions. But I've extended the article with a (oops, it got rather long) suggestion wrt. Exceptions; incidentally, we had the very same question on the company's internal dev mailing list yesterday… –  mirabilos May 24 at 12:14
    
Also, use braces even on one-line ifs and fors. You don't want another goto fail; hidden in identation. –  Bruno Kim May 25 at 4:23
1  
@BrunoKim: this fully depends on the style / coding convention of the project you work with. I work with BSD, where this is frowned upon (more optic clutter and loss of vertical space); at $dayjob however I place them as we agreed on (less difficult for newbies, less chance of errors, etc. as you said). –  mirabilos May 25 at 14:32

In my opinion 'Guard Condition' is one of the best and easiest ways to make code readable. I really hate when I see if at the beginning of the method and don't see the else code because it is off the screen. I have to scroll down just to see throw new Exception.

Put the checks at the beginning so that the person reading code doesn't have to jump all over the method to read it but instead always scan it from top to bottom.

share|improve this answer

(@mirabilos' answer is excellent, but here's how I think about the question to reach the same conclusion:)

I'm thinking about myself (or someone else) reading the code of my function later on. When I read the first line, I can't make any assumptions about my input (except those which I won't be checking anyway). So my thought is "Ok, I know I'm going to be doing things with my arguments. but first let's 'clean them up" - i.e. kill control paths in which they're not to my liking." But at the same time, I don't see the normal case as something that's conditioned; I want to emphasize it's being normal. Thus,

int foo(int* bar, int baz) {

   if (bar == NULL) /* I don't like you, leave me alone */;
   if (baz < 0) /* go away and die */;

   /* there, now I can do the work I came into this function to do,
      and I can safely forget about those if's above and make all 
      the assumptions I like. */

   /* etc. */
}
share|improve this answer
    
Nicely worded answer, +1 from me! –  mirabilos May 31 at 19:03

This kind of condition ordering depends on the criticallity of the code section under question and whether there are defaults that can be used.

In other words:

A. critical section and no defaults => Fail Early

B. non-critical section and defaults => Use defaults in else part

C. in-between cases => decide per case as needed

share|improve this answer
    
is this merely your opinion or you can explain / back it up somehow? –  gnat May 23 at 11:27
    
how exactly is this not backed-up, as each option explains (without many words indeed) why it is used? –  Nikos M. May 23 at 13:04
    
i wouldnt like to say this, but the downvotes in (my) this answer are out-of-context :). this is the question OP asked, whether you have alternative answer is another matter altogether –  Nikos M. May 23 at 13:16
    
I honestly fail to see the explanation here. Say, if someone writes another opinion, like "critical section and no defaults => don't Fail Early", how would this answer help reader to pick of two opposing opinions? Consider editing it into a better shape, to fit How to Answer guidelines. –  gnat May 23 at 13:29
    
ok i see, this indeed could be another explanation, but at least you do understand the word "critical section" and "no defaults" which can imply a strategy to fail early and this is indeed an expanation albeit a minimalistic one –  Nikos M. May 23 at 13:31

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.