FART: A framework for Code Reviews
Tags: codereview, productivity, tutorial
Published: 2022-04-29 16:03:28

Code reviews are very difficult to get right. It is common and easy for developers to waste huge amounts of time on unproductive work. Even worse, poor code review practices contribute to industry burn out. It is very easy for a harsh code review to make a junior developer regret their choice to join your company and it is just as common for senior developers to spend so much time on code reviews that they burn out from lack of variety. Developing a better understanding of the benefits and purpose of code review is critical to avoiding this.

## What a good code review looks like

- Code reviews are inherently subjective however the goal of a code review is to be **as objective as possible**. Every developer has a story of the time a code review turned into a quagmire of conflicting valid opinions.
- Code reviews should be a **two-way process**. If a junior developer submits completely unusable code, that's not just a sign that they need training - it's also a sign that your codebase is hard to work on! Consider whether this is due to poor onboarding, unreadable code, underspecified work tickets, lack of architecture documentation, etc.
- Code reviews are **not adversarial**, they are a collaborative effort to improve the quality of your codebase. You were a bad developer in the past and will be a better developer in future - grant the same respect to others that you want for yourself.
- Code reviews aim to eliminate the need for code review. It is not enough to simply point out every mistake a developer has made. Good code reviews seek to **prevent future mistakes** as well.

## Code review pre-checks

Before even starting on a code review it is essential that the submission is structured correctly. The exact specifications here will vary depending on workflow, developer seniority, codebase complexity, business requirements etc. In general there are three key themes to look out for:

- **Description.** At the very least, does the submission describe what it intends to achieve? You might also require that the submission links to a work ticket, specification of rollback plans, specifying risks and other codebase-specific requirements. These requirements must be covered in documentation about the code submission process.
- **Testing.** Code must have a testing plan. This could be as simple as outlining manual testing steps or as complex as hitting specified code coverage metrics. I generally recommend the latter.
- **Pipelines.** CI pipelines must pass.

If a code review fails any of these pre-checks then you should note this and _immediately pause the code review_. At this point you have identified that the code is not ready for review and must be resubmitted. Do not waste your time investigating further.

From a developer perspective, the reason we stop here is that this gives the developer a chance to fix their mistakes. We do not need to search through the code to outline needed refactors and bug fixes which the developer might resolve as part of making the review ready. Giving extensive notes on an incomplete code review risks demoralizing the developer and denies them the opportunity to reflect and self-review their submission.

From a reviewer perspective, this is simple: if the code isn't ready to review then you have more productive things to do with your time. If the code review is critical or you really don't have anything better to do with your time then you could instead follow up by directly mentoring the developer (e.g. via pair programming).

In general, code reviews should not fail pre-checks and ideally they can be somewhat automated. By establishing pre-checks as a standard we can ensure that the developer fixes most simple mistakes before the reviewer has to invest any time.

**Code review checklists** are a very useful tool for averting this consequence, particularly for ensuring proper issue descriptions. 

We can also require **code coverage metrics** to ensure that every submission has at least basic test coverage and testability.

## Let's start FARTing: performing code review

FART is my useful and easy-to-remember acronym for the four areas you should consider when performing a typical code review.

- **Functionality**: Does the code do what the issue requires? As in, _everything_ it requires? Does it handle large data sets? Does it handle null data? Does it have caching? What about cache invalidation?
  - For frontend development: Will it render nicely on smaller screens/mobile devices? Are there performance implications on mobile? Does it have internationalization (i18n)? Does it support accessibility (a11y)? Might it cause issues on (supported) browsers?
- **Architecture**: It is critical to be _as objective as possible_ here. The aim is not to have perfect architecture, it is to have architecture which _matches or improves on surrounding code_. 
  - Failures here are almost always _not the developer's fault_. The fault instead lies with your codebase, documentation or issue description. 
  - Always consider how to improve your codebase to prevent future failures in this area. Be very careful in how you word comments requesting architectural changes as these can be extremely demoralizing for the developer.
