DEV Community: Adam Braimah

Code Review Anti-Patterns

Adam Braimah — Mon, 16 Oct 2023 08:22:03 +0000

There are few developers who haven't felt a touch of nerves after finishing a feature, raising a pull request, and then waiting for the approval of a reviewer - sometimes, multiple reviewers! When done well, code reviews can help prevent bugs and be a welcome opportunity for engineers to collaborate. When done poorly, they can be a source of dread and dysfunction that can badly fracture a team.

Here are three very common issues, all of which I've experienced during my career, with the first one in particular being the inspiration for this piece. Note that these are all issues on the reviewer and organisational side - a second article could easily be written on practices to avoid when on the receiving end of a review!

Rude, Bullying, or Cruel Tone

Software isn't carpentry, painting, or some other craft where there's both a mental component and a physical output that relies on dexterity. As an all-virtual affair, the software that someone writes can very much be regarded as a direct mental projection - a pure manifestation of their knowledge and intelligence. As such, it is important to be extremely careful when critiquing someone's code, because the line between "that's stupid" and "you're stupid" is so thin as to essentially be absent. Even without using those specific words, it's all too common for code reviews to be aggressive, demeaning, or undermining. Firstly and most importantly, this is disrespectful and hurtful to individuals, but it also demolishes trust and psychological safety within the team as a whole.

I'm not the first person ever to observe that there is, sadly, far too much intellectual bullying in software development. While code reviews are an absolute playground for such types, all of us could stand to be more mindful of our tone and approach. Bear in mind that everyone is doing the best they can, may be working with rules or constraints you don't know about, and that anyone can have a sub-par day!

Holding Pull Requests to Ransom

Code authors should be open to feedback and willing to take on board suggestions from the reviewer that will improve the code, so what we're not talking about here is a reviewer properly withholding approval until some reasonable changes have been made. This anti-pattern has two distinct sub-types;

1. Withholding approval pending changes based on personal taste.

This often takes the shape of nit-picking over small stylistic issues, specifically for cases in which the organisation hasn't mandated an approach. This can be reduced with the use of linters/pre-commit hooks in combination with agreed style guides for whichever languages are in use. However, some issues are more subjective and don't lend themselves to automated detection and correction. It's worth making the distinction for anything that falls into this category between something that you wouldn't do that way and something that shouldn't be done that way. Only the latter should hold up the approval of a pull request.

2. Withholding approval pending changes outside of the reasonable scope of the work being carried out.

This second type is a particularly odious practice. In this case, an engineer has submitted a completely reasonable solution to their task, fitted it into the existing codebase with respect to the existing style and structure, and overall there are no problems. Despite this, the reviewer will refuse to approve the pull request until a large amount of further work is done. Commonly, this is a substantial refactor or entire redesign of an area that happened to be touched by one of the commits, even if the proportional size of the submitter's change is tiny. If the reviewer is much more senior than the submitter, this can carry the implicit weight of an order, even if the reviewer is in no official position to assign work. This is an anti-pattern because:

It strong-arms the submitter into resolving historic or systemic issues that preceded them by using their need to complete their currently assigned work as leverage, which is bullying.
It reduces the speed of feature delivery and disrupts scheduling by adding in additional "hidden" work not accounted for in planning/estimation.
Over time, it can ultimately discourage developers from making changes, fearful of being burdened with a disproportionate amount of additional work on anything they touch.

If further work in an area would be of benefit, it should be noted and then added to the product backlog to be prioritised and assigned appropriately. This allows for better planning, visibility, team input, and collective ownership.

No Reviews At All

While it can certainly be argued that dependent on the context and organisation, some changes are so trivial (e.g. a text content update) and externally observable before official release that mandating a review might be seen as wasteful, I would say that the vast majority of changes should be reviewed in some fashion. This isn't a universally held view though, and one counter I often hear is that new functionality that can be disabled/enabled easily via a feature switch removes the risk of deploying new code without the delay of a review. However, using this approach in isolation rests on a very specific assumption;

That the externally observable behaviour of the code is the only thing that matters.

