We’ve all been there. You’ve been working on a new project for a while and battling setback after setback. Just when you’re ready to call it quits and work on something else, you finally start to make some real progress. The code starts to come together and function as you had imagined. Success! It works now. Time to push all this code up, get it reviewed and wash my hands of it… Right?
The minute you actually start to put together functioning code is only the beginning. Even though this is a nice hit of dopamine and makes you feel accomplished, its also a signal that you should begin (or continue) to make preparations for actually packaging your code for review. That’s right, not only do you have to write the core functionality, you also have to write the supporting material for it. Documentation, examples, tests, benchmarks and more.
Throughout this article we’re going to cover some of the items you should be checking off your list before requesting a code review. This list is not exhaustive and may not be applicable to all languages, but is a good starting point for building your own “pre-flight” checklist to make your commits review-ready.
Disclaimer: I am not advocating that you don’t push your code until it is pristine, I’m suggesting that you run through this list before requesting someone review your code.
Lint your code
This almost goes without saying, but please lint your code before opening a PR. If you or your organization have an existing rules configuration for the linting tool of choice you should use that as well. Nobody wants to do the job of a code linter, there’s a reason these tools are automated. This only wastes time and prolongs the whole code review process.
Running linting tools on your code allows you to catch simple mistakes, syntax errors, poor styling and other small bugs. In most cases you can also automatically have the linter fix simple style violations. This cleans up your code before it even hits the reviewer’s eyes.
A lot of people forget to run these tools, so its a good idea to build them into your git process using hooks. Check out: PreCommit Hooks! by Vanessa Ating to learn how to setup your own git hooks for linting tools.
Here are some popular linting tools for multiple languages to get you started:
- Pylint (Python)
- RuboCop (Ruby)
- CheckStyle / FindBugs (Java)
- Clang-Tidy (C++)
- Golint (Go)
Run security analysis on your code
You might not even know you’ve just written a gaping security hole into your project, but a security analysis tool might. You should always run the most applicable security or static analysis tools on your code before opening a PR. This not only saves the reviewer (a human) from having to catch it, but also guards against the reviewer missing it as well.
Security issues are not always completely obvious and can be easy to overlook if you are strapped for time. Pushing up code that has not been analyzed could lead to security vulnerabilities being released into production. Things only get worse from there, so always remember to analyze your code.
Here are some popular security analysis tools to check out:
Confirm no sensitive data is present
Don’t leave plain text secrets in code. Especially don’t commit them and especially don’t open a PR with them. Testing (pre-commit) with a hard-coded secret is one thing, but bringing it into the code base is another very bad thing. Remember to always remove any secrets you tested and store them securely outside of the code base.
In the event you do accidentally commit a secret, you should invalidate it immediately, remove it from the code base and inform the appropriate stakeholders.
There are also some tools dedicated to secret scanning that you can add as pre-commit hooks or as a periodic task:
Test your code
I don’t mean writing tests for your code (although you should), I mean actually testing that your code works. This might seem silly at first, but I can’t tell you how many times I’ve seen untested code come across in a PR. Even if its a one-liner and incredibly simple, you should still give it a quick test. Humans are clumsy and even the smallest change could have a hidden mistake in it.
If you’re unable to test your code locally due to environmental constraints, you should look at actually writing test mocks or deploying to a development environment. Throwing your hands up and hoping it works in production is never the most correct solution.
Test your code on other machines
After you’ve made sure your code works by… running it. You should run it again. This time on a different machine, if you can. This is more applicable to backend code that could be deployed to disparate hosts, but an important step nonetheless. If you have another test or development environment at your disposal, try running it there.
When you test your code on different machines you’ll uncover any environmental impacts that might have been unaccounted for. Different language versions, missing binaries, etc.
Run any automated tests
If you or your organization have any automated testing for incoming commits in place you should run that before opening a PR (or from the new PR if automated checks are available). This can be a set of integration tests, code coverage reports, static analysis, etc. As long as you can pass whatever automated testing is in place you’ll remove one roadblock from your code review experience.
Opening a PR and requesting review when you haven’t even run automated tests is a quick way to delay your review and waste time.
Remove any commented out code
Code that is commented out doesn’t run. It doesn’t do anything but introduce confusion and doubt. Commented out code often gets left behind “just in case” someone needs it again or some new feature doesn’t work right. This is a bad practice only pollutes your code base.
If you’re worried about referencing old code, don’t comment it out and commit it. That’s what Git is for. Git remembers. Trust Git.
Assess the size of your PR
Nobody wants to try and read a 2,000 line PR that touches 75 files. It is untenable to even complete an effective code review with a PR of that magnitude anyway. The reviewer will be overwhelmed and unable to spend enough time critiquing the code adequately. Your overall code review will suffer and you won’t get quality, constructive feedback.
Before opening a PR and requesting review you should examine the overall size of the PR. There is no hard rule about at what number of lines or files you should break things up, but let common sense be your guide. If it seems like even you don’t want to read through it then it is probably too large.
If you submit a PR that is too large the following things will happen:
- The review will take ages to complete due to size
- The review will be less effective due to lack of dedicated time with each individual change
- There will be a higher likelihood of bugs or other issues slipping through due to the sheer volume of code to cover
- Your reviewer will resent you
Read your code
Last but not least, please take some time to read through your code. This is extremely simple and a wonderful habit to build. Once you’ve ticked all the other boxes and you’re ready to push this code out into the ether make sure you sit down and read through it.
I suggest reading the code in a different editor, or directly in your code review tool, like GitHub or Gerrit. This is a great way to get a different perspective and view the code with slightly different syntax highlighting, formatting, etc. You may uncover even more minor issues you were unable to see before, and as a bonus this let’s you read the code from the reviewer’s perspective as well.
Thanks for reading! With the help of these simple pre-review tasks hopefully you can build a more efficient and effective code review process for yourself and your teammates. Check out Code Reviews Should Be Brutal for even more tips on exploring the review process.