What is a Code Review?

Firstly, let me tell you what a code review is not. It is not as it was represented in the Netflix series Russian Doll, where the team meets in a room and the manager goes from person to person telling them whether their code was ‘great’ and they ‘nailed it’ or, as in the case of the protagonist, that ‘we found bugs in your code’.

Nope. Code reviews are not for finding bugs. That is what tests are for. They are also not a way for managers to judge the performance of engineers in the team.

A Code review is done peer-to-peer. This means that every member of the team will look at and review other team members’ code. A code review is done when an engineer has finished working on a piece of code and wants to merge it in to the main codebase. To do this, they raise a ‘pull request’ on github (where our code is stored). Before the ‘pull request’ can be merged, other team members need to check it, to make sure they are happy with the changes. Comments can be added to the ‘pull request’, or if the reviewer is happy, they can approve the pull request.

When performing a code review, I look for the following main things:

Acceptance Criteria

Does the code meet the acceptance criteria (i.e. will it do what it was intended to do)? For example, if the task was to expose a new field in an API, I would check if the correct work has been done to expose that field.

DRYness

Is the code DRY? This stands for Don’t Repeat Yourself, and means that code should be written efficiently, with maximum code re-use and no repetition of the same blocks of code. If we have an existing piece of code in a library that already does what the engineer’s code is doing, I will suggest they use that instead.

Simplicity

Is the code simple to understand? If I look at a block of code and can’t easily see what it is doing, I may ask for it to be simplified or for comments to be added. Convoluted or complicated code is difficult to maintain and update.

Type annotations and docstrings

Does the code have type annotations? In a strongly-typed language, like Java, type annotations are mandatory for the code to compile. However, for weakly-typed languages, like python, you don’t have to declare the types of variables. However, for the code to be readable and maintainable, it is good practice to declare variable types in method definitions and class definitions.

Does the code have docstrings? Docstrings are comments explaining what each class/method does. Adding these also makes the code more readable and maintainable.

Coding Best Practice

Are there any coding best practice concerns? While the purpose of a code review is not to explicitly find bugs, a good code reviewer will look for code implementations that are not best practice and could therefore lead to bugs. For example, if someone has used a mutable default in python, it would be worth checking if that was intended, due to the (possibly) unexpected side-effects of their use.

Security

Are there any security concerns? For example: Are passwords or other secrets exposed into the code? Are we allowing unfiltered or unparsed external input into a database? Are we requiring any kind of authentication for the new API? Is the correct encryption being used to store passwords?

Error Handling

A crucially important part of software engineering, error handling is necessary to ensure that the software can handle errors gracefully, report problems in logs, and continue to run if something goes wrong. Unless new code has been entered in a space with an existing error handler in place, it will need error handling implemented. Even if an error handler exists, the new code could be introducing new potential errors, so that needs to be checked to make sure that the code is covering all bases.

Tests

Are there unit tests? Any code change means modifications to and/or additional unit tests are needed. Unit tests are meant to test a chunk of code in isolation from other libraries or code it may depend upon. They ensure that the specific chunk of code being tested does what the engineer intended it to do. Unit tests can catch bugs, but only if they are testing enough paths through the code. I would check if the unit tests cover enough cases given the code that has been added.

Are there database tests? If the code change affects the database, additional database tests will be needed. These are like unit tests, except they run with a copy of the database, to make sure that any interactions between the code and the database operate as expected.

Are there integration tests? If the code change affects the interface for an API, for example, integration tests will be needed. Integration tests, unlike unit tests, don’t test the code in isolation. The code runs fully and interacts with all its dependencies. This may even mean calling libraries or APIs that are not maintained by the team. However, integration tests are necessary to make sure the code works as a whole, when it is in the ‘wild’.

Version

Any change to the code will mean an update to the software version. The version number scheme that we use (and is adopted widely) has a format of three numbers separated by a dot, e.g.:

2.14.8

The first number is the major version. This only gets updated if a breaking change is introduced. A breaking change is a change to the code that will affect other code integrating with it. It usually means a change to the interface of the code package. For example, it could mean that the API no longer has a certain endpoint, as it has been replaced by a different endpoint. If the code being reviewed has such a breaking change, I will make sure that the major version has been updated.

The second number is the minor version. This gets updated if the code package has a new feature or substantial changes but no breaking changes. If the code being reviewed falls into this category, I will make sure that the minor version has been updated.

The third number is the patch version. This gets updated if a bug has been fixed in the code package. If the code being reviewed falls into this category, I will make sure that the patch version has been updated.

README

Software generally comes with a README document, with helpful tips on how to use the software, as well as a record of every change that has been made. The last part of the code review is to ensure that this document has been updated as appropriate to the changes that have been made.

 

So there you have it. A real code review. Perhaps we can re-write that scene from Russian Doll now:

INTERIOR: MEETING ROOM

MANAGER: Welcome to the Code Review!

ENGINEER ONE: Why are we in a meeting? We’ve all done our reviews on github already.

ENGINEER TWO: Yeah, and I found a bug in your code.

ENGINEER ONE: No you didn’t, because you’re not NEO.

ENGINEER TWO: Haha! Only joking. I just left a comment that you may need another unit test to cover the case where foo is true.

ENGINEER ONE: And I left a comment that you need to update the version number.

ENGINEER TWO: I always forget that!

ENGINEER THREE: Is there a point to this meeting?

MANAGER: Nope, not at all. Thanks all, see you in stand-up!

 

Much better, eh? I’m expecting a call from the makers of Russian Doll any day now…