Code Reviews: Hints, tips & principles

10 years or so into my development career, something I have come to appreciate more and more is that both giving excellent code reviews, and structuring my work so that it is easy for others to review, is not just a procedural side note, but one of the key central practices in a development team which can aid productivity, team scaling, knowledge sharing and the minimisation of bugs and regressions. Code reviews can, at first, seem like quite a simple process, but doing them really well requires a lot of thought and nuance beyond just checking lines of code are doing what you’d expect. And for teams who care enough to invest the time and effort in doing them thoughtfully and diligently — they can deliver untold value, both to other individuals but also the wider business.

Clearly this is a big topic, but as a simple outline if you are less familiar with this idea, code reviews can involve (but not limited to)…

Technical Concerns

• Use of SOLID & other design principles
• Adherence to Clean-Code practices / coding guidelines & conventions
• Checklist of language-specific nuance/gotchas
• Application architecture implications
• Application Security
• Confirming test coverage
• Performance & usage monitoring concerns

Business strategy

• Check change against User Acceptance Criteria
• Ensure communication of changes to wider stakeholders

Team Culture & Career Development

• Affirming & broadcasting excellent work to wider team
• Constructive feedback / peer-to-peer professional development

… and in this short-ish thought piece, I’m not attempting to be exhaustive, but just to focus on a handful of practices which I find most critical in both giving great code reviews but also making it easy for others to review my work.

But do we really need this?

Yes. Yes you do.

No matter how experienced you are, peer review of your work helps mitigate some of the fundamental challenges of writing code. And to borrow a couple of well known quotes:

Optimism is an occupational hazard of programming: feedback is the treatment — Kent Beck

Any fool can write code that a computer can understand. Good programmers write code that humans can understand — Martin Fowler

(Which, if this isn’t software heresy, I would be tempted to rewrite as “Any dev can knock out code which meets spec. Good Engineers will write code which another can easily understand and extend” — Someone who isn’t as smart as Martin Fowler)

Technical checklists are (I think) fairly self evident in their benefits. I’ve listed a handful above, which you could easily extend with your own specific context and paradigms, the application of which will almost certainly improve the quality of code you ship. Now whilst these clearly aren’t a magic pill to fix all ailments in your code, done well, with full engagement from the team, they will get you some of the way there — and anything you can do to have higher confidence around code changes going into Production is almost always worth it. Fixing bugs, changing persistent structures and closing security holes is way simpler and less damaging to the business before the code has shipped!

Furthermore, I think the often overlooked benefits of having good code reviews are the softer cultural and team benefits which emerge…

• Creating a culture where everyone’s code is up for discussion, change and improvement… not just senior devs wielding their might over their more junior colleagues.
• Encouraging knowledge sharing and professional development as team members of all levels of experience and seniority can mentor each other.
• Giving a natural context for people to affirm work done well and spend time interacting over a shared enterprise.

Preparing your code review for your colleagues

• If a change necessitates renames, code restructuring or annotations of existing work — split that into a separate no-op change which can be stacked up to review separately. The work your colleagues put most of their focus on should be the smallest diff without additional noise. This makes reviewers’ lives as easy as possible and maximises the chance of bugs and regressions being found! There can be a lot of review fatigue associated with trying to discern sections of a diff which are “worth” reviewing and which are meant to be no-ops — this can and should be largely avoided with helpful change structuring.
• Consider getting a colleague who knows the code well to scan your change before you add all the final tests / annotations, to give a sanity check before you send it round to a wider group. One person seeing off obvious review points on a first pass is much more efficient than lots of people all noticing / commenting on the same thing! And whilst ideally most of the time your communication in the team should ensure you don’t get to code review with fundamental problems with the work, the shorter the feedback cycle, the smaller the chance of lots of wasted work.
• Put together a quick summary (with screen-shots if there are UI changes) of what the change-set does and the impact on the system, users and the wider business. Circulate this not just to Engineers, but also Product Management and Business Analysts. It’s not rare for this process to prompt some discussion which perhaps should have happened in advance, but it’s much better to have this before a change is shipped, than after when it might be harder to change. It’s a good health-check that the internal processes of your team are working well if a summary of the work done is exactly in line with all the different stake-holders’ expectations. Anecdotally — I have a given a few code reviews, where I have helped an Engineer summarise the set of decisions made and how they will limit or affect potential future changes — and when discussing with wider stakeholders, the change has been binned because the now realised costs outweighed the benefits.

Giving a great review

