Chain of OR conditions in if statement

Question

In my code I have a if statement, which looks like:

if(someFunction1(a) || someFunction2(b->b1,c) || *d == null || somefunction3(e) > f * g || !e->e1 || ...){
   return 0;
} else {
   do_something;
}

In my code with real variable and function names are conditions nearly in three lines and it looks very overlook. So I decided to rewrite it into form:

if(someFunction1(a)){
   return 0;
} else if(someFunction2(b->b1,c)){
   return 0;
} else if(*d == null){
   return 0;
} else if(somefunction3(e) > f * g){
   return 0;
} else if(!e->e1){
   return 0;
} else if(...){
   return 0;
} else{
   do_something;
}

Is there any argument why I should not do it?

Answer 1

From a purely semantic-syntactical point of view there's no effective difference between them. But if readability is your concern, why don't you use the "datenwolf" formatting style – I came to develop that style over the course of my past 5 projects or so:

if( someFunction1(a)
 || someFunction2(b->b1,c)
 || *d == null
 || somefunction3(e) > f * g
 || !e->e1
 || ...
){
   return 0;
} else {
   do_something;
}

Do you see how beautiful everything lines up? It really looks like a tube the program is falling down through until it hits a met condition. And if you have && it looks like a chain of operations that must not be broken.

Answer 2

As you're asking because of readability you may want to rearrange the long conditional into predicate variables that say why zero must get returned.

bool unnecessary = someFunction1(a) || someFunction2(b->b1,c);
bool beyondTolerance = somefunction3(e) > f * g;
bool invalidInput = *d == nullptr || !e->e1;

if (unnecessary || beyondTolerance || invalidInput)
    return 0;
else 
    ...

This is Martin Fowler's Decompose Conditional refactoring.

Answer 3

Option 1:

• Terseness
• One exit point to avoid redundancy of return statement.

Option 2:

• Exact failure point can be diagnosed easily i.e logs can be added to each branch to detect the failure.

Answer 4

I don't think there is any other problem in this code other than the redundancy involved. If at all you have to make change to the return statement, you have to change it at 6 places,according to your implementation.

But that redundancy does not occur in the first implementation.

Both are similar otherwise.

Answer 5

First of all, you can't answer this question without providing some rationale, or the answer will become completely subjective. I would be wary of people answering "do like this, because I like this best", with no rationale provided.

---

Looking at the code, it is obviously a number of error checks done inside a function. In a real code example, all such error handling usually requires plenty of comments, to describe each individual error condition, as functions with extensive error handling tend to be complex.

Given that, it is not a good idea to write the code as one statement at all, because if you have to squeeze in comments in the middle of the statement, the code will become a mess.

With the above rationale, the best way to write such is perhaps:

/* comments here */
if(someFunction1(a)){
   return 0;
} 

/* comments here */
if(someFunction2(b->b1,c)){
 return 0;
}

...

/* if we got here, then there are no errors */
do_something();

This also have the advantage of being maintainable, should you need to execute code in between the error checks. Or if you wish to split some of the more complex expressions into several lines for readability.

Even though there are plenty of cases where multiple return statements have the potential to create messy code, this is not one of them. In for this case, multiple return statements actually improve readability/maintainability. You shouldn't dogmatically avoid multiple return statements just because some coding standard tells you to do so.

Answer 6

You can do it the following way

int not_valid = someFunction1(a) || 
                someFunction2(b->b1,c) || 
                *d == null || 
                somefunction3(e) > f * g || 
                !e->e1 || ...;

if ( !not_valid )
{
   do_something;
}

return !not_valid;

Instead of not_valid you can select a more appropriate name.:)