Setting up Code Review Strategy

Products I shipped benefited greatly from the systematic approach to code review culture. Here is the thing, it doesn't matter how smart is the person, the end product can always benefit from the editorial process. Whether it's design meeting for UX, editors remarks for writers, or peer review for scientific papers the process of editing is how great products are created.

And writing code is no different, code reviews can improve the end product, but on top of that – reviews can help distribute knowledge across the team. So while each product and circumstance is unique, I usually start setting a code review strategy for a new product or component within a product with the following goals:

  • Keep the codebase adaptive, and ready to new business capabilities.

  • Making sure than at least more than one person reads and aware of a particular feature being implemented, through code reviews.

  • Making sure that the team is aware of the old code.

  • Understand the current state of technical debt. The goals can change from product to product, but it's proven to be a good default list, feel free to add or remove items that are necessary but don't overload code reviews. I achieve this through 3 types of code review: Pull Requests, Code Review Workshops, Code Audits, and Pair Programming. I've sorted processes in the order of helpfulness according to my experience.

Pull Requests

Pull requests are great, and they ensure that at least two people are aware of any given implementation: the person who wrote the code, and the person who reviewed.

HOW TO:

Set a policy in source control, such as GitHub, GitLab, Bitbucket, requiring that at least one person have to review the code before it can be merged back into development or production branch (depending on branching strategy).

GOOD:

  • Pull-requests help keep the quality of the product.

  • The team is aware of what other people are doing at the same time, this can inform their decisions, and help avoid duplication of work.

  • And as stated multiple times - it makes sure there are at least two people aware of given feature implementation.

BAD:

  • By definition code reviews are not holistic, people review a piece of a larger entity, and while the piece could be good, the way it fits could be wrong.

  • Code reviews have deadlines, engineers are busy, and have to take time from their work to review somebody else, which means engineers end up time-boxing them, which is a pragmatic approach, but doesn't help reviews.

NOTES:

  • Depending on team size, and constraints, such as compliance, and security - additional people can be asked to review the code, to make sure that it's compliant.

  • Schedule pull-requests after the code went through Continuous Integration pipeline, we don't want engineers to spend time reviewing code that will fail a test, static analysis, or other checks.

Code Review Workshops

The goal of a workshop is to take a single piece of code, such as RESTful API, RPC call, implementation of a specific algorithm, the definition of UI component, test case, etc.

HOW TO:

Run workshops at the same cadence as retrospectives, or once in two weeks, if there are no retrospectives. Cadence can be temporarily increased to once a week from time to time if there is an increase in regressions or a lot of newcomers to the team. Chose a piece of code, ideally 1-3 files, and send everyone an invite with a link, in case they want to prepare ahead of time.

GOOD:

  • Reducing confusion. By defining the way type of code should be implemented, we eliminate a need for an engineer to clarify minutia, and amount of non-important decisions they have to make. If outcomes are documented, and the accessible team can build a knowledge base of how a category of items should be implemented.

  • Improving the usefulness of tests. In my experience, no one is good at producing consistently useful tests. The workshop can help eliminate unnecessary tests that won't improve the velocity of development, and improve remainder.

  • Improve separation of acceptance criteria (business logic, functional requirements) and quality criteria (non-functional requirements).

BAD:

  • If outcomes of the workshops are not documented, or not accessible, the advantages of running such a workshop are rather limited. Documentation doesn't have to be separate though, it can be a comment. If we define how a RESTful API Endpoint should be implemented - we can mark it with appropriate comments, and refer people to a specific code.

NOTES:

  • I suggest adding workshops to the ceremonies, maybe have a 45-90 minute workshop before retrospective. Depending on a codebase, need, and how good you are at convincing.

  • I strongly advise inviting product owners, software architects, business analysts, acceptance testers, and QAs to the workshops. Ideally, you want the code to read like a contract, and be able to describe what happens, hiding non-functional aspects of it behind helper methods. But that's another paper altogether.

Code Audits

This is probably more important to senior technical staff, such as solution and technical architects, tech leads, principal engineers, etc.

HOW TO:

Schedule a code audit once in 2-3 months in your calendar. And make sure you actually have time. Easier said than done!

