Maintainability of Boolean logic - Is nesting if statements needed?

Question

Which of these is better for maintainability?

if (byteArrayVariable != null)
    if (byteArrayVariable .Length != 0)
         //Do something with byteArrayVariable 

OR

if ((byteArrayVariable != null) && (byteArrayVariable.Length != 0))
  //Do something with byteArrayVariable 

I prefer reading and writing the second, but I recall reading in code complete that doing things like that is bad for maintainability.

This is because you are relying on the language to not evaluate the second part of the if if the first part is false and not all languages do that. (The second part will throw an exception if evaluated with a null byteArrayVariable.)

I don't know if that is really something to worry about or not, and I would like general feedback on the question.

Thanks.

Answer 1

I think the second form is fine, and also more clearly represents what you're trying to do. You say that...

I recall reading in code complete that doing things like that is bad for maintainability. This is because you are relying on the language to not evaluate the second part of the if if the first part is false and not all languages do that.

It doesn't matter if all languages do that. You're writing in one particular language, so it only matters if that language does that. Otherwise, you're essentially saying that you shouldn't use the features of a particular language because other languages might not support those features, and that's just silly.

Answer 2

I'm surprised nobody has mentioned it (so hopefully I'm not misreading the question)--but both kind of suck!

The better alternative would be to use early-exits (placing a return statement early in the code if no other code should execute). This makes the assumption that your code is relatively well refactored, and that each method has a concise purpose.

At that point you would do something like:

if ((byteArrayVariable == null) || (byteArrayVariable.Length != 0)) {
  return; // nothing to be done, leaving
}

// Conditions met, do something

It might look a lot like validation--and that's precisely what it is. It validates that conditions have been satisfied for the method to do it's thing--both options you offered hid the actual logic within the pre-condition checks.

If your code is a lot of spaghetti (loooong methods, lots of nested ifs with logic scattered between them, etc) then you should focus on the bigger problem of getting your code into shape before the smaller stylistic issues. Depending on your environment, and the severity of the mess, there are some great tools to help (e.g. Visual Studio has an "Extract Method" refactoring function).

---

As Jim mentions, there's still room for debate about how to structure the early-exit. The alternative to what is above would be:

if (byteArrayVariable == null) 
  return; // nothing to be done, leaving

if (byteArrayVariable.Length != 0)
  return;

// Conditions met, do something

Answer 3

Don't waste time trying to trap for every non this condition. If you can disqualify the data or condition then do so as soon as possible.

if (byteArrayVariable == null) Exit;
if (byteArrayVariable.Length == 0) Exit;
// do something

This lets you adapt your code much easier for new conditions

if (byteArrayVariable == null) Exit;
if (byteArrayVariable.Length == 0) Exit;
if (NewCondition == false) Exit;
if (NewerCondition == true) Exit;
// do something

Answer 4

Your example is the perfect example of why the nested ifs are a bad approach for most languages that properly support boolean comparison. Nesting the ifs creates a situation where your intention is not at all clear. Is it required that the second if immediately follows the first one? Or is it just because you haven't put another operation in there yet?

if ((byteArrayVariable != null) && (byteArrayVariable.Length != 0))
  //Do something with byteArrayVariable 

In this case, these pretty much must be together. They are acting as a guard to ensure you don't perform an operation that would result in an exception. Using nested ifs, you leave it up to interpretation of the person following you as to whether the two represent a two-part guard or some other branching logic. By combining them into a single if I am stating that. It's a lot harder to sneak an operation inside of there where it shouldn't be.

I will always favor clear communication of my intention over someone's stupid assertion that it "doesn't work in all languages". Guess what... throw doesn't work in all languages either, does that mean I shouldn't use it when coding in C# or Java and should instead rely on return values to signal when there was a problem?

All that said, it's a lot more readable to invert it and just return out of the method if either is not satisfied as STW says in their answer.

Answer 5

In this instance your two clauses are directly related, so the 2nd form is preferable.

If they weren't related (e.g. a series of guard clauses on different conditions) then I'd prefer the 1st form (or I'd refactor out a method with a name which represents what the compound condition actually represents).

Answer 6

If you work in Delphi, the first way is, in general, safer. (For the given example, there's not much to gain from using nested ifs.)

Delphi allows you to specify whether or not boolean expressions evaluate or "short circuit", through compiler switches. Doing way #1 (nested ifs) allows you to ensure that the second clause executes if and only if (and strictly after) the first clause evaluates to true.

Answer 7

The second style can backfire in VBA since if the first test is if the object generated, it will still do the second test and error out. I've just got in the habit in VBA when dealing with object verification to always use the first method.

Answer 8

The second form is more readable, especially if you are using {} blocks around each clause, because the first form will lead to nested {} blocks and multiple levels of indentation, which can be a pain to read (you have to count indentation spaces to figure out where each block ends).

Answer 9

I find them both readable, and I do not accept the argument that you should worry about code maintainability if you were to switch languages.

In some languages the second option performs better so it would be the clear choice then.

Answer 10

I'm with you; I do it the second way. To me, it's logically clearer. The concern that some languages will execute the second part even if the first evaluates to false seems a tad silly to me, as I have never in my life written a program that ran in "some languages"; my code always seems to exist in a specific language, which either can handle that construction or can't. (And if I'm not sure whether the specific language can handle it, that's something I really need to learn, isn't it.)

In my mind, the hazard with the first way of doing it is that it leaves me vulnerable to accidentally putting in code outside of that nested if block when I didn't mean to. Which, admittedly, is hardly a huge concern, but it seems more reasonable than fretting over whether my code is language agnostic.

Answer 11

I prefer the second option, because it is more readable.

If the logic you're implementing is more complex (checking that a string is not null and not empty is just an example), then you can make a useful refactoring: extract a boolean function. I'm with @mipadi on the "Code Complete" remark ("it doesn't matter if all languages do that...") If the reader of your code isn't interested in looking inside the extracted function, then the order in which boolean operands are evaluated becomes a moot question to them.

Answer 12

if ((byteArrayVariable != null)
 && (byteArrayVariable.Length != 0)) {
    //Do something with byteArrayVariable 

but that's basically the second option.