Is an If branch that does nothing a code smell or good practice?

Question

I've responded to threads here (or at least commented) with answers containing code like this, but I'm wondering if it's good or bad form to write a series of if branches with one (or more) of the branches doing nothing in them, generally to eliminate checking for null in every branch.

An example (C# code):

if (str == null) { /* Do nothing */ }
else if (str == "SomeSpecialValue")
{
    // ...
}
else if (str.Length > 1)
{
    // ...
}

instead of:

if (str != null && str == "SomeSpecialValue")
{
    // ...
}
else if (str != null && str.Length > 1)
{
    // ...
}

And, of course, this is just an example, as I tend to use these with larger and more complex classes. And in most of these cases, a null value would indicate to do nothing.

For me, this reduces the complication of my code and makes sense when I see it. So, is this good or bad form (a code smell, even)?

Answer 1

I prefer doing it like this-

if (str != null) 
{
 if (str == "[NULL]")
 {
    // ...
 }
 else if (str.Length > 1)
 {
    // ...
 }
}  

I think you can always "reword" an if with an empty body into it's negation with a body, and that it looks better and makes more sense.

Answer 2

Your first example is perfectly readable to me -- doesn't smell at all.

Answer 3

In this particular case I will return early and it makes code easier to read

if (string.IsNullOrEmpty(str)) { return; }  

I like to put an explicit return statement.

Answer 4

It all depends on context. If putting an empty if statement makes the code more readable, then go for that.

Answer 5

It's readable, whether it is good or bad depends upon what you are trying to achieve - generally long nested "goes-on-forever" type if statements are bad. Don't forget about static string methods baked into the framework: string.IsNullOrEmpty() and string.IsNullOrWhiteSpace().

Your if (str == null) { /* Do nothing */ } line is unusual, but does have one positive point: it is letting other developers know up front that you are deliberately doing nothing for that case, with your long if/else if structure your intentions could become unclear if you changed it to

if (str != null) 
{ 
    /* carry on with the rest of the tests */
}

Answer 6

It is indeed good to avoid the following, because it needlessly re-checks one of the conditions (the fact that the compiler will optimize this away is beside the point--it potentially makes more work for folks trying to read your code):

if (str != null && str == "SomeSpecialValue")
{
    // ...
}
else if (str != null && str.Length > 1)
{
    // ...
}

But it's also rather bizarre to do what you suggested, below:

if (str == null) { /* Do nothing */ }
else if (str == "SomeSpecialValue")
{
    // ...
}
else if (str.Length > 1)
{
    // ...
}

I say this is bizarre because it obfuscates your intent and defies the reader's expectations. If you check for a condition, people expect you to do something if it is satisfied--but you're not. This is because your intent is not to actually process the null condition, but rather to avoid a null pointer when you check the two conditions you're actually interested in. In effect, rather than having two conceptual states to handle, with a sanity provision (non-null input), it reads instead like you have three conceptual states to handle. The fact that, computationally, you could say there are three such states is beside the point--it's less clear.

The usual case approach in this sort of situation is as Oren A suggested--check for the null, and then check the other conditions within the result block:

if (str != null) 
{
 if (str == "SomeSpecialValue")
 {
    // ...
 }
 else if (str.Length > 1)
 {
    // ...
 }
}

This is little more than a matter of readability-enhancing style, as opposed to an issue of code smell.

EDIT: However, if you're set on the do-nothing condition, I do very much like that you included a "do nothing" comment. Otherwise, folks might think you simply forgot to complete the code.

Answer 7

I would normally put a return or something like that in the first if:

void Foo()
{
  if (str == null) { return; }
  if (str == "SomeSpecialValue")
  {
      // ...
  }
  else if (str.Length > 1)
  {
      // ...
  }
}

If you can't do this, because the function does something else after the if/else, I'd say it's time to refactor, and split the if/else part out into a separate function, from which you can return early.

Answer 8

Yes it is a code smell.

One indication is that you thought to ask this question.

Another indication is that the code looks incomplete- as if something should belong there. It may be readable sure, but it feels off.

When reading that code, an outsider has to stop for a second and use brainpower to determine if the code is valid/complete/correct/as intended/adjective.

user359996 hit the nail on the head:

I say this is bizarre because it obfuscates your intent and defies the reader's expectations.