Pull Requests
As per the methodology set out earlier, at Built By Cactus, we work via pull requests.
If the repository has been set up correctly, then you won’t be able to push to the master or develop branches. This
means that all code will need to be merged via a pull request.
Why use pull requests?
Pull requests are good for two reasons, one they allow us to run status checks on the code before it’s merged with the pre-existing codebase. Secondly, they give the team a chance to review and comment on the code and approach to the problem before merging.
What should a pull request look like?
The idea of the chosen Git branching methodology is that work is broken down into manageable chunks and each of those chunks is developed in its own branch. Because of this, the pull requests should be reasonably small.
We’d hope that pull requests contain code that only edits a few lines or files. The fewer the better as it makes it easier to review. You should set some soft limits for these, I’d suggest a maximum of 30 files (that’s pretty big) and no more than 400 lines (also pretty big). If a pull request grows bigger than that, then it generally won’t get a proper review as too much is going on.
What to look for when reviewing a pull request?
You should be on the lookout for anything that doesn’t seem right. Some that I look for are below:
Functionality
This is the most important thing to look out for, does the code solve the problem it was intending to solve?
Mostly, we expect engineers to test their code well-enough that they work correctly by the time they get to code review. However, as the reviewer you should still be thinking about edge cases, looking for concurrency problems, trying to think like a user, and making sure that there are no bugs that you see just by reading the code.
Potential security issues
- Not escaping output
- Not sanitising user input
- CSRF issues
The approach taken to solve the issue
Is there a better way of doing it? Could we make it more performant or reduce the LOC?
Overcomplicating the codebase.
Sometimes developers try to be too clever, this can complicate the codebase and make other developers afraid to refactor. Unless there’s a need for micro-optimisation then always lean towards readability over tiny performance gains. It’s more important for the team to know why a piece of code exists.
Tests
If the project is tested (as it should be) then with the change of the codebase, there should be a change of the tests also. Unless of course the PR was to fix a bug that the tests caught or is a refactor that doesn’t change the functionality.
Naming
Did the engineer pick good names for everything? A good name is long enough to fully communicate what the item is or does, without being so long that it becomes hard to read.
Good Things
If you see something nice in the pull request, tell the engineer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, in terms of mentoring, to tell a developer what they did right than to tell them what they did wrong.
Summary
In doing a code review, you should make sure that:
- The code is well-designed.
- The functionality is good for the users of the code.
- Any UI changes are sensible and look good.
- The code isn’t more complex than it needs to be.
- The engineer isn’t implementing things they might need in the future but don’t know they need now.
- Code has appropriate tests.
- Any tests are well-designed.
- The engineer used clear names for everything.
- Comments are clear and useful, and mostly explain why instead of what.
- The code conforms to the appropriate style guides (CI can usually do this).
Make sure to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment engineers on good things that they do.