Many moons ago, when I was working as part of a team creating our own Linux kernel distros for general availability across the organisation to be used for internally developed software (this was a thing before containerisation and microservices!), I came across code in an old function that I was tasked to patch that was so incomprehensible that I had no idea what to do with it! It was full of unnecessary conditionals and no annotation.
I came across this code again when, stated below, during the Coronavirus lockdown, I decided to clear out some stuff and came upon one of my old work notebooks where I’d copied that code snippet so that I had it to hand when I went over to talk to the engineer who originally wrote it to ask what on earth it was supposed to do.
|
|
if (trans_len_check(fid, symbol_len + sizeof(symbol), &fid) || fid <= 15) { proc_stop(fid); } else { fid -= symbol_len + sizeof(symbol); } |
Luckily, the engineer was still with the company, and after scrambling about trying to remember what, and why he’d written it a year back he was able to tell me it’s purpose. That resulted in me writing the following updated and improved code in the patch.
|
|
if (fid < symbol_len + sizeof(symbol) + 16) { proc_stop(fid);} else { fid -= symbol_len + sizeof(symbol); } |
I think this code is better because it’s far more legible, which means – and this is something I’m more interested in now that I’m no longer a programmer and am more into managing initiatives, teams, organisations and SAFe engineering processes – it’s also organisationally and process-wise more efficient and safer because it is far more easily maintainable and less prone to future maintenance code update errors.
This brings me to a personal bug bear of mine – reviewing someone else’s incomprehensible, unnecessary wrapped in conditionals and especially badly (usually zero) annotated code. Something that’s usually done by a junior member of staff working on patches and bug fixes!
When I first came across the code, I was so inexperienced that it took me some time to figure out what the problem actually was – that the code was illegible and it was futile attempting to try and understand what it did, why it did it the way it did it, and what it was even created to do in the first place!
Which leads to the following – something I always encourage my engineers to do – Annotate! Within the code itself, and not just in the release notes (although that helps too, especially if they can include diagrams of function calls or explanations of why they did something the way they did it). Annotating not just the code or function calls itself, but including, properly structured literature that explains how the function works and it’s purpose helps not just future maintainers of the codebase, but the overall organisation too if the original programmer is asked to review their code again by an enthusiastic but inexperienced junior team member working on updates and patches 😊
