Code Review
My point of view and takeaways
Code review is an essential part of any software development process, and it can have a significant impact on the quality and maintainability of the codebase. However, the way code review is conducted can vary greatly depending on the team's size, experience, and the type of project being developed.
Tradeoffs
Sometimes I see a dogmatic approach that some people take towards code review, which can be time-consuming and resource-intensive. I think we need to adopt a pragmatic approach that recognises the tradeoffs every company must consider, especially smaller ones, deeply involved in scaling themselves and working iteratively towards their main goals, survive and grow.
By being more flexible in the way code review is conducted, companies can achieve a more efficient and effective process that is tailored to their specific needs and resources. This approach can help to strike a balance between maintaining code quality and productivity, which is critical for companies seeking to grow and succeed in today's competitive market.
In this blog post, we will explore some pragmatic approaches to conducting code reviews and highlight some key takeaways. You can apply just some of them, depending on the repository, system component, pull request, and seniority or code-specific knowledge of the engineer(s) involved.
Ease your life beforehand
Having pre-work done, such as setting up a CI/CD pipeline for testing, linting, formatting, measuring performances is critical in ensuring code quality, reliability, and maintainability. Also, it can be handy to leverage git hooks, where possible, to address a subset of these.
These tools help to automate and streamline the development process by detecting and fixing minor issues, improving code consistency, and reducing the reviewer effort, especially onboarding new engineers in the team that are not familiar with code style and best practices.
A well-designed CI/CD pipeline can catch issues early in the development cycle, enabling developers to address them quickly before they cause significant problems. By using these tools, developers can focus on writing high-quality code that meets business requirements, and reviewers can focus on the important stuff.
The role of submitter
The submitter of the pull request plays a critical role in ensuring the smooth and efficient code review process. To help facilitate this process, teams should provide clear onboarding instructions on what to do before submitting a pull request for peer review.
This can include expectations such as:
Sounds obvious but, documenting code with inline comments in the trickiest parts, as well as documenting changes in the pull request description (use templates!).
If there is no process in place for assigning a reviewer, the submitter can reach out to their team lead for guidance. Github can be configured to automatically assign them for you based on the repository.
Additionally, if the change is significant, it can be helpful to include a diagram. If you are developing or modifying a workflow or a non-trivial algorithm, a flowchart provides an immediate overview to the reviewers. As a side note, it’s a best practice to do this before the change, don’t need to say why.
By following these guidelines, submitters can ensure that their code changes are clearly documented, and reviewers can quickly understand the proposed changes, leading to a more efficient and effective code review process.
Reading, Understanding, and Running Locally
One of the most fundamental aspects of code review is to read and understand the code changes thoroughly. This involves analysing the proposed changes to ensure they align with the project's objectives, adhere to best practices and standards, and are well-documented.
Running the code changes locally can be essential sometimes, as it enables the reviewer to test the functionality and identify any potential issues that may not be immediately evident from a code review. Additionally, it helps to ensure that the code integrates seamlessly with the existing codebase and spotting issues in the development setup such as dependency problems or versions mismatching.
Level of Depth
Code reviews can vary in-depth, depending on the size and impact of the changes. For minor changes (e.g. renaming a local variable in a statically typed language), a brief review may be sufficient, while larger changes may require a more detailed review.
In general, and considering the assumptions above, code reviews should focus on the most critical aspects of the code changes, such as the logic, functionality, and performance. More on this later on.
Reviewers should also ensure that the changes are consistent with the project's style and guidelines whereas they can’t be addressed by a linter. In many languages exist dozens of ways to do the same thing, these is fun for sure, but can (and it does) reduce the overall readability of your codebase. I do really recommend, despite being technical and vertical on Go, the Golang FAQ to have a grasp of what I mean.
Number and “role” of Reviewers
The number of reviewers involved in a code review can vary depending on the project's size, complexity, and team structure. However, having at least two reviewers can provide a balanced approach.
They can play the same role, doing a both generic review, but from time to time I’ve found another approach working better, splitting their focus/time (which are limited) in technical aspects (language gotcha, readability, dev tools etc.) and business logic, especially where it is impacted by the change.
Having a subject matter expert on the project or repository can be particularly helpful in ensuring that the proposed changes align with the project's objectives and are consistent with the existing codebase. In some companies, for the same reason, a language-specific review (conducted by a more expert engineer) is required to achieve a good quality code and at the same time growing the language expertise of the submitter, which will not require this level of review later on.
Conclusion
In conclusion, conducting code reviews is an essential part of our engineering life, and of software development, it needs to be conducted with a pragmatic approach and considering the tradeoffs and specificity of our pull request, codebase, team and company objectives.
Teams should automated as much as possible from the beginning, in order to alleviate the burden of long and consequently weak reviews. Therefore, by prioritising the core logic, reviewers can work towards improving the overall team in both general aspects such as language, code style, and skills, as well as specific aspects such as the codebase, business logic, and team knowledge.