As developers, we are all too familiar with code reviews. Having another pair of eyes take a look at our code can be wonderful; it shows us so many aspects of our code we would not have noticed otherwise. A code review can be informative, and it can be educational. I can confidently attribute most of what I know about good programming practices to code reviews.
The amount of learning a reviewee takes away from a code review depends on how well the review is performed. It thus falls on the reviewer to make their review count by packing the most lessons into the review as possible.
This is a guide on some vital aspects of the code you should be checking in your reviews, the expectations you should have from those checks, and some ideas on how tooling (such as linters, formatters, and test suites) can help streamline the process.
Code review checklist
We have formulated this guide in the form of a checklist. It lists a set of questions that you need to ask about the code. If the answer to any of them is not a ‘yes’, you should leave a remark on the PR.
Do note that this list is only meant to serve as a guideline. Your codebase, like every codebase, has its own specific set of needs, so feel free to build upon this guide and override pieces of it that do not fit well for your use-cases.
Etiquettes
The first and foremost thing to check during a review is how closely the PR adheres to basic etiquettes. Good PRs are composed of bite-sized changes and solve a single well-defined problem. They should be focused and purposefully narrow to have as few merge conflicts as possible. Put simply, a good PR facilitates being reviewed.
For large-scale swooping changes, make a separate target branch and then make small incremental PRs to that target, finally merging the target with the main branch. Making one humongous PR makes it harder to review, and if it goes stale, many merge conflicts may pop up.
When reviewing PRs from new developers, I also make it a point to ensure their commit messages are well-written.
Checklist:
- Is the PR atomic?
- Does the PR follow the single concern principle?
- Are the commit messages well-written?
Ideas:
Enforce a commit message format in the team. For example, you could try gitmoji wherein you use emoji in commit messages. For example, bugfixes should start with [FIX], or the 🐛 emoji and new features should start with [FEAT] or the ✨ emoji. This makes the intention of the commit very clear.
Functionality and syntax
The next thing to check is whether the PR is effective in that it works. Code changed by the PR should work as expected. A bug fix should solve the bug it was supposed to fix. A feature should provide the additional functionality that was required without breaking something else.
An important thing to keep in mind is that any new feature added by a PR is justified. A simple way to ensure this is to accept only the PRs that are associated with an already triaged issue. This practice minimizes feature-creep.
Checklist:
- Does the PR work?
- Does the new feature add value or is it a sign of feature-creep?
- Does the PR add test-cases for the modified code?
Ideas:
Having comprehensive tests in the code makes it easier to check that new functionality works and harder for PRs to break existing stuff. The portion of the code covered should never go down in a PR. Any new code should come with complete coverage in unit/functional tests. Python comes with a robust unit testing framework built into the language itself. You should make use of it in your codebase.
Design
Once we’ve established that the code works, the next step is to check how well it integrates with the existing codebase. One key thing to inspect in this regard is duplication. Often, code added to a repo provides functionality that might already be a part of the code in a different location or something provided by the frameworks or Pip packages in use. This knowledge is something that one can only gain by experience. As a senior, it becomes your duty to point such duplication out to new developers contributing to your repo.
Attention to paradigms is also essential. Many projects have pre-adopted design paradigms such as microservices, mono repo, or cloud-nativity. Any incoming code should be in line with these paradigms.
Checklist:
- Is the code properly planned and designed?
- Will the code work well with the existing code and not increase duplication?
- Is the code well organised in terms of placement of components?
Patterns and idioms
Python is a very mature language. It is driven based on certain philosophies, described as the Zen of Python. Those philosophies have given birth to many conventions and idioms that new code is expected to follow.
The difference between idiomatic and non-idiomatic code is very subtle, and in most cases, can only be intuitively gauged. As with all things honed by experience, intuition must be transferred from experienced people, like yourself, to newbies like your reviewees.
Checklist:
- Does the code keep with the idioms and code patterns of the language?
- Does the code make use of the language features and standard libraries?
Ideas:
Most linters, popularly PyLint, can help you identify deviations from the style guides and, in most cases, even automatically fix them. Linters work incredibly fast and can make corrections to the code in real-time, making them a valuable addition to your toolchain.
Readability
Python is widely regarded as a very readable language. Python’s simplicity of syntax and reduced usage of punctuation contribute heavily to its readability. It only makes sense for code written in the language to be readable as well.
Checklist:
- Is the code clear and concise?
- Does it comply with PEP-8?
- Are all language and project conventions followed?
- Are identifiers given meaningful and style guide-compliant names?
Ideas:
A good code formatter like Black can help a lot in formatting the code for consistency and readability. Black also offers minimal customization, which is good because it eliminates all forms of bikeshedding.
We’ve previously talked about integrating Black into your CI pipeline can work wonders during a code review.
Documentation and maintainability
The next thing to check is the maintainability of the code. Any code added or changed by the PR should be written to facilitate someone other than the original author to maintain it.
It should preferably be self-documenting, which means written in a way that anyone reading the code may be able to understand what it does. This is one of the hallmarks of good Python code. If the code has to be complex by design, it should be amply documented. In an ideal world, all classes and functions would have Python docstrings, complete with examples.
Checklist:
- Is the code self-documenting or well-documented?
- Is the code free of obfuscation and unnecessary complexity?
- Is the control flow and component relationship clear to understand?
Ideas:
Sphinx is a documentation generator that exports beautiful documentation from Python docstrings. The exported documentation can then be uploaded to ReadTheDocs, a popular doc-hosting tool. Sphinx is one of the main reasons why I absolutely love writing documentation.
Security
Ensuring that the application remains secure is critical. The next thing to check is if the PR maintains or improves the security of the project. You need to ensure that the changes do not increase the attack surface area or expose vulnerabilities. If the PR adds new dependencies, they could potentially be unsafe, in which case you might need to check the version for known exploits and update the dependencies, if necessary.
Checklist:
- Is the code free of implementation bugs that could be exploited?
- Have all the new dependencies been audited for vulnerabilities?
Ideas:
One of the renowned security analyzers for Python is Bandit. Also, if you use GitHub for hosting code, you should absolutely read this guide about setting up vulnerability detection and Dependabot for your codebase.
Once you set up vulnerability detection on GitHub, you’ll get notifications like this…
… with details about the vulnerability and PRs in your repositories that you can merge to patch them.
We’ve also recently talked about common security pitfalls in Python development and how you can secure your projects against them.
Performance, reliability and scalability
The final things to check are the performance and reliability of the code at scale. While these are undoubtedly key metrics, I put them on the bottom of the checklist because I believe well-planned, well-designed, and well-written code generally works well too.
Checklist:
- Is the code optimised for in terms of time and space complexity?
- Does it scale as per the need?
- Does it have instrumentation like reporting for metrics and alerting for failures?
Ideas:
An excellent way to have add some reliability to Python is to use type hinting and static type checking that can provide hints into possible errors before runtime. Python new native support for type-hints is primarily inspired by the Mypy syntax, which can be incrementally adopted in existing projects.
DeepSource
DeepSource is an automated code review tool that manages the end-to-end code scanning process and automatically makes pull requests with fixes whenever new commits are pushed or new pull requests.
Setting up DeepSource for Python is a quick, easy, no-fuss process. Just add a .deepsource.toml file in the root of the repo, and immediately DeepSource will pick it up for scanning. The scan will find scope for improvements across your codebase, make those improvements, and open pull requests for the changes it finds. I was blown away by the simplicity of the setup and the efficacy of their self-built code engine.
🙋♂️ Trivia: Did you know that DeepSource holds the distinction of being on OWASP’s curated list of source code analysis tools for Python?
The ideal code review
Returning to my point about code reviews being educational and informative, I would also like to add that code reviews are time-consuming and resource-intensive. While each review takes time, the more in-depth and comprehensive ones consume even more.
So each review must be as productive as possible. This is where all the tooling and automation efforts should be directed. By automating whatever can be automated, which is generally the mundane parts of the review, such as code style and formatting, we can allow devs to focus on the important stuff like architecture, design, and scalability.
I’ll leave you with a profound thought my mentor shared with me.
A good code review not only improves the code but the coder as well.
Top comments (1)
Thx for this! This is really what I wanted. Helped A LOT.
Can I translate in Korean this article? If you don't mind, I wanna share this awesome information in Korean. Surely, There will be a link directing to this original one.