GOOD:

  • This is the only process that addresses how to understand solution holistically.

  • Code audits help understand how specific implementation behaved in time. Something might sound good in a moment, but it's important to understand if it's good 2-3-4-10 sprints later.

BAD:

  • The process is time-consuming, and not always fruitful. Try playing around with timeboxing it, or cancel altogether, if it doesn't benefit the product.

NOTES:

  • If an employee wants to be promoted to tech lead or some kind of software architect - assigning this task to them could be a good way for them to get experience in working with code holistically.

Pair Programming

To be honest, I didn't find full-time pair programming to be all that effective, unless the team is used to the process. That said occasional pair programming is extremely effective. My remarks aside - pair programming ensures that at least two people have an understanding of implementation, as long as it's two people paying attention.

HOW TO:

It's an integral part of the eXtreme Programming, so one way is to do that. I usually give the engineering team a few examples and tell them that they are free to pair if they think they will be more productive. If you need more stringent rules or don't trust your engineers - probably look into your hiring strategy, and culture, or at yourself ;)

GOOD:

  • As mentioned multiple times - the distribution of knowledge.

  • Tests and implementation can be implemented by two different people, that said - they could also just work in parallel.

  • It frees one engineer to think strategically about how the feature fits into the overall product, whi

BAD:

  • It takes time for people to get used to the pair programming, and I don't think it's for everybody.

  • People with ADHD (or similar) can find it hard to concentrate on code if they are not in the driving seat.

Notable Mention - Mob Programming

I only did this once in my career, for about two months we were implementing a lot of code with business analyst, front-end, and back-end developer looking at the same code. I'm not going to list the pros and cons of it, because I don't have enough information, but please contact me if you have good or bad experiences.

Example Code Review Strategy

In their book - Building Evolutionary Architectures, Neal Ford, Rebecca Parsons, Patrick Kua define continual and triggered, atomic and holistic tests. Example of a triggered test is something like a Unit Test, that's being invoked by an engineer on a specific event such as after compilation, or CI pipeline, while continual test execute well continuously, such as health checks on individual services and so on.

I use similar approach that I stole from Fort et al to code reviews where possible: I try to complement triggered, with continuous, and atomic with holistic. Here is an example strategy that I use as a default.

  • Prioritise Quality Criteria, and make sure we are not spending time reviewing performance if performance is not important. We should be reviwing only for beneficial to product items.

  • Set Pull-Request policy, to require at least two people to review a feature before merging it. In my case merging in development branch, since I prefer git-flow merging strategy.

  • Schedule 45 minute code-review workshops every week for the first month, and then reduce candence to once per two weeks, ideally before retrospective. This has additional advantage, of providing engineers with time to think and frame on how to present specific tech debt, instead of confusing stakeholders.

  • Schedule meeting with myself, and other senior technology managers once in two-three months for code audit.

Again, look into the team, product, and requirements, and modify accordingly.

Appendix A: Some Notes and Random Advice

  • Avoid reviewing code style: it was a fair game in the mid-2000s, but these days majority of languages have dedicated tooling to auto-format code, such as prettier.js for JavaScript and TypeScript, gofmt for Go, and others. Try to make sure that formatting is applied automatically as the engineer writes the code, or before code being pushed (uploaded) to the repository.

  • Review tests: keep an eye on tests, especially unit tests that are not reviewed by product owners or other teams. Make sure that unit tests are useful, i.e. test outcomes and not a particular implementation. Testing particular implementation can slow down development, and unlikely to help.

  • Review documentation: if there is documentation in code, review it, and make sure it's still actual and describes what the function does.

  • Prioritise Quality Criteria (a.k.a. Non-Functional Requirements), and don't spend too much time on reviewing aspects that are not important to an end-product.

  • Pair programming is amazing thing to do in the initial set-up phase, when code base and pipelines are being set up, this way everyone knows how the foundations are laid, and also there is a huge chance that a team member or few will be blocked from time to time in the first few days of development.

  • Tech debt is not necessarily a bad thing, just like financial debt, debt can be a great boon if it's known and manageable, and a disaster if it's hidden.

This article was also posted on Fyuld Blog - Setting up Code Review Strategy

David Grigoryan