Five golden principles of a code review
Hello fellow developers and engineers. Many of you do software development as part of your daily job. You work on tickets/issues/tasks, create branches, merge requests, and patches. How many of you do code reviews or give your code for others to review as a religious practice? Do every single line of code developed is reviewed by your team? If your company/team do not have the culture of code reviews, this is the right time for you to start it. If you have a casual code review process, then you should turn it into a serious one. In this article, I am going to explain why code reviews are essential for a software development team and how it helps big a team/company to have a stable codebase. At the end of this article, I am sure you will take something back to your team tomorrow.
I work at a middle-sized startup company in a pizza-sized team. We(our team) mostly write our code in Python(Django) and JavaScript. Initially, we create a new branch for each JIRA issue we work with. We commit our code every day and run our code through pylint(a pre-commit hook)before pushing our code to the remote branch. We have a strong CI setup which checks our code’s linting and, test coverage. We have a robust testing culture(min 90% coverage). If builds are unsuccessful, there is no way for a code-change to get merged into our development(main) branch. We create a merge request from our first commit and add a WIP(Work In Progress tag) so that other team members can see the progress of issue in terms of development. Once the coding job is done, we move the ticket to review stage and remove WIP tag from merge request.
Even after having a well-defined process in place, we care a lot for peer code reviews. At least two developers from our team will review the code and approve the merge request if everything is fine. Why? Can’t we take it for granted saying that code is PEP-8 compliant and tests cover almost 90% of the code?
The answer is NO! A cautious team should think there are many other vital factors beyond Continous Integration(CI) and code style. For a software project to be successful in the long run, a team should be cautious what goes into their codebase. We are going to discuss the five golden rules to keep in mind when a team member reviews her peer’s code.
If you are a developer who is undergoing a code review, this article can be a reverse guide for you to fix obvious mistakes in your code.
1. Consistency
As a code reviewer, always look for consistency. Consistency plays a major role in the maintenance of software projects. An inconsistency can cost a team with serious penalties while working with multiple projects in parallel. In the current culture of developing micro-services, teams tend to have a project per service rather than one single code base. When your team is hopping between multiple smaller projects, inconsistency can cause friction to the velocity of feature development.
These should be in your checklist while looking for inconsistencies in code:
- Is this merge request has changes that affect the consistency of overall project definition?
- Is this project having the same runtime environment (container, stand-alone) and runtime execution definition(docker-compose, Makefile) as existing projects?
- Is this project(merge request) is affecting the software design (entity vs behavioral project structure)?
- Do the merge request has code that has different naming conventions(camelCase vs snake_case etc) and module names from defined/standard ones?
If I have a hundred different ways to build and run a hundred projects just because of a developer’s laziness, it confuses a newly on-boarded developer in the team. So please maintain consistency at any cost.
2. Simplicity
There is a famous saying that Necessity is mother of all inventions but Simplicity is the father. All successful software projects are developed as simple and maintainable code entities. If a code reviewer cannot understand a merge request in a first glance, the code is not readable. If code reviewer after seeing the JIRA issue cannot figure out how the given code change can achieve the feature specification, the code is not pure enough to read. Many great people emphasized this point at a multitude of venues before.
The most important skill for a programmer is the ability to effectively communicate ideas.
— Gastón Jorquera
Readability is essential for maintainability.
— Mark Reinhold, JVM language summit 2018
Programs must be written for people to read, and only incidentally for machines to execute.
— Hal Abelson and Gerald Sussman in Structure and Interpretation of Computer Programs
So it is the responsibility of a code reviewer to check the readability of merge request. Below is the checklist to look for:
- Are you able to find out what code is doing on first glance?
- Is code achieving what it is promised as per the feature specification?
- Is the developer overkilling a simple change?
- Is the developer adding changes that are not part of the JIRA issue(Feature or Bug)? (Those changes should be separated as a different merge request)
3. Design Patterns
Design patterns are intended to make people stand on a common viewpoint. They may vary depending on the programming language and tech stack one work. For example, Java developers may religiously follow patterns from Gang of Four Similarly, Django developers can use class-based views instead of function based views. These kinds of patterns always evolve, and one should be aware of the best practices that the community is suggesting. Apart from best practices, one can re-use the existing code instead of re-inventing the wheel.
These should be your code review checklist while looking for design patterns:
- Check whether the code falls into a standard design pattern like a singleton or an abstract factory. Ex: Get a run time project configuration based on environment (Development, Staging, Testing, Production), A database connection which should be initialized only once.
- Do code is providing a sufficient abstraction for other modules/classes?
- Is the developer reinventing the wheel? Can he/she use a proper construct(Ex: LoginView in Django) instead of implementing a feature from scratch(login mechanism)
- Does the code deals with concurrent patterns like asynchronous callbacks(Python aysncio, JavaScript) and is it straight forward to know whether code execution path is deterministic?
4. Code Level Implications
This section is a vast topic, and many low-level details can be reviewed as part of a code review. Looking for the essential traits of good code is the theme for all points in this section. As a reviewer, you should check for code level best practices like performance improvements, identifying bottlenecks, and provisioning runtime debugging.
- Are database connections and file streams are correctly closed? Is there any chance for memory leaks?
- Does the code use inefficient data structures?https://wiki.python.org/moin/PythonSpeed/PerformanceTips
- Do all possible errors handled in the code? Do custom errors are created and used when built-in errors are too vague to identify system failure?
- Does code logs useful messages for runtime debugging?
- Do the developer positioned her newly written code at the end of a program or inserted based on context.
- Does alphabetical order maintained while defining variables/entities?
- Are there any edge cases where this code may fail?
- Do unnecessary libraries used(Ex: requirements.txt or package.json has unnecessary dependencies)?
These are only a few out of many things you can inspect and question about the code in a merge request. My advice is to keep an Excel sheet for all possible code issues you found by either reviewing code of others or from comments on your code. Another good way to see all potential problems that can creep into code is to go to a few open source projects(Django, Node.js) of your tech domain and read the comments on merge request. I am sure that you can learn a lot.
5. Quality of test & Documentation
Last but not least. Tests are low-level documentation of code. Even though if tests cover 90% of the code, there can be few caveats if you do not review changes carefully. For example, these things should be checked at the time of code review:
- Test coverage can show a wrong picture about overall test coverage(test files are mostly covered at 100%) if test coverage includes test files. So make sure the reviewing code exclude tests from the code coverage.
- Check whether tests are sticking to their responsibilities? For Ex: 1. A unit test should only test a function and mock all external entities it uses. 2. An integration test should use real entities like test database etc.
- Check the quality of tests than the number of tests written. It is to check whether tests are testing critical logic of the implemented feature.
Now comes the important thing. As we said code tests are documentation for developers at a shallow level. But high-level documentation is a project README file. Every project should have a README which has instructions about what the project does, how to run it, what is the runtime configuration supplied like ENV variables. So check the below things related to documentation:
- Check whether the code changes need an update of project’s README and developer has done it or not?
- Check whether the documentation is consistent with other projects?
- Is document now less readable?
More or less, these are the factors a developer from our team checks when she does a code review. You can take your code review skill to the next level by remembering these five golden principles in your mind.
As I mentioned before, If you wish to be a good developer who wants to get fewer code review comments on code she writes, crosscheck your merge request with above factors before submitting a code review.
Code reviews exist not only for the benefit of a project but for developers it is a wonderful opportunity to share knowledge and learn from others. So ask for sincere code review from your team next time you merge your feature request.
I hope you enjoyed my article. If you are doing something different while a code review, please comment below. I am eager to know. Have a good day.
