Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
    
The first form has a risk of dangling-else's –  JBRWilkinson Oct 19 '10 at 20:59
    
just curious, what language are you working with? –  STW Oct 21 '10 at 1:36
    
@STW - we use C# and have some legacy code in Delphi. –  Vaccano Oct 27 '10 at 14:20
1  
good to know. I'm not familiar with Delphi but in C# you can be confident in the && and || operators performing short-circuit evaluation. Going from one language to another usually has some gotchas, and it's good to be firm about doing code reviews to help catch those little issues. –  STW Oct 27 '10 at 17:18
    
While along with many answers here I find the second more readable, the first is far easier to debug. In this case, I'd still use the second because you're checking easy to debug things. But be careful in general. –  Magus May 14 at 14:28

12 Answers 12

up vote 25 down vote accepted

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.

share|improve this answer
10  
Agreed. Why on earth would I care if my code will run the same when copied into another language? –  Tim Goodman Oct 19 '10 at 21:24

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
1  
In 23 years of Delphi development I have never changed the "Complete boolean eval" option. If I was shipping code elsewhere I would put {$BOOLEVAL OFF} or {$B-} in the unit. –  Gerry Oct 20 '10 at 3:01
    
Me neither. I also always use range checking too... but other people don't. And it shouldn't make a difference except perhaps from a performance perspective becuase you shouldn't be using side effects in functions used in boolean expressions. I'm sure someone out there does, though! –  Frank Shearar Oct 20 '10 at 5:55
    
@Gerry: Agreed. The only reason you could possibly want full evaluation is side effects--and side effects in a conditional are a bad idea! –  Loren Pechtel Jan 4 '11 at 5:16
    
Just re-read my comment 23 years should be 13! I don't have a Tardis –  Gerry Jan 4 '11 at 11:41

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.

share|improve this answer
2  
That's because VBA is a terrible language –  Falmarri Oct 19 '10 at 21:32
    
@Falmarri, it has some terrible things about it (and some good things). I would fix a lot of things if I had my druthers. –  user1842 Oct 19 '10 at 21:38
2  
The lack of short-circuit Boolean evaluation is more of a legacy thing than anything. No idea why they didn't start out with it. The VB.NET "fix" of OrElse and AndAlso is kinda annoying, but probably the only option to deal with it without breaking backward compatibility. –  JohnFx Oct 19 '10 at 21:49

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.

share|improve this answer

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

share|improve this answer

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

share|improve this answer

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
share|improve this answer
1  
I agree with the return instead of the wrapping of the good logic, but the same people will use the same argument about using || as for &&. –  MIA Oct 21 '10 at 1:41

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.

share|improve this answer
    
More than two conditions might warrant their own function which return a null. –  Job Jan 4 '11 at 2:09

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
share|improve this answer
1  
+1 Yup, yup, yup. Logic as simple as this should not result in difficult code. Your (asker's) gut should be telling you this. –  Job Jan 4 '11 at 2:08
if ((byteArrayVariable != null)
 && (byteArrayVariable.Length != 0)) {
    //Do something with byteArrayVariable 

but that's basically the second option.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.