How to review changes¶

As part of our software development process at Beautiful Canoe, all code, without exception, goes through peer-review before it is merged. In general, your code changes will be reviewed by the CTO, but as part of your experience here you will be asked to review code that other developers have written. Code review is not a straight-forward process, and there are a number of technical skills and other competencies that you will need to practice to learn to review well.

Technical aspects of code review¶

The purpose of code review is to ensure that any code we merge into a develop branch (and more so with ~"Hot Fix" changes) is carefully scrutinised to avoid adding bugs, or technical debt to the code. Many merge requests will bounce back and forth between the author and the reviewer before they can be passed to @bc-bot to be merged, and it is quite common for MRs to receive a large number of comments requesting changes during this process. If you are reviewing a code change, there are several aspects of the code to think about, but in general you should be asking yourself the question does this code really work, correctly, in all possible cases, and will future developers be able to understand and extend it?.

Issues and clients¶

Firstly, give some thought to the functional aspects of the code. Do you understand the original issue, that this MR is intended to resolve? If not, ask for clarification.

Does the MR actually resolve the issue? This is something that you should check, ideally by running the application yourself, and walking through the steps to reproduce the original bug, or work through a feature request.

Tip

If you are reviewing code for a static website, such as this site or the company website, your MR should have a button titled View app which will show you a live version of the code in the MR, without you having to serve the site locally.

Secondly, is the new code an improvement for the client? This may be clear from the description in the original issue, but it's worth thinking through how a real user would experience the product. Can it be streamlined? If the product is a website, is it responsive -- you can check this with Chrome Developer Tools? Are there a small number of steps to take to complete the relevant use case, or is there unnecessary complexity?

If you find problems at this stage, feed them back to the author.

Well-factored code¶

Once you are happy that the functionality of the code has been improved by the changes, you should also think about the non-functional aspects of the code.

Every line of code adds technical debt to a project and has to be maintained by future developers.

Well-factored code, is code that has been organised well into modules, packages classes, methods, functions, and other scopes that are available in the language you are using. In general, if there is a standard way of structuring a piece of code (for example using MVC in a web application), then the developer should use a standard technique. If the code is structured in an unusual way, or if the code can be split into smaller, reusable scopes, then it probably should be. Equally, repetition in code should be avoided -- if code has been copied and pasted, factor it out into a function or method.

One obvious reason for factoring code well, is that smaller units of code will be easier to read for future developers. A related reason is that it helps to ensure the code is self-documenting. For example, this Ruby code is used by @bc-bot to create a label like ~Missed:2019.1 from a milestone name such as 2019.1 Sprint:

    ...
    sprint = milestone['title'].tr(' Sprint', '')
    new_label = "Missed:#{sprint}"
    ...

In the real code base, we have factored this out into methods:

def get_sprint_name(milestone)
    milestone['title'].tr(' Sprint', '')
end

def get_label_name(milestone)
    sprint = get_sprint_name(milestone)
    "Missed:#{sprint}"
end

This avoids repetition later on, when we want to create a description for the new label:

def get_label_description(milestone)
    sprint = get_sprint_name(milestone)
    "Missed the end of milestone #{sprint}"
end

and the method names clearly document what each method is intended to do.

However, there is a third reason for factoring out these methods: once they have been separated from their context, they can be tested with unit tests.

Well-tested code¶

New projects should always add unit tests as new code is added to their repository. Ideally, we want code coverage in each repository to be as close to 100% as possible. In reality, there may well be parts of a code base that are not useful to test, for example code that has been auto-generated by a framework. Wherever possible though, new code should come with new tests.

In the longer term of the project, this means that when old code is refactored, we can have some confidence that new bugs have not been introduced.

For legacy projects, which were started without tests, we should add unit tests wherever possible. New code without tests should usually be rejected.

Manual testing¶

Before a developer assigns a reviewer, they should have tested their code manually, and the reviewer should do the same.

Note that some aspects of the code may be difficult to test automatically and are easily missed by developers. If you are reviewing a project with a web-based deployment, it is worth keeping the Javascript console open at all times, to check that there aren't any Javscript warnings or errors.

Javscript console

Well-commented code¶

It is often difficult for new developers to know when it is useful to add code comments and when it is not. As a rule, the more self-documenting your code is, the fewer comments you need. If you think that a piece of code cannot be understood without comments, think about whether it can be refactored into code that it easier to read.

However, there are several times when comments are really essential:

When a piece of code has unusual performance characteristics, which anyone calling the code should know about.
When the author has thought about a number of difficult trade-offs in the code, and has decided to make a design choice that might not be obvious.
When a piece of code has to be written in an usual way to work around an upstream bug or a quirk in an API.

Notice that all these cases are times when the comment is communicating something that cannot be known from the code alone.

Comments should never be used to communicate TODOs, or aspects of the code that need to be fixed in future. Using comments for this purpose means that they will only be seen by developers looking directly at the relevant bit of code, which is particularly problematic when developers work on short-term contracts. Instead, please raise an issue in the relevant repository, so that the work left over can go on the %Backlog and be scheduled appropriately.

Coding conventions¶

In order to be readable code needs to also be consistent.

Programs must be written for people to read, and only incidentally for machines to execute. ― Harold Abelson

We should use 4-spaces for indentation, remove trailing whitespace, and stick to common coding conventions, such as PHP PSR2. Ideally, pipelines should use a lint, such as phpcs to check this automatically, but it should certainly be commented on in review.

Soft skills in code review¶

Code review is an unusual activity, because it requires you to adopt two very different attitudes at once:

You need to be adversarial, in the sense that you are asking the author of the merge request to convince you that it is correct, that it meets the requirements of the issue it hopes to resolve, and that is an improvement on the current code base.
You also need to be collaborative, in that you are collaborating with the author to produce the best possible code that they can write.

Many of your code review comments will be critical (we only use 4-space indentation at Beautiful Canoe! this change hasn't been tested!) or questioning (why doesn't this code ...?), so it is worth thinking about how you phrase these comments, to best help the author improve their work. How would you want your reviewers to communicate with you?