Code smell: obscured intent
G16. Obscured Intent. We want code to be as expressive as possible. Run-on expressions, Hungarian notation, and magic numbers all obscure the author’s intent. .. Small and dense as this might appear, it’s also virtually impenetrable. It is worth taking the time to make the intent of our code visible to our readers.
Robert C. Martin. Clean Code. A Handbook of Agile Software Craftsmanship, 2009
A code snippet had me scratch my head while I was creating a new test case for it.
if (op_orig == 2 && !get_config(obj, flag1))
raise_fault("bit unset"));
else if (op_orig == 1 && !get_private_config(obj, flag_foobar))
raise_fault("bit unset"));
else if (type != TxOther && mode != 0x3) {
int seg = get_seg();
ireg_t offset = get_lea();
execute_in_core(seg, offset, type);
}
else {
if (!obj->cfg->has_legacy_compat) {
raise_fault("bit unset"));
}
}
The cases for op_orig == 0 and 3 were supposed to behave identically, but
they did not. In the end I figured it out (type was depengind on op_orig).
The way the code is written contributed to the obscuring of its intent.
- Long chain of if-then-else.
- Mixing of error handling (three conditions for raising fault) and normal path (call to
execute_in_core()). - Arbitrary order of conditionals: two fault conditions in the beginning, one at the end, and the normal path in the middle.
- No handling of the pass-through case (when all conditions are false) leaves it ambiguous whether it was done on purpose, or an omission.
As the first step of refactoring it, all the exceptional conditions have to be moved
to the beginning of the block (and then to a separate function). There is no
need to daisy-chain them using if-else-if, because raise_fault() throws unconditionally.