While the code may initially appear to work, there may yet be problems lurking below the surface, just a few of which could be:

Performance issues due to a naïve implementation that won't be obvious in real usage until it has been running for some time
Poor maintainability/flexibility due to the chosen approach
In the worst possible case, unauthorised changes could be slipped in alongside assigned work

Bearing that in mind, why not make use of both approaches together? Reviews can help avoid some of the pitfalls above, while feature-switching can be used both to align feature releases with business needs and to have a safety control in case of unforeseen problems.

Conclusion

The code review process is not only a method for improving software quality but also provides the opportunity for team members to teach, learn, and build trust. Conducting reviews in the right spirit, acting in and assuming good faith, can help make them pleasant and productive for everyone involved.

Technical Debt As Debt

Adam Braimah — Wed, 03 Nov 2021 07:08:58 +0000

Bad Debt by Nick Youngson CC BY-SA 3.0 Pix4Free.org

When technical debt is raised within an organisation, it is sometimes brushed off as being little more than the desire of prima donna developers to spend time polishing and reorganising code for purist reasons rather than any practical benefit, but I would argue that to be so dismissive could be a costly mistake. Good software engineers are as conscious of the financial realities of their workplaces as anyone else, and the term "technical debt" is actually an accurate description of the nature of the trade-offs that are made during development - and the risks that come with them.

The same set of questions that might apply to a loan in the traditional sense could also be asked by a prudent team or organisation deciding whether to incur a particular technical debt - with this in mind, here are some parallels to consider.

1. What are you borrowing for?

All but the richest of us would need to take out a mortgage to buy a home, and in many countries, would need to take out sizeable loans to pay for higher education. A majority of people would likely consider these to be worthy reasons to apply for credit, but would regard borrowing the same for a holiday, a piece of jewellery, or some designer clothes to be unwise. In software, the thought process should be no different - is the debt-producing work in question intended to address a considered investment, a real need, or a mere want?

2. How much are you borrowing?

In money terms, this is a straightforward one - the currency value of the loan. The parallel in code would be asking the size and nature of the compromise being made between the ideal solution and the solution about to be implemented. This definitely isn't just a matter of the number of lines of code, but a question of the pervasiveness, permanence, and external visibility of the changes.

3. What's the interest rate?

I think the best way to think about this in development terms is "how much more difficult (and expensive) does it get to move back towards a better design/implementation as time moves on?" The ideal would be a "zero interest" loan where the amount of work needed to repay the debt would be identical regardless of when it gets done, but a much more common situation could be something like making a sub-optimal change to the main data schema for your system in order to hurriedly introduce a new feature. As time goes on and more data is created, and/or the new schema becomes relied on by more components, moving to a better representation becomes a riskier and more time-consuming prospect. This is especially so if the system in question is one that has no scope for downtime, or if the change involves the interface being provided to external entities.

4. What is your ability to pay the debt back?

If you happen to know that you have a long period with reduced workload coming up, or an increase in staffing, you're likely to be in a much better position to pay off any technical debt incurred than if you're constantly running at capacity with no end in sight. As more and more requirements for new work land, those TODOs and tech debt backlog items will tend to be pushed down the priority order, leading to the once-temporary changes solidifying as accepted parts of the codebase and architecture.

5. What are the consequences for late or non-payment?

Hopefully the analogy is holding so far! This point is related to, but subtly different from the "interest rate" factor. If we assume that we've borrowed a manageable amount with a reasonable repayment plan, what happens if something upends that plan and you can't sort it out on schedule?

Failing to repay borrowed money on time can, depending on the lender, have consequences ranging from damaged relationships at best through to legal action, or even, sadly, risks to personal safety. For the specific type of technical debt incurred, think about what an inability to address it on schedule could mean - it may just be an annoyance, or at worst, critically damaging to the quality and maintainability of your system. You should also bear in mind that codebases that are difficult to work with can in time have negative effects on the morale and even the retention of the staff needed to maintain and further develop them.

Conclusion

