Commit of the week - 2020 week 9

I had an idea recently of presenting a commit of the week, as a way to showcase good software engineering in a bite-sized show and tell fashion. The thinking is that a lot of good practices, principles and computer science fundamentals can be easily taught with actual code from actual projects, which ensures that it’s relevant and applicable. I want to showcase work from a variety of people and projects, be that something I encounter at work or in open source, or, if I’m being lazy and haven’t been able to think of something for a given week, some of my own work. This is one of those latter types of weeks.

The format I’ll follow is presenting the commit, explaining a bit of the background and what is happening, and the key takeaways as to why this is good thing. That’s it for the formalities, let’s jump right in!

Author: Tarjei Husøy <git@thusoy.com>
Date:   Thu Feb 20 11:46:18 2020 -0800
    Remove postgres version overrides on travis
    The default seems to be 9.5.20 now, so this is moot.
diff --git a/.travis.yml b/.travis.yml
--- a/.travis.yml
+++ b/.travis.yml
services:
    - postgresql
addons:
    postgresql: "9.5"
 install:
-    # There's a bug in postgres 9.5.3 in "on conflict" clause handling that's
-    # fixed in 9.5.4 that we run everywhere else, but is not the default on
-    # travis yet. Thus manually upgrade postgres.
-    - sudo apt-get install postgresql-9.5
-    - sudo service postgresql restart
     - SKIP_VAGRANT=1 SKIP_BOOTSTRAP=1 ./configure
 script:
     - QUIET=false DO_NETWORK_TESTS=true ./tools/docker-test.sh

What is happening here is that the Travis CI build image we were using shipped with Postgres 9.5.3, which had a bug that impacted our code. Some time ago I had added a workaround to the install phase to upgrade this to a newer version of Postgres where the bug was fixed to make sure our test suite could run to completion. Since the default Postgres version had been upgraded, this override was no longer needed and could be removed.

What do I think is good about this? Firstly, we’re removing code. Less code is almost always better. In this case it also made our builds 20-30 seconds faster because we’re doing less work for every build, which means shorter feedback loops. It’s also now more correct, because our builds started failing due to these lines, because when the default was upgraded these lines now made 9.3 the new version for some reason, which also failed our tests. This illustrates how code rots, something that at some point was a good workaround is now making problems, without any intentional change on our part. Code needs maintenance to stay functional.

The other thing I think is good here is that the initial override was well commented. The way the comment here is written made it easy to understand the purpose of something that would otherwise be very confusing, as there’s an earlier statement in the file that says 9.5 should be installed, why is that repeated here? The other good thing about this comment is that explains not only what is being done, but what the original symptons were (the wrong version being installed by default). This makes it easy when reviewing this later to check if that is still the case, by simply checking what is the default version now. If that is no longer the case, the workaround can be removed.

This ensures that when a funky thing is added to the codebase, it has a limited lifespan because it’s easy to evaluate when it’s no longer necessary. Codebases that don’t document similar workarounds will keep accumulating them, and they can’t be removed safely because nobody remembers why they were added in the first place. This makes the codebases gradually harder to work with until they’re basically unmaintainable, because people will have to accomodate for all these weird things that are being done, without understanding why it’s being done in the first place. When the engineers working on something get used to not knowing why things are being done in a certain way, anything they do will come from a place of partial understanding, which leads to partial solutions.

Well-documented code does not fall victom to Chesterton’s Fence, as the reasoning is right there with the weird obstruction. The alternative (or complementary action) to adding a comment is to make sure there’s a test in place that validates the fix. If the woraround is removed, the test will fail and can draw attention to the underlying problem. But tests are also code and will rot over time, thus you should also consider the impact of the test over time, as every single test added makes a test run a little bit slower. If the tests are no longer relevant, they should also be removed.

Key takeways

• Remove code that is no longer relevant.
• When you do anything that is not obvious from the context, explain what the conditions were at the time and why the fix is necessary. Write like you’re writing to a future maintainer that is evaluating if a thing can be deleted.