- **Readability**: Is the code self-documenting? If there are unconventional pieces of code, are they signposted with a comment? Functions, classes and files should be as small and focused as is considered practical for this codebase. You may also consider compliance with Object-Oriented and/or Functional paradigms.
- **Testing**: From the pre-checks you already know that the code is testable and that it has basic coverage. Now it's time to introduce your own judgement: does the coverage handle all of the functionality introduced? Is there coverage for potential side effects of the code? 
  - It is critical to consider **potential failure cases** as code coverage tools rarely pick these up and they are easy to overlook. Developers and code coverage tools are typically biased in favor of handling the [happy path](https://en.wikipedia.org/wiki/Happy_path).

## Wait, but what about...

### Bug spotting?

To me this is a major misconception in the world of code reviews. The truth is that [code reviews are not a very good tool for finding bugs](https://medium.com/swlh/code-reviews-fail-at-finding-bugs-a45957bad1ac). Studies indicate that code reviews find between 15% to 80% of bugs. Now, that's not zero, and there's nothing stopping you from combining this with other techniques to improve your numbers. So why don't I list it above?

- **It doesn't scale**. How many times have you spotted and fixed an obvious bug in the codebase only for it to pop up in production anyway? If you don't have test coverage for the bug then you can't guarantee that the bug is gone for good. Worse, as codebases grow in complexity you run the risk of introducing bugs in completely unrelated code. Code review tools only display the changed lines - how often do you scan through the entire codebase to consider side effects? Automated testing works to reduce the risk of introducing bugs in code that was untouched by the submission.
- **It's not a good use of your time**. Just because it finds bug doesn't mean it's an _efficient_ way to find bugs. If you're planning to spend 5 hours going line-by-line through a large changeset, why not simply spend that time pair programming instead? Pair programming has very strong results in bug detection studies and also helps a great deal with knowledge transfer and skill development. It's also a nice way to socialize with your fellow developers.
  - Of course I'm not saying you have to start pair programming tomorrow but if it's critical to catch every bug in the codebase then it's important to realize that there are better tools for the job. Every hour you spend on code review is an hour you're not spending on something else - always consider whether you are making the best use of your time. 
  - That said, every hour you spend on pair programming is also an hour that's not spent with two developers working on new features. Making tradeoffs between reliability and functionality is a critical part of being a developer. You can choose where and how to optimize the tradeoff but you should not abdicate the decision by reflexively applying maximum effort to code reviews.
- **It's not useful for teaching**. As much as possible, developers must learn to avoid writing bugs in the first place. If a theoretical perfect reviewer goes through every line in the code and points out 100% of bugs then this denies the developer the learning experience of evaluating and fixing their own code. It also encourages lax attitudes towards code quality - why should I take the time to spot my own bugs if I can delegate responsibility to the reviewer?
- **It's a recursive problem**. Let's say you encounter a serious bug and the developer reworks their code substantially to fix it. The reviewer now needs to redo their review - and let's be honest here, they're not going to do it to the same strictness as their first attempt. Even if you do catch every new bug introduced by the changes you will demoralize the developer by requesting _yet another set of fixes_.
- **You're doing it anyway**. Yep, I was being inflammatory to encourage you to engage with the text. Got ya! Working for complete test coverage gives you a strong critical lens through which to analyze code paths and consider alternative possibilities. As mentioned above, most developers concentrate solely on the "happy path" and it is rare for there to be any bugs there. The best way to identify bugs is by working through the code to consider alternative paths - so why not formalize it by explicitly considering all the required test cases?
  - If you do spot a bug, it's often best *not* to point it out! Instead you can request a test case which would trigger the bug. This improves developer and reviewer skill in critically reading code and contemplating failure cases.

### Syntax?

- Just use a linter, seriously.
- There can sometimes be reasons to discuss syntax (when it is a genuine concern and falls under "Architecture" or "Readability") but developers love to have a syntax argument. Don't waste time and morale on arguments that don't actually matter.
- The easiest way to avoid syntax arguments is to define explicit coding standards and use all the tools at your disposal to entirely prevent these rules from being violated. So: use a linter. Maybe even multiple linters, if it helps!
- There are many different opinions on how strict a linter should be but my opinion is that **increasing strictness is allowed by default, reducing strictness needs discussion**. Linters are an extremely powerful tool for ensuring code consistency and thereby improving readability. Linters can even detect and prevent common bugs! As such you should put them to work with as much strictness as your team is able to tolerate.
- Putting a linter into your CI pipeline will ensure that it gets run as part of the code review pre-checks.

## Disclaimer

_All of the above is just, like, my opinion, man._ There are good reasons for departing from these rules depending on codebase, team and developer. This is just intended as a discussion point for your team to identify their own set of best practices.