What is the best coding practice for if conditions?

Question

Many years of coding have brought me to believe and try to reach this kind of if conditional coding: (demonstrated in C, but it's relevant almost to any language)

 if(a <= 0)
      return false;
 if(strlen(str) <= a)
      return false;
 if(str[a] == NULL)
      return false;
 return true;

Which i think is much more readable than the next coding sample, especially on a vast number of conditions:

 if(a >0)
 {
     if(strlen(str) > a)
     {
            if(str[a] != NULL)
            {
                return true;
            }
      }
  }

While the upper code is much more readable, on large conditions you might find yourself closing so many "}", and the last code i believe is compiled to a better performing code.

Which one do you think it is better to be used?

(the code in this examples is just for demonstrating don't take it literally)

Answer 1

Personally I go with the first section. Return as early as possible.

Answer 2

I choose the second want would be better, since it describe the logic better, and common best practice is you better to have only one return statement inside a function.

I won't choose the first one for performance reason, since the compiler will do the optimization anyway, I prefer code that readable.

Answer 3

The question is not really clear, here. If we are to compare this:

if (c1) return false;
if (c2) return false;
if (c3) return false;
return true;

With this:

if (!c1) {
  if (!c2) {
    if (!c3) {
      return true;
    }
  }
}
return false;

Then I'd vote for neither. Do this instead:

return !c1 && !c2 && !c3;

If the question is about whether or not multiple returns are acceptable, then that question has already been discussed (see: Should a function have only one return statement)

Answer 4

The first approach looks aestheticly better, but I don't think it quite captures what you're trying to do. If all the conditions logically belong together then put them in one if statement (I put my boolean operators at the start of a line to make it easier to comment out single conditions during testing (a hang over from big SQL WHERE clauses). ):

   if(a > 0
      && strlen(str) > a
      && str[a] != NULL)
   {
      return true;
   }

Or, when returning a boolean (which is a special case so might not apply to your question):

   return (a > 0
      && strlen(str) > a
      && str[a] != NULL);

Answer 5

If the conditions have similar or related semantics, it is probably better to use a logical expression instead of a bunch of ifs. Something like

return a > 0 && strlen(str) > a && str[a] != NULL;

But when you have to use the if, the best approach to organizing the branching often (if not always) depends on the nature of if itself.

Most of the time the ifs in the code are "disbalanced": they either have only one branch (no else), or one of the branches is "heavy" while to other branch is "light". In cases like that it is always better to deal with the "simple" branch first and then explicitly indicate that the processing for this branch is over. This is achieved by using the first variant from you question: detect the situation that calls for "simple" processing, do it, and immediately return (if you have to leave the function) or do continue (if you have to proceed to the next iteration of the cycle). It is that immediate return (or continue) that help the reader to understand that this branch is done and there's no need to worry about it anymore. So, a typical function would looks as follows: a bunch of ifs that "intercept" the simple cases, handle them (if necessary) and return immediately. And only after all the simplistic cases are handled, the generic "heavy" processing begins. A function organized like that is much easier to read, than the one with a bunch of nested ifs (like your second variant).

Some might argue that returns from the middle of the function or continue in the middle of the cycle go against the idea of "structured programming". But somehow people miss the fact that the concept of "structured programming" has never been intended for practical usage. No, the "return early" principle (or "continue early" in cycle) significantly improves the readability of the code specifically because it explicitly emphasizes the fact that the processing for this branch is over. In case of "structured" ifs something like this is much less obvious.

To resume the above: Avoid nested ifs. They are hard to read. Avoid "disbalanced" ifs. They are also hard to read. Prefer "flat" branching style which deals with simple cases first and immediately finishes processing by doing an explicit return or continue.

Answer 6

If you follow Design By Contract, then you have pre-conditions that must be met when the function is entered. Although there are situations where it can be proven that some pre-conditions are already met when the function is entered and in those cases a test won't be necessary, let us assume here that tests are necessary. Under these assumptions, function-contract-wise the pre-conditions must be checked before anything else can be done and the checks should be considered as independent of the actual work that the function is supposed to do. It then follows that:

• tests for pre-conditions must appear before any other code in the function
• if a test for a pre-condition fails, the function should return immediately

Returning immediately ensures the complete separation of pre-condition tests from the actual body of the function, not only semantically but also lexically. This makes it easy to review the code and determine whether or not the function contract is followed. It also makes it easy to add or remove pre-conditions later in the event that the function contract is revised.

Sample Code:

// function contract
//
// pre-conditions:
//
// o  bar must not be zero
// o  foo_ptr must not be NULL
// o  foo_ptr must refer to a foo variant of type blue_foo
//
// ...

Foo *foo_blue_init_with_bar(Foo *foo_ptr, int bar, foo_status *status) {

    // ***** Test Pre-conditions *****

    // bar must not be zero
    if (bar == 0) {
        if (status != NULL) *status = FOO_STATUS_INVALID_BAR;
        return NULL;
    }

    // foo_ptr must not be NULL
    if (foo_ptr == NULL) {
        if (status != NULL) *status = FOO_STATUS_INVALID_FOO_POINTER;
        return NULL;
    }

    // foo_ptr must refer to a foo variant of type blue_foo
    if (foo_ptr->type != blue_foo) {
        if (status != NULL) *status = FOO_STATUS_INVALID_FOO_VARIANT;
        return NULL;
    }

    // ***** Actual Work Goes Here *****

    ...

} // end foo_blue_init_with_bar

On the other hand, if you do not follow Design By Contract, then it may perhaps be considered a matter of personal preference.

Answer 7

A main reason for going with the first approach is the indentation remains within sane levels. Anything beyond two levels starts becoming unmanageable. Personally, I try to not exceed a single indentation level inside a method most times.

However, intuitively, for me at least, the second choice is clearer as the logic seems straightforward. When all three conditions are met, we return true. I tend to store each boolean result into a variable whose name makes sense as to what the value conveys, and return their AND'ed value.

isPositive = a > 0;
isStringLonger = str.length() > a;
isCharacterNonNull = str[a] != NULL;

return isPositive && isStringLonger && isCharacterNonNull;

I just made up the names based on your example so they look like garbage but the point is that they should not.

Alternatively, when these three variables seem a lot, I combine them into a single variable before returning that.

isValidString = isPositive && isStringLonger && isCharacterNonNull;
return isValidString;

As far as multiple return statements go, I've found that Ruby's way of suffixing a condition to most statements is very clean and readable.

return false if a <= 0
return false if str.length() <= a
return false if !str[a].nil?
return true

Answer 8

Your two samples do different things (see code below). I would prefer the second if it was written like the below (except maybe with less brackets.

if((a>0) && (strlen(str) > a) && (str[a] != NULL))
{
    return true;
}

or written like

return ((a>0) && (strlen(str) > a) && (str[a] != NULL))

---

To answer your question. I prefer the first (unless the code above is correct) and i always prefer

1. Less Tabbing. I prefer everything as left as possible which your 2nd example breaks (but not mine)
2. Return at the top if possible ex if(cond) return blah; else { code(); }

3. Have short code at the top and longer code at the bottom

   if(cond) { code(); } else if(cond2) { code(); code(); } else { code(); code(); code(); }