Code review is not a battlefield, the reviewer is not an opponent of the author. Both of them are aligned on the same goals - solve product problems and make a high-quality codebase. Let's dig deeper and understand how to make an unforgettable code review from the reviewer's side.
Do not waste time
There are questions that are repeated from time to time. First in one pull request, then another, once from one author, then from another. They are absolutely identical - this is a routine. And as every routine should be, it must be automated, and indeed, if something can be automated, then it must be automated.
Code style. There is no point in fighting and arguing over the code styles, which were 100% already defined many-many times and decades ago by everyone on a project, or by the entire community. Set the length of strings, names of methods, and classes in linter and formatter and forget about it.
Tests. Automate all tests (unit, integration, end-to-end tests), their launch, coverage, and other things. Just set them up, and set an acceptable threshold for metrics, for example, the coverage of new code in the PR should not be less than 90% and this simple setting makes life easier for many people.
Do not repeat yourself (DRY). Common repetitive things should be brought to one place for easy, centralized changes or fixes to them. Collect code duplication percentage across all applications and handle it in the same way tests.
Code analysis. Code analysis would help collect more data and metrics to work with. It would check not only the code on review but also how it would be integrated into the existing ecosystem. Some of the analysis tools would provide a report on possible vulnerabilities or security hot spots based on historical cases. Integrate the repository with code analysis tools and run them on each code review.
Summary. Engineers should be concentrated on solving problems, not repeating routines. The thing that I could guarantee, configure at least basic automatization of any topics above and the average quality of code review would dramatically increase. It would save time for reviewers cause why it needs to check the code if:
- Not all tests are passing
- Not enough test coverage / lower than accepted percentage
- The code duplication percentage is higher than acceptable
- Code “smells”
- Unexpected security hotspot
Generic rules
Respect. Be polite and respect an author. Remember code review participants are here to help each other and share the same goals. It is possible to criticize the code and never a person.
Task. Do not make a code review until fully understand what was solved. Get information about what was the problem, real cases, conditions, steps for reproduction, the audience of the feature, etc. Ask for code demos, code review sessions, or even just synchronization meetings to clarify all aspects of a given task.
Proposal. When proposing anything to change, explain the reason. Provide background, examples, details, and information, and share links on resources - why the author should make that update. One-word or one-liner comments sometimes are really hard to understand. Keep in mind usually tasks could have multiple solutions, so before suggesting a change try to understand why exactly this solution was chosen. Using commit history, and structured commits with good messages could give a key to understanding.
Review flow
Below I have collected some fundamental categories and questions that are really worth paying attention to in order of their priority from the most important.
Business goals
Look in-depth, look not just at the code but at the business logic behind it. First of all, any code must perform the assigned tasks and achieve the specified goals. No matter how good the code is, no matter how well it is written, if it does not fulfill its goals, it is useless. Code is not written for the sake of code. Code is written to add new functionality, to develop and move the product forward. Do not limit checks only to happy paths, think about edge cases and how they are handled. All could be generalized in the question - does it solve the issue?
Implementation
On the next level, start paying attention to the numbers, metrics, and reports. Analyze code from different angles.
Security. Does it bring or solve vulnerabilities? How stable would it be under attack? Passive or active? Some specific like DDoS or any kind of injections (SQL, Cross Site Scripts, etc)?
Error handling. How is proper error handling done? Would the application crush or send a report to error tracking software? Would it show all stack traces to the end user? Is it retriable failed action? Would data be corrupted, or collision?
Performance. Was performance affected after the new changes? Possibility of memory leaks caused by this code? How good is optimization? Does everything was done to name this code efficient (cache systems, resource pooling, data compression, etc)
Integration. How would it work with other modules and systems? Does it increase coherence in code? Could it be easily swapped with other implementations or integration? How compatible is code with other versions of other parts of the system (e.g. backward compatibility of a new release)?
Logging and tracing. That is the thing that could simplify or make a more complicated process of debugging and troubleshooting. For e.g. logging response time from integrated third-party service helps to identify its downtime if it happens. Are there enough logs and traces? Neither more nor less?
Going through this category and answering those questions should confirm the right choice of tools for implementation.
Maintainability
Here one of the main questions would be - “how would code live without an author?” whether the author will be legendary or cursed.
Readability. Code is letters that are building words, words that are building lines, and so on. And all code sounds just like a classic book but written in a specific language, programming language. So readability should be understood just literally, code should build a story (e.g. class, function) with well-written characters (e.g. arguments, variables, etc) and they should take action (call other functions, mutate or be immutable, etc). How readable is the code? Could it be maintained other than by the author? Intelligibility of named arguments, variables, functions, etc.
Documentation. It saves a lot of time during development, reduces time on synchronization, simplifies the onboarding process, and in general good storage for the knowledge base about the project. How good is code documented? Does it leave questions after reading? Do all needed MD files, and external documentation add/update according to changes?
Reusability. For preventing code duplication when logic could be common for more than one module it could be moved to shared places like helpers, utilities, etc. Could some parts of the code be reused somewhere else? If not, is its uniqueness justified? If yes, was it not over-engineered to reach this metric?
Design. Code should never reinvent a bike. There are dozens of already accepted by community best practices and defined design patterns. Solutions to common problems in software engineering. Does code utilize best practices and patterns? Is it utilized in the right way?
Impression. Code should encourage anyone and everyone who in one way or another intersects with it now or in the future to strive to do just as good and quality or even better. Does the codebase become better after the merge? Would other engineers be excited to work with this code?
Conclusion
Doing a good code review is a lot of work. Reviewer is the very first tech quality gate for it. Before merging, code is owned and managed by its author, but after this responsibility will be transferred to the entire team. That is why reviewers should be interested in transferring a stable, reliable, and impeccable state of code. Code review is an opportunity for growth. Growth for both reviewer and author.