Code reuse can prevent bugs

20 Dec 2020

I am as surprised as you are at the title. The statement does seem like "Duh!", but much to my embarrassment, I have seen enough code in many code bases to need to reiterate this.

The latest example was the following bug that I spotted. First some background. The caller to this function can choose any subset of a given set of 'modes' or 'features'. The caller represents this via a bit-mask. So 1 represents feature 1, 2 represents feature 2, 4 represents feature 3, 8 represents feature 4, and so on. So, if the caller requests with bit-mask 3, then it represents features 1 and 2, and bit-mask 10 represents features 2 and 4.

The following piece of code is support to filter out requests depending on the requested set of 'modes/'features'.

if ((bitMask & ModeEnum.FEATURE_FOO.getValue()) == 0) {
  // FEATURE_FOO has already taken care off the by some other module. So do nothing here.
  return null;
}

/* Some more code here */

if ((bitMask & ModeEnum.FEATURE_BAR.getValue()) == 0) {
  // For now, we process the request only if caller explicitly specified FEATURE_BAR.
  return null;
}
/* more code to process the request with FEATURE_BAR. */

The code above has a bug because the comment inside the first if-check does not match the if-check's logic. The intent was to skip processing the request if FEATURE_FOO is enabled. Instead, it does the exact opposite.

The naive way to fix it would be to replace

if ((bitMask & ModeEnum.FEATURE_FOO.getValue()) == 0) {

with

if ((bitMask & ModeEnum.FEATURE_FOO.getValue()) != 0) {

However, this misses the more important point of why this bug occurred in the first place. The simple answer to that question is that this bug occurred because the author ignored the principle of code reuse. By putting that principle into practice, a cleaner way to write this code (and therefore fix this bug and prevent similar bugs in the future) is as follows.

We first encapsulate the logic to detecting various modes via this function

boolean hasRequestedFeature(int bitMask, ModeEnum feature) {
  return (bitMask & feature.getValue()) != 1;
}

With that function in place, the new code looks as follows

if (!hasRequestedFeature(bitMask, ModeEnum.FEATURE_FOO)) {
  // FEATURE_FOO has already taken care off the by some other module. So do nothing here.
  return null;
}

/* Some more code here */

if (hasRequestedFeature(bitMask, ModeEnum.FEATURE_BAR)) {
  // For now, we process the request only if caller explicitly specified FEATURE_BAR.
  return null;
}
/* more code to process the request with FEATURE_BAR. */

This make the code more readable, and as long as we reuse the hasRequestedFeature() function, such bitwise operation fragility will not reoccur in the code.

Is this obvious? I think so. Was it necessary to belabor the point? Empirical evidence seems to scream "YES!".

Srikanth Sastry A Techie in Boston

Code reuse can prevent bugs