• Scan the change first and get an overview sense of the complete change so you know the categories you are reviewing. (Not every change impacts security / public APIs / UI / database structures… getting a quick sense of what you are looking for can help to trim down the set of concerns you have fighting for attention in your mind. Minimising cognitive load is really important as you can focus more on details after you have a sense of the bigger picture).
• Check out the code locally, check it builds and runs (this is not always the case 😦). If there are visible UI changes, fire up the app and interact with it — if you don’t know how to observe the change, it’s a good chance to get your colleague to show you how! Do some basic QA then and there. Whilst the change should be sound — any QA engineer will tell you that junk does get pushed more often than it should. Having pride in giving good code reviews should encompass making sure that basic issues don’t get through to QA / your test environment. When you do find problems — these provide great examples for obviously missing tests (and these can be some of the most valuable in the test-suite as they represent real defects, not purely hypothetical ones).
• Go over the business Acceptance Criteria both with the Engineer who has made the change but also with the Product Owner / UX specialist. I have been in many code reviews where there has been some misunderstandings of the spec, just because people in the team weren’t communicating well. Whilst these things should ideally be caught well before code review, it’s a good chance for final confirmation before a change ships. This is also a good context for newer members of the team making changes to get more context on why the change is useful to the end consumer — it’s all too easy for the wider business context to have never been clearly communicated.
• Prefer at-desk review with your colleague (if possible!). Whilst this can feel a bit more expensive than going through the detailed review on your own, it’s a great chance to share tips & tricks of using your IDE, sharing different perspectives / ways of thinking and quite simply building good relationships. It’s also an excellent chance to comment on work done really well. There’s little better for building a good relationship with your colleagues than to let them know good work has been noticed.
• The technical minutiae of giving a through technical review is going to be very dependent on your particular code base and languages. But whatever your context, get familiar with clean-code, your own internal coding guidelines, SOLID principles, and the details and nuance of the languages you are using — and apply those across the diff — these relatively simple things will get you a long way.

Don’t lose sight of the real end goal…

Code isn’t implicitly valuable or useful just because it exists. In order to say with certainty that a change has really delivered its intended aim, it needs to be monitored out in the wild with relevant metrics which will tell you it’s doing its job. Will you know if Users are interacting with the new UI at all, how often and in ways you expect? Do you know whether the performance improvement has actually sped up that critical code path with real production traffic? Do you have monitoring when those pre-conditions you thought were always going to be met are actually violated with some odd database state you didn’t think was possible and wasn’t found in your test environment? There are obviously myriad concerns here, and many will be knocked off by generic analytics solutions you have no doubt already plugged into your application — but in my experience, how a change is monitored is often wrongly considered a second class concern in a change-set. The capability to monitor how well a change is performing should be a key output along with the logical code change itself — make it a main part of your code review process.

Senior Devs — I’m looking at you…

If you’re a more senior member of the team, and you keep finding other contributors are making lots of mistakes when modifying / extending code in an area you have done a lot of work in — be more willing to consider that it might be that you have created a system which is overly complex, inappropriately abstracted (under or over!), poorly annotated, under tested, badly named… before assuming that other people are the problem. Whilst there are clearly times that other contributors in your team might be being lazy or just unprofessional, if these are folk who generally produce good work and care about their craft, a code review is a good time to put your hand up and accept the system you have help build might not be all that easy to work in! Going a step further, if you can help refactor/rename/annotate the sections in a change where mistakes were made, perhaps it won’t happen so often in the future…

Sunk cost fallacy

In short — a change doesn’t have to go in just because someone has spent time doing the work. Sometimes a dev. working through a change will reveal considerations which were not foreseen upfront — and it can be that when the work comes to review, it is clear, with input from all stakeholders, that the work needs a complete re-think. If this happens, it’s worth reflecting as a team how you could have found this out sooner, but don’t just accept the change because it’s easier than facing up to the feeling of losing work. The branch will still be there if you need it in the future. However, code with broken assumptions, once shipped, has a habit of getting built on and ingrained in a code base — and it can be hard, both technically and relationally, to remove later. Be brave and be willing to bin work, even at final review stage.

The Golden Rule

Leave the code in a better place than when you found it. Little in the universe seems to suffer entropy and a general decline into disorder quite like a code base where people don’t keep improving it. As a team, all agree that any block of code should be in a better state after a change than before, stick to it, and take pride in it. Code review is a key time to check this — if a file/class/method is touched, make sure it ends up easier to understand and cleaner.

These tips and principles aren’t exhaustive, simply my top ones. Hope these are useful to you and the teams you work in!