Programming for Maintainability 2, Update That Code!

Today’s programming for maintainability snippet comes from some code I wrote a few weeks ago. In our codebase we have a (C++) class which looks something like:

template<typename T>
class tAttribute {
public:
    tAttribute& operator=(const tAttribute& other)
    {
        _coercedValue = other._coercedValue;
        _getCallbacks = other._getCallbacks;
        _setCallbacks = other._setCallbacks;
        _commitCallbacks = other._commitCallbacks;
    }

    tAttribute(const tAttribute& other)
    {
        *this = other;
    }

    ...

private:
    T _coercedValue;
    std::vector<iCallback> _getCallbacks;
    std::vector<iCallback> _setCallbacks;
    std::vector<iCallback> _commitCallbacks;
};

This code has been written many years ago, and nobody had touched it until recently. I needed to add a _desiredValue field to the struct, so I happily submitted this diff for review:

 private:
     T _coercedValue;
+    T _desiredValue;
     std::vector<iCallback> _getCallbacks;
     std::vector<iCallback> _setCallbacks;

Reviewers approved without comment, build worked, it ran fine on my machine, submitted.

A week later I had to come back with my tail between my legs when our internal customer was having strange behaviour (it was getting initialized to garbage values when being copy-constructed). The most easiest change to make then was:

     {
         _coercedValue = other._coercedValue;
+        _desiredValue = other._desiredValue;
         _getCallbacks = other._getCallbacks;
         _setCallbacks = other._setCallbacks;

It is said that the developers of the space shuttle software would, whenever they found a bug in the code, they wouldn’t just fix the bug but they would meet and update their process to make eliminate the possibility of that bug occurring. Here, we can do the same sort of thing: Instead of just fixing the bug, how can we prevent the bug from happening?

(NASA also had an extremely rigorous waterfall-style requirements process, and their code cost hundreds of dollars per line, where a typical corporation might be running at around $10/line. So don’t maybe read too much into the analogy.)

Veteran C++ programmers saw the solution as soon as I showed the diff, but C++ has a way to specify “default” operators:

-    tAttribute& operator=(const tAttribute& other)
-    {
-        _coercedValue = other._coercedValue;
-        _getCallbacks = other._getCallbacks;
-        _setCallbacks = other._setCallbacks;
-        _commitCallbacks = other._commitCallbacks;
-    }
-
-    tAttribute(const tAttribute& other)
-    {
-        *this = other;
-    }
+    tAttribute& operator=(const tAttribute& other) = default;
+    tAttribute& operator=(tAttribute&& other) = default;
+    tAttribute(const tAttribute& other) = default;
+    tAttribute(tAttribute&& other) = default;

(adding move operators for good measure)

Now nobody will have to deal with this bug again. It’s always worth keeping in the front of your mind, when digging around older codebases, whether you can do these small changes to keep the software development process running smoothly. Sometimes I find it helpful when editing code to, mentally, throw away the code and ask myself: How would I write this code if I were to write it today from scratch? This allows me to escape from thinking in the same mindset the code was originally written in, to find new better ways to solve the problem.

As a side note, having the right source control can help immensely with this sort of thing. In git (yeah, I know, it’s 2020 and I’m advocating for git), it’s very easy to branch and make a small commit and PR containing only this change. Without (for example, if you use Perforce), if you’re working on a bigger change, you may have to simply bundle this small change into your larger otherwise unrelated review, increasing reviewer burden.

Next weekend is Christmas, so there won’t be an update, but I will be back in the new year.

This blog series updates every week at Programming for Maintainability