Wednesday, July 30, 2014

Useless Asserts

Avoid asserts that restate the obvious

In code, it is fairly common to see something like:
class B { ... } ;
class D : B { ... } ;
void foo(B* b) {
  D* d = dynamic_cast<D*>(b);
  assert(d != 0);
  if (d->somefield) ... ;
}
In this context, the assert is just visual noise, and should therefore be removed.  The dereference of d in the subsequent line will abort the program just as effectively as the assert: The fact that d is expected to be a valid pointer is implied by the dereference, and any competent programmer will recognize this.

Rather than using an assert to restate the obvious, the space would be better spent by inserting a comment stating why the program may assume that d is valid at that point in the code.

It makes more sense to add an assert ahead of the dynamic_cast, since dynamic_cast will return a null pointer when b is a valid pointer but cannot be dynamically cast to type D*.  In debugging, that would permit the programmer to distinguish between abuse (passing in a null pointer) and misuse (passing in a pointer of the wrong type).

Of course, strict use of exceptions solves both problems:
void foo(B* b) {
  if (! b) throw NullPointerException("foo must be called with a valid B*");
  D* d = dynamic_cast<D*>(b);
  if (! d) throw InvalidArgumentException("foo's argument must be convertible to type D*");
  if (d->somefield) ... ;
}
But the least a programmer should do is never add (and conscientiously remove) asserts that belabor the obvious.