Skip to content

More Coding Gaffes

August 23, 2012

I’ve always been fascinated with sniffing out bad code, and previously wrote about some coding pet peeves. Usually this involves code that functions correctly, but compared to alternatives is hard to read or precariously designed, which in turn leads to difficulty in maintenance. Learning to recognize it is good practice, a skill that leads to better programming. And besides, it can also be amusing, as highlighted in the Daily WTF (where the examples can veer away from correctness into the downright insane). I guess it’s similar to the proverbial train wreck, you just can’t look away.

This week I did some maintenance work, cleaning up some legacy code, and found a couple of examples. Initially this article was titled “More Coding Horrors”, but these turn out to be not so horrible, just slightly bad.

To warm up, let’s look at the first one:

if (A && B) {
  doSomething();
} else if (!A && B) {
  doAnotherThing();
}

The code is correct, but on examination it’s clear the same condition is needlessly checked twice. It pops out here, because the conditions are simply called A & B. But imagine these conditions with longer variable or function names, and separated by longer blocks of code between the brackets, so that they don’t appear next to each other, and then drop this logic into the middle of a complex function. When reading it that way, by the time you reach the else function, you forgot the condition that got you there; it becomes camouflaged by all the detail surrounding it.

But rewriting it this way makes the intent more clear:

if (B) {
  if (A) {
    doSomething();
  } else {
    doAnotherThing();
  }
}

Both versions say the same thing, but this makes it much more obvious that none of this code ever fires if B isn’t true. And if B is true, only one of two actions will occur, depending on A.

This next example is more egregious, finding multiple ways to offend. This again is from actual production code (with real names obscured):

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
class A {
  private final B mB;
  private final Data[] mData;
 
  public A() {
    mData = readDataFromSomewhere();
    mB = new B();
    mb.numDataPoints = mData.length;
    mb.loadData(this);
  }
 
  public Data getDataPoint(int index) {
     return mData[index];
  }
}
 
class B {
  private final A mA;
  public int numDataPoints;
 
  public B(A a) {
    mA = a;
  }
 
  public void loadData() {
     for (int i = 0; i < numDataPoints; i++) {
        Data point = mA.getDataPoint(i);
       // ... process point...
     }
  }
}

How offensive? Let’s count the ways:

  1. Making a class attribute public (line 19)
  2. Requiring an attribute to be set separately from the function that requires it (lines 8 & 26). How error-prone is this?
  3. Initialization requiring multiple sequential steps (lines 7-9)
  4. Violating RAII principle (class B). This is by-product of the previous point.
  5. Needlessly exposing data when better alternatives exist (line 12), e.g. passing the values as function parameters
  6. Holding onto storage of data that is only needed for a short duration (line 6). This data may be quite large, but is only needed for processing when loaded, although the object lifetime may be much longer, so this memory is ineligible to be freed.
  7. Circular reference ownership (lines 2 & 18). Although this doesn’t create memory leak problems for Java, where the garbage collector detects them, if implemented with C++ reference-counted pointers it could be problematic. Also, when circular references may be avoided, ownership and lifetime semantics of objects are easier to understand.

Did I forget some? Probably. But all the above is addressed by a simple rewrite:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
class A {
  private final B mB;
 
  public A() {
    Data[] data = readDataFromSomewhere();
    mB = new B(data);
  }
}
 
class B {
  public B(Data[] data) {
    loadData(data);
  }
 
  private void loadData(Data[] data) {
     for (Data point : data) {
       // ... process point...
     }
  }
}

Both classes are much cleaner, with no need to expose extraneous data publicly.

No comments yet

Leave a Reply

Your email address will not be published. Required fields are marked *