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 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 2

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 3

I personally find the nested code significantly easier to read.

Answer 4

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 5

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 6

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 7

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 8

I think both ways are possible and have their Pros and Cons. I would use the bool-driven style in cases where i would have really deep nesting (8 or 10 or something like that). In your case with 3 levels, i would choose your style but for the exact sample from above, i would go like that:

void YourMethod(...)
{
  if (condition1 && condition2 && consition3)
    return true;

  if (!condition 1)
    log("condition 1 failed");

  if (!condition 2)
    log("condition 2 failed");

  if (!condition 3)
    log("condition 3 failed");

  return result;
}

... or like that if you prefer a single exit point (like i do) ...

void YourMethod(...)
{
  bool result = false;

  if (condition1 && condition2 && consition3)
  { 
    result = true;
  }
  else
  {
    if (!condition 1)
      log("condition 1 failed");

    if (!condition 2)
      log("condition 2 failed");

    if (!condition 3)
      log("condition 3 failed");
  }
  return result;
}

That way, you will get all the failed condition logged in the first run. In your example, you will get only one failed condition logged even if there are more than one failing conditions.

Answer 9

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 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

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 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

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 14

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.