Programming for Maintainability 1, While Break.

When car companies design a car, one of the major things they care about is the ability to maintain the car. The oil stick needs to be accessible and legible, the diagnostic codes need to be within reach of the driver (this is actually a legal thing too), the driver needs to be able to change a tire. Software engineering is not much different: Yes, we need the code we write to compile and not crash, but any professional programmer will tell you they spend at least 5x more time reading code than writing code, whether it be fixing bugs or writing new features in an existing codebase.

There’s a lot of different aspects to this, and in my college at least, it was only covered tangentially at best. We’ve all had at least some exposure to lofty (and sometimes abused) “design patterns.” Raise your hand if you know the name “Uncle Bob” and his five principles, the SOLID principles.

This is all well and good, but sometimes it’s high-level and abstract, and I find it sometimes difficult to apply the principles in our day-to-day programming work. So inspired by a fellow human bean, I’ve decided to start a weekly blog series looking at real-life code and discussing what we can concretely do to improve it. If you have code that you want comments on, feel free to send it my way (C/C++/Python/Rust, anything from one line to hundreds, we’ll discuss anything from naming conventions to abstract design principles).

For the inaugural example code of this series, today’s (C++) code comes from a snippet that came across my desk the other day for review. I’ll give you the background necessary to understand the code real quick:

• This code interacts with hardware devices, where a device is composed of a motherboard with zero, one, or two daughterboards, each of which has two channels.
• The API involves opening sessions to one or more of these devices, and then configuring the behaviour of the device by setting attributes to particular values.

Without further ado, take a look at the snippet:

tChannelMapIterator otherChannel = session->_channelMap.begin();
const size_t currentDboardIndex = session->_channelMap[thisChannelKey].dboardIndex;
while (otherChannel != session->_channelMap.end())
{
    // Skip comparing the attribute for the channel whose
    // callback we're currently in OR if it is located on
    // a different DB.
    const size_t otherDboardIndex = session->_channelMap[otherChannel->first].dboardIndex;
    if (otherChannel->first == thisChannelKey || otherDboardIndex != currentDboardIndex)
    {
        otherChannel++;
        continue;
    }

    setAttributeCoercedValue(otherChannel->first, ATTR_MUX_PATH, value);

    // At this point we have handled all channels on current
    // channel's DB since we have only 2 channels per DB; so exit loop.
    break;
}

What does it do? If you read the code from top to bottom:

• We get the begin() iterator of channelMap.
• We loop over otherChannels until we hit the end() iterator.
• We get the daughterboard index of the iterand.
• If the iterand is the same as our channel or is not on the same daughterboard, we increment the iterand and go back to the top.
• Otherwise, we set the value of the attribute on the other channel and exit the loop.

Could you describe it in one sentence, though?

I’ll give you a second.

Another second.

Okay. Got it? The answer is “this code sets the MUX_PATH attribute to value for every other channel on this daughterboard.” This description makes it fairly straightforward: We understand what things are affected (“every other channel on this daughterboard”) and how they are affected (“set the MUX_PATH attribute to value”).

A wise person once said that code has many readers. But how many? We are taught in school to write code for the compiler. And in a sense, this is the only reader that you must please. If your code doesn’t even compile, your boss will send you packing. What the compiler wants to see is specifically laid out: we know what it wants from specifications and documents and error codes that it gives us when its unhappy.

The compiler is clearly not the only person reading the code though. Another reader is the code reviewer (hopefully, your company has a code review process). The reviewer wants to know: Does this code do what it is supposed to do? But before they answer that, they must answer: What is this code supposed to do?

And so the reviewer likes when we give them the English sentence above, because it means that they can skip having to figure out that second question and get right to the meat of the problem. This is the draw of pair programming and over-the-shoulder reviews: Because the reviewer (or pair partner) can ask, “what the heck is that code supposed to do?” and they get the answer quickly.

There is at least a third reader of the code as well, the maintainer. The maintainer comes weeks, months, maybe years or even decades later (hey, I work on driver software, stuff sticks around). The maintainer wants to know one thing: “What does this code do?” Pair programming and desk reviews don’t help the poor maintainer. Comments don’t really help the maintainer either, since comments invariably lose sync with the code (at which point they start actively hurting the maintainer)

(there’s also a fourth reader: The source control system, which becomes unhappy if your diffs are excessively noisy)

So. The challenge is making this code match the above English sentence as closely as possible. A few things come to mind for this snippet:

• We can use a ranged-for loop so the reader doesn’t have to think about start/end conditions and incrementing.

• The break at the end of the while loop stands out, it’s rare for while loops to look like:

  while (condition) {
    if (otherCondition) {
        iterand++;
        continue;
    }
    doSomething();
    break;
  }
  

  Where does the eye naturally expect to see otherChannel++ in a while loop? At the end. As it turns out, the break is only a minor optimization (the loop will only ever iterate, at most, a few dozen times). So we can just remove it.

• We can split the if statement and put the setAttribute inside the if statement checking the daughterboard.

Rewriting as above, we get:

const size_t currentDboardIndex = session->_channelMap[thisChannelKey].dboardIndex;
for (auto otherChannel : session->_channelMap)
{
    const size_t otherDboardIndex = session->_channelMap[otherChannel->first].dboardIndex;
    if (otherChannel->first == thisChannelKey)
    {
        continue;
    }

    if (otherDboardIndex == currentDboardIndex) {
        setAttributeCoercedValue(otherChannel->first, ATTR_MUX_PATH, value);
    }
}

And now read this code from top to bottom:

• For every channel otherChannel,
• That isn’t this channel,
• If the otherChannel is on the same dboard as this channel, setAttribute.

And look at that: That readthrough now matches our English description above almost exactly. One of the themes in today’s example, and a theme I will likely touch on repeatedly, is thinking about code not as us describing mechanical steps to the computer but considering code as a way of encoding intent for future readers - the people reviewing the code in the short term, and the people adding features or fixing bugs in the long term. We wish we could take a snapshot of our current selves and describe what the code is supposed to do in person, but we can’t so we arrange our code appropriately.

Of course, different languages vary in their expressiveness. In assembly, we could get nowhere near as nice code as above. Some languages allow for more expressiveness, though, such as this equivalent Rust:

for (otherChannel,_) in session.channelMap.iter()
    .filter(|(k,v)| { k != specifier && v.dboardIdx == channel.dboardIdx })
{
    setAttribute(otherChannel, Attribute::MuxPath, value);
}

“For every otherChannel which isn’t this channel and is on the same dboard, set the MUX_PATH attribute to value.”

This blog series updates every week at Programming for Maintainability