Software Engineering: If it’s not broken, don’t fix it!

If it’s not broken, don’t fix it…

One of the great advantages of software is its ability to be easily changed.

One of the biggest disadvantages of software is it’s ability to be easily changed.

Why do I say that?

Because it’s far too easy to tweak the code…

…or add functionality that’s not really required;

…or quickly tidy up the code because you’ve though of a better way of implementing it;

…or fix a bug you’ve just found whilst editing the file.

We’ve all done it, even me, but ad hoc changes are a massive source of error. How many times have you heard a red faced programmer say ‘I only changed a comment’ when asked why the code suddenly broke?

Being a software engineer, a good software engineer, takes discipline. At the risk of ruffling a few feathers, you should only ever change the code that you have permission to change. If you spot a problem, log it or raise a defect or do whatever your company’s process is then fix it when you have the authority to do so. Going off piste and making changes willy nilly is a brilliant way to introduce more problems even if you know what you’re doing, in fact, ESPECIALLY if you know what you’re doing.

One company I used to work for had a code base of well over 6 million lines of code. By far the biggest source of defect injection was through the engineers making ad hoc changes to the code base — whilst they had a file open it was easy to make a few other changes and skirt the paperwork, slipping the change in through the back door. Unfortunately this very behaviour frequently created more defects than it fixed. Let me give you a few real world examples to illustrate this point…

Case Study — Refactoring for the sake of refactoring

One of my engineers loved to refactor code, almost to the point of obsession. Every time he fixed a defect or added a new feature, he’d scour the source files looking for ways to improve it. He’d submit changes as part of whatever defect or requirement he was working on, often making substantial changes to the codebase where a simple 1 or 2 line fix was all that was needed.

What’s the problem with this? Surely reducing the codebase, simplifying the design and speeding up the execution is a good thing?

Indeed it is, but that amount of change takes time to implement, time that many engineers simply don’t have owing to the pressure of project deadlines. Furthermore, if that change introduces new and/or unexpected behaviour to the API, creates a memory leak or adds a new defect, yet more time is needed to fix the fix.

Don’t get me wrong, I’m a big fan of refactoring but it needs to be done in a controlled manner. BUT, it needs to be planned, prioritised and scheduled into the workload.

Case Study — Fix only what you’re authorised to change…

At a previous company I was assigned a defect to fix in the file server code. It was relatively straightforward, requiring a few lines of code to be changed within a single file. I’d made the changes, run the tests and everything was fine but my peer reviewer rejected the fix on the grounds that he’d spotted another defect in the code. It was in the same file but a completely different function and his argument was that whilst the file was checked out it made sense to fix that one as well.

He was absolutely correct except that the original defect only authorised me to make changes pertaining to that defect and that defect alone. I could easily make the changes needed to fix the other issue but those changes weren’t sanctioned. To make them would be to make an unauthorised change to the codebase, thereby losing traceability of a behavioural change.

The solution? To raise another defect then make the code changes against it and retest. That might seem like introducing paperwork for the sake of introducing paperwork, and you’d be right — BUT, it now means the code changes are properly documented and traceable; the fix would be included in the release notes and what’s more, my backside doesn’t get kicked for making an unauthorised change.

Why is this even an issue?

In short, if you’re working in a team of one or with a very small codebase this probably seems like overkill.

However, if you’re working on a codebase of over 6,000,000 lines of code with over 1,000 engineers making changes and continuously submitting them, any and every change that’s made needs to be traceable whether that be through your defect fixing process or your feature release process. If even a small handful of engineers started to make ad hoc changes, the code would quickly become unstable and ultimately the overall quality would reduce.

Share your thoughts...