Which is better coding style?

Question

During a code review, a senior dev commented on some nesting I had going on in my code. He suggested I set a bool value so that I never have more than one level of nesting. I think my code is more readable but want to get other devs' opinion on this. Which is better style? Is his knee-jerk aversion to nesting founded?

Below are some simplified code examples.

Nested:

If(condition1)
{
    If(condition2)
    {
     if(condition3)
     {
      return true;
     }
     else
     {
      log("condition3 failed");
     }
    else
    {
     log("condition2 failed")
    }
}
else
{
    log("condition1 failed")
}

return false;

or

Bool Driven:

bool bRC = false;

bRC = (condition1);
if(brc)
{
    bRC = (condition2);
}
else
{
    log("condition1 failed");
    return false;
}

if(bRC)
{
    bRC = (condition3);
}
else
{
    log("condition2 failed");
    return false;
}

if(bRC)
{
    return true;
}
else
{
    log("condition3 failed");
    return false;
}

Answer 1

I generally prefer nested for my conditions; of course, if my nested conditions are getting indented too far to the right, I have to start wondering if there's a better way to go about whatever I'm trying to do (refactoring, redesigning, etc..)

Answer 2

I personally find the nested code significantly easier to read.

Answer 3

The second style is absurdly verbose: did he really suggest exactly this? You don't need most of those if statements, because there is a return in the else part.

With the nested example you are relying on not forgetting to include any possible else clauses.

Neither of these seems satisfactory to me.

Answer 4

I like yours better, but I'd probably do something like:

if (condition1 && condition2 && condition3)
{
    return true;
}
else if (!condition1)
{
    log("condition1 failed");
}
else if (!condition2)
{
    log("condition2 failed");
}
else
{
    log("condition3 failed");
}
return false;

If the conditions are complex expressions then I might assign the expressions to appropriately named variables before evaluating the if statements to avoid having to recompute the values in each if.

This is assuming that the normal mode is that all conditions are true and thus you want to have that check first. If the normal mode is that one or more conditions are false, then I'd reorder it and check each negation in turn and simply return true if all the checks failed. That would also remove the need for temporary variables to take the place of complex expressions.

Answer 5

The bool driven way is confusing. Nesting is fine if required, but you could remove some of the nesting depth by combining the conditions into one statement, or calling a method where some of the further evaluation is done.

Answer 6

Code shall restate the problem in a language given. Therefore I maintain that either snippets can be "better". It depends on the problem being modeled. Although my guess is that neither of the solutions will parallel the actual problem. If you put real terms instead of condition1,2,3 it might completely change the "best" code.
I suspect there is a better (3d) way to write that all together.

Answer 7

I'd probably go with

   if (!condition1)      log("condition 1 failed");
   else if (!condition2) log("condition 2 failed");
   else if (!condition3) log("condition 3 failed");
   // -------------------------------------------
   return condition1 && condition2 && condition3;

which I believe is equivilent and much cleaner...

Also, once the client decides that all conditions should be evaluated and logged if they fail, not just the first one that fails, this is much easier to modify to do that:

   if (!condition1) log("condition 1 failed");
   if (!condition2) log("condition 2 failed");
   if (!condition3) log("condition 3 failed");
   // -------------------------------------------
   return condition1 && condition2 && condition3;

Answer 8

If the language supports exception handling, I'd go with the following:

try {
    if (!condition1) {
        throw "condition1 failed";
    }

    if (!condition2) {
        throw "condition2 failed";
    }

    if (!condition3) {
        throw "condition3 failed";
    }

    return true;

} catch (e) {
    log(e);
    return false;
}

EDIT From charles bretana: Please see Using Exceptions for control flow

Answer 9

If you don't have any silly rules about multiple return points, I think this is quite nice (and so does someone else, but they deleted their answer for unknown reasons):

if(!condition1)
{
    log("condition1 failed");
    return false;
}

if(!condition2)
{
    log("condition2 failed");
    return false;
}

if(!condition3)
{
    log("condition3 failed");
    return false;
}

return true;

Maybe this is an equal knee-jerk aversion to super-nesting, but it's certainly cleaner than his crap storing the boolean conditions in certain values. However, it may be less readable in context: consider if one of the conditions was isreadable(). It's clearer to say if(isreadable()) because we want to know if something is readable. if(!isreadable()) suggests if we want to know whether it's not readable, which isn't our intention. It's certainly debatable that there may be situations where one is more readable/intuitive than the other, but I'm a fan of this way myself. If someone gets hung up on the returns, you could do this:

if(!condition1)
    log("condition1 failed");

else if(!condition2)
    log("condition2 failed");

else if(!condition3)
    log("condition3 failed");

else
    return true;

return false;

But that's rather underhanded, and less "clear" in my opinion.

Answer 10

This is how I would implement it, provided your implementations actually reflect the desired behavior.

if (!condition1) {
    log("condition1 failed");
    return false;
}
if (!condition2) {
    log("condition2 failed");
    return false;
}
if (!condition3) {
    log("condition3 failed");
    return false;
}
return true;

However, I think it's more likely that every failed condition should be logged.

result = true;
if (!condition1) {
    log("condition1 failed");
    result = false;
}
if (!condition2) {
    log("condition2 failed");
    result = false;
}
if (!condition3) {
    log("condition3 failed");
    result = false;
}
return result;

Answer 11

Similar to the nested version, but much cleaner for me:

if not condition1:
    log("condition 1 failed")
else if not condition2:
    log("condition 2 failed")
else if not condition3:
    log("condition 3 failed")
else
    return true;
return false;

Beware that each condition is evaluated once.

Answer 12

I don't like either way. When you have so much nests something is wrong. In the case of a form validation or something that does indeed require something like this try to figure something out that's more modular or compact.

An example would be an array holding the conditions, through which you'll iterate with a while, and print/break as needed.

There are too many implementations depending on your needs so creating an example code would be pointless.

As a rule of thumb, when your code looks too complicated, it sucks :). Try rethinking it. Following coding practices most of the times makes the code turn out a lot more aesthetic and short; and obviously, also smarter.

Answer 13

if( condition1 && condition2 && condition3 )
    return true;

log(String.Format("{0} failed", !condition1 ? "condition1" : (!condition2 ? "condition2" : "condition3")));
return false;

That way, you don't have to see many lines of code just for logging. And if all your conditions are true, you don't waste time evaluating them in case you have to log.