Nested If (x) checks - Better way to write this?

Question

There are places where I check for valid pointers before I perform an operation with them; these checks can be nested pretty deeply sometimes.

For example, I have

if (a)
{
  if (a->b())
  {
    if (a->b()->c())
    {
      a->b()->c()->DoSomething();
    } 
  }
}

I really don't like the look of this. Is there a way to turn this into something more readable? Ideally,

if (a && a->b() && a->b()->c() )
{
 ...
}

would be great, but obviously would not work.

EDIT - nvm the example that I put up DOES work as everybody has pointed out. I did test it out to see if this works, but there was a bug in my code in my test. duh!

Answer 1

Why would the latter not work?

In C, && is a short-circuit operator, so it is evaluated from left to right, and if any evaluation is false, evaluation stops.

In fact, you could write:

a && a->b() && a->b()->c() && a->b()->c()->DoSomething();

Answer 2

Your second example does work in C/C++. It short circuits when the first FALSE value is hit.

Answer 3

Why 'obviously would not work'? Since the && operator only evaluates the right term if the left is valid, the rewrite is perfectly safe.

Answer 4

Quoting from K&R¹:

Expressions connected by && or || are evaluated from left to right, and it is guaranteed that evaluation will stop as soon as the truth or falsehood is known.

Therefore the latter example will work perfectly, as WhirlWind has noted.

---

¹ The C Programming Language, Second Edition, Page 21.

Answer 5

if (a && a->b() && a->b()->c()) { a->b()->c()->DoSomething(); }

C++ performs lazy evaluation so this will work. First, a would be evaluated and if it's 0 the whole condition is false so there will be no evaluation of the other parts.

This works for the || operator as well. If you write if (a || b), b won't be evaluated if a is true.

Answer 6

You've seen from the other answers that using && will work, and will short-circuit the evaluation when a null pointer is encountered.

The uneasy programmer in me likes to avoid repeating method calls for tests like this since it avoids worrying if they are idempotent or not. One option is to rewrite like this

A* a;
B* b;
C* c;

if ((a=a()) && (b=a->b()) && (c=b->c())) {
  c->doSomething();
}

Admittedly verbose and a bit clunky, but at least you know each method is called just once.

Answer 7

Second one will work provided you want to call only DoSomething().

Answer 8

Since you've already received the direct answer to your question I'll just mention that long chains of calls like you've got there are a code smell and you might consider a better design. Such a design might, in this case, include use of the null object pattern so that your call might just boil down to:

a->CDoSomething();

Answer 9

Chaining works, but it's not necessarily the best general-case answer, particularly because it obscures the failure point. I would instead suggest flattening the tests by inverting the logic so that it exits on failure.

if (!pa)
  return Fail("No pa");

B* pb = pa->b();
if (!pb)
  return Fail("No pb");

C* pc = b->c();
if (!pc)
  return Fail("No pc");

pc->DoSomething();

Same thing, but flat and easy to read. Also, because it immediately handles the failure case, that doesn't get relegated to an else that you might never get around to writing.

In this example, I assumed you didn't want to just silently fail, so I added Fail as a helper that logs the text and returns false. You could also just throw an exception. In fact, if the various methods signaled their failure by throwing an appropriate exception instead of returning null, then all this would be unnecessary. If silent failure was desirable, then a null object pattern would be appropriate.