It seems blindingly obvious to say, but is often forgotten - development time costs actual money. This is where the two concepts of debt overlap most strongly; almost without exception, taking out technical debt is literally borrowing money from the present (by taking a shorter and initially cheaper development path for a given feature) and then creating a need to repay that later on (with a costlier reworking of the now-existing functionality into a more maintainable form). It is all too easy for today's "just do it" task to become tomorrow's eye-wateringly expensive constraint on maintainability and further product development, especially if there are competing commercial priorities. This is where organisational culture plays a crucial role; the more everyone sees the overall picture rather than their own specific interests and targets, the more all the important factors can be balanced in decision-making.

A Personal Git Repo as a Knowledge Base Wiki

Adam Braimah — Fri, 19 Feb 2021 20:28:15 +0000

While it's not something that everyone likes to do, I've always found it essential to write notes. There are the project-specific notes that only have relevance within a particular workplace and are of course confidential, but then the stacks of notes that cover everything from the content of the training course you last went on to "what's the command-line way to add a new project again?" - and that second category is the one we're going to discuss here.

Besides good old pen and paper I've tried all sorts, with tools like Evernote, Keep, the original Catch Notes (RIP), and Apple Notes each having very different levels of support for formatting, embedding, and crucially, portability of stored data. The latter becomes especially significant if the service closes down, or if (in the case of Apple Notes) you hand back the computer at the end of a job. As I looked at the pages of pencilled notes I'd written while following an online tutorial recently, I realised that a more unified and durable approach would be extremely useful. A solution flashed into my head while updating the wiki for a personal project on Bitbucket - why not have a private repo consisting entirely of Markdown notes, rather than code?

Benefits

Markdown is familiar to a large proportion of developers, is easy to write, and widely-supported - but also fairly readable even as plain text without a parser.
Markdown has great support for inserting the kinds of things we so often need - code snippets, links, tables, and multi-level bullet lists - with minimal effort, allowing you to concentrate on the content.
Using a Git repo makes it easy to quickly sync the notes across machines.
This approach removes the reliance on proprietary sites. As each copy of the repo is just the same as any other, even the loss of your online Git account is not catastrophic.

Basic Structure

The root of the repo has an index.md file, with each overall topic being contained in a folder - and each of these also contains an index.md. If it's a big enough topic, there might be an extra level, but the aim is just organised enough without having a crazy directory tree structure.

The root index.md contains links to the index files in each folder.

Those index files, in turn, link to other .md files within their own directories for each article.

Article Content

The beauty of this being your private repo is that you're not beholden to any particular standard - you can be free to write in as formal or as loose a way as you'd like. At some times you might have nothing but a raw text braindump, at others you may have topics that you can write on in a more structured way. The point that is central is that you write in the way that works for you, in the terms that make it the most useful for the way you read, think, and recall.

Code snippets (with the original spacing preserved) and shell commands can be inserted easily, as can links to external resources that might be useful for each topic.

In addition, now that everyone has a camera in their pocket, diagrams can be sketched, photographed, and included as inline images, rather than being laboriously reproduced with a computerised drawing tool.

As an aside, if you need to copy/paste formatted text from other document formats, PuppyPaste is a very handy tool which will convert the formatting to the Markdown equivalent, which you can then copy/paste/edit as necessary.

Reading

The main online Git sites will support the viewing of Markdown files, but if you want to view them outside of this framework then a small amount of extra work is needed. As web browsers don't generally have native support for Markdown files, you can't get the benefit of using one as a viewer directly, but some very kind people have provided solutions to this. Someone recommended MDwiki and having used it, I will too.

Simply download the HTML file into the root of your repo, and rename it to index.html. Pointing a web server at this folder (I wrote an absolutely minimal one with Node.js and Express) makes it an HTML web-browsable wiki.

Uses and Conclusion

I now have copies of my own repo on a Windows desktop, Mac laptop, and a USB flash drive, as well as on Bitbucket. A quick git pull on each grabs the latest changes as needed, and after that the notes are available even if there is no network available. I'll be continually adding to it and think that this is a solid way of preserving the jewels picked up along my development journey. I hope some of you find this approach useful too!