Yext Engineering does pre-commit code reviews to assist knowledge sharing among developers and to increase the quality of the resulting software. This article presents a few tips that we try to abide by when planning our work and responding to reviews.
Submitting code for review
When do you know when your work is ready to submit for review?
The ideal changeset:
- is not too big - code reviews beyond 250 lines will result in a significant disruption to the reviewer’s day and reviewers have more difficulty giving the same attention to each line.
- is an atomic improvement - it should stand on its own as a new use case, an incremental improvement to an existing use case, or a new library / function.
- is complete / ready to check in - don’t waste your reviewer’s time by uploading early while you continue to test it or work on it.
- has been self-reviewed - have you looked at it in the review tool to see if unrelated changes (e.g. formatting) snuck in?
- has been tested - have you exercised the functionality and edge cases? does your change merit new tests?
The first item is especially important. Good code reviews are typically 50-250 lines of code, encompass a day or two of work and implement a self-contained improvement (perhaps with unit tests). Some tasks will be larger, and it is then the engineer’s responsibility to break their work into reviewable pieces. Other tasks will be smaller in which case the code review can be the complete implementation.
It can be extremely tempting to avoid the overhead associated with planning your commits, but it is a trap that can result in gigantic code reviews (difficult for the reviewer), are impossible to parallelize (someone on your team finishes their part but can’t help with yours), and may take an indeterminate time to complete (they may drag on far longer than you expect). It is much better to use a lightweight process (e.g. a gist) to convey and debate the overall design, and then make small incremental commits to get there.
The code reviewer should address the following points
Correctness: Does the code do what it claims to do? Is the code correct in both the nominal case and the boundary cases? As a reviewer, this is your opportunity to point out edge conditions of which the original developer may not have been aware.
Complexity: Does the code accomplish its task in a reasonably straightforward way? If you can point out simpler approaches that do not compromise the correctness or performance of the code, you should.
Consistency: Does the code achieve its basic goals in a way that is consistent with how similar code in our codebase achieves those goals? Is it re-using the available libraries and utility classes? Where possible, has code been refactored for re-use instead of just copying and pasting?
Maintainability: Could the code be extended by another developer on the team with a reasonable amount of effort? More than any item on the list, this is the karma investment you make by doing code reviews - the code you review today may be the code you have to update tomorrow, so taking the time to make sure it’s maintainable by others pays itself back to you.
Scalability: Will the code be performant at the expected volumes? It is important that this question always be asked in the context of expected volumes. When building a new product in an untested market, it is fine to write code that works for 10,000 users but not 1M; if the product should be that successful, we will profile, optimize, and, when necessary, re-write the critical bits. The corollary is that we should not spend time optimizing code when the market demand is unproven.
Style: Does the code match the team style guide? This should rarely be controversial.
The code reviewer should not address the following:
Scope or mission feedback: “I don’t think you should be doing this project” is almost never a useful comment for a code review. If you think the team is embarking on projects that are not worthwhile, that is great feedback to share, but not in the context of a code review. The exception here is if someone is introducing a new way of doing something that is already well-handled in some other way.
Design review: A code review is not the time to evaluate the overall design of a project. For example, “I don’t think you should be using the DB to store this data” is not useful. Of course it is incumbent upon the developer to have their designs reviewed before implementation, and there will be scenarios in which the fundamental design is questioned during the implementation, but for a project that has been through a design review, let the results of that design stand. If design issues frequently pop up in code review, the team should discuss the reasons for that and find a design process that works for them; it could be as simple as a whiteboard discussion, email, or shared document, as long as it precedes the main implementation.
Personal preference: “I would rather you do it my way” is an invitation to an unproductive debate. If you have a way that is demonstrably better, you should always argue for it, but if you find yourself trading hypothetical scenarios in which your solution is better than the one already implemented, with no way of determining the likelihood of said scenarios, the default is to use what the developer has already written.
These guidelines help reviewees and reviewers alike get the most value from this process.