Code Review: Best Practices

Why Code Review is Important

At Waverley, we understand that writing high-quality code is absolutely necessary.  Better code ensures stable and seamless software performance, saves time, makes it easy to expand and scale systems, allows for realistic planning and meeting of deadlines. Better code in software development means fewer bugs, higher customer satisfaction, and a lot more efficient development process.

Regular code reviews are especially important on long-term projects. Poor quality code decreases the overall development speed since more time goes into knowledge transfer and fixing bugs. We do regular code reviews to ensure the highest code quality.

When performed correctly, code reviews can help reduce the number of bugs, improve software performance and help the team navigate the code better. However, if developers do not follow code review best practices, code reviews can be misleading, slow the team down, distract developers and, in some cases, be a total waste of time. Based on our experience, we’ve prepared a software engineering code review checklist to help you introduce (or improve) the code review procedure at your company.

What

Experience has taught us that code review can have a significant impact on the code quality. So what is a code review and how does the code review procedure take place? It is a process, integrated into the development flow when one or more individuals who haven’t been involved in the code creation review the source code of an application after implementation and provide feedback. 

The benefits of formal code reviews and regular static analysis are:

• Early-Stage Bug Identification.  It helps to find bugs in source code when they are still easy to fix.
• Сoding Standards Compliance. Code review helps to maintain a consistent coding style across the company.
• Teaching and Knowledge Sharing. During the code review, team members gain a better understanding of the codebase and learn from each other.
• Consistent Design and Implementation. Peer code review helps to maintain a level of consistency in software design and implementation.
• Higher Software Security. Applications that require a high level of security benefit from targeted security-focused code reviews.
• Team Сohesion. Code review discussions keep team members from becoming isolated and bring them closer together while affording the opportunity to share knowledge and experience.
• Stakeholder Confidence. Stakeholders have more confidence in the technical quality of the product.

How to Conduct Code Reviews? Best Practices

So how does the formal code review processes unfold at Waverley? Every task on a project has a life cycle that follows a pattern:

1. Development Starts. At this stage, a developer creates a branch and starts implementing the logic.
2. Development is Over. At this stage, a developer finishes an initial version of his changes and submits everything for review.
3. Review Process. At this stage, our software engineering team inspects the code, asks for changes if needed. After all feedback has been processed, the changes are merged into the main branch and the task is finished.

Let’s look at each stage in detail and outline some code review best practices, guidelines and ways we make the review process more efficient. 

Development Starts

The developer has to make sure that the branches are named according to the convention.

One branch should include only one task. This approach makes managing tasks easier. Also, every task can be merged or put on hold without any effect on the others. Long-term tasks are split into subtasks. This prevents reviewers from time-consuming reviews.

Development is Over; The Work is Sent for Code Review

• The developer must ensure that their code builds and works
• The task is sent for review by making a pull request.
• The developer should review the PR himself first to find and fix obvious errors and save the reviewer’s time.
• The PR title should be meaningful and include the ticket from the issue tracking.
• Details should be added to the PR description if necessary.
• The developer must ensure that CI checks are passed.
• The developer should notify the team about the PR.

The Code Review Process: Best Practices

During any code review, the reviewee:

• Has to respond to all comments (example: “fixed”, “won’t fix” or “doesn’t need to be fixed”).
• Can merge the PR only if all comments have been resolved.
• Needs to get a PR approval from 2 reviewers.
• Needs to get a PR approval from their teammate, if they work in pairs.
• Can only consider the PR Approved if a reviewer has added the “Approved” comment.  

During any code review, the reviewer:

• Should verify that every piece of code follows the Waverley’s Recommended Code Quality Criteria (see below).
• Marks the request “Approved” if, after reviewing, there are no comments or all comments have been resolved. The changes are not considered to be “Approved” by the reviewer without this action.
• It is recommended to review requests within 30 minutes after they were made. However, the reviewer should avoid disruptions to their own work, if the requests are too frequent.
• Must not approve a request without looking into it. To do so would make the review process no more than a formality and contradict the purpose, which is to find and fix potential bugs, thus improving quality.

Code Review Checklist: Waverley’s Recommended Code Quality Criteria

Here is a short code review checklist, a list of the aspects our development team has to pay attention to while conducting a peer code review:

• Code readability
• Security
• Bugs, edge cases
• Errors processing
• Logging
• Input data validation
• Stability
• Compliance with requirements
• Reverse compatibility
• Code smells

Code Readability

Methods and Functions

• Does the method name have the form “expressive verb + object”? 

getName, sendEmail, printMessage, saveUsers

• Have you chosen verbs and nouns that you prefer to use on the project? Stick to them. Examples:

add/remove
increment/decrement 
open/close 
begin/end 
insert/delete
show/hide

create/destroy
lock/unlock
source/target
first/last

min/max 
start/stop 
get/put
next/previous
up/down
get/set

• Is it clear from the method name what type of data it returns? For example:

isValid - boolean
getUser - User
sendMessage - nothing or true/false

• Are there too many parameters for the method? If there are more than 4 parameters, it’s a good indicator that the method should be grouped in a different way.

sendMessage (to, subject, body, ...)
sendMessage ({ to: ..., subject: ..., body: … })

• Are parameters ordered in the same way as in other similar methods? Example of inconsistent order:

logError(errorType, data)
logWarning(data, errorType)

• Does the method name describe all the actions it performs?
• Is the method too big? The rule of thumb is that a function should be less than 20 or so lines. If a method is above 50 lines, it is best to cut it into smaller pieces. (Can be automated.)
• Is the method doing only one thing, but doing it well?  (single responsibility principle)

• Is there a logic that should be extracted as a separate method?
• Does the method change the environment, global variables, or input data? Is it obvious that the method does this?
• Does the method return the correct value in all possible cases?

Variables

• Does each variable have one and only one goal?
• Are the variables named according to the convention (camelCase, kebab-case, etc)?
• Does the variable name describe the represented entity fully and accurately?
• Does the name of the variable have a length sufficient to ensure that no one will be puzzled by it?
• Does the name of the variable make it easy to understand what kind of data it keeps?
• Are there any magic numbers or strings?

Data Type Conversions

• Are type conversions obvious?
• If variables of two different types are used in the same expression, will it be calculated the way you expect it to be calculated?
• Is there a comparison of variables of different types?

Classes

• Does the class have a central purpose?
• Is the class well named and does its name describe its central purpose?
• Is the class independent of other classes? Is it loosely coupled?
• Does the class collaborate with other classes only when it’s absolutely necessary?
• Does the class hide its implementation details from other classes as much as is necessary?

Logical Checks

• Are there complex logical checks? Can you use variables or functions to improve readability?

Data Structures

• Do you use data structures instead of variable sets to organize related data? 
• Did you consider class creation as an alternative to using structure?
• Do you use Enum types where appropriate?

Conditional expressions

• Are complex checks converted to calls of logical functions?
• Are the most likely cases checked first? Are all options considered?
• Does the method contain more than 10 conditional expressions? Is there a good reason for not redesigning it?

Recursion

• Does the recursive method contain code to stop recursion?
• Does the method use a security counter to ensure that execution will be completed?
• Does the recursion depth match the constraints imposed by the size of the program stack?

Copy / Paste

• Is there a copy/pasted code with minimal changes? Consider refactoring.

Security

• It should not be possible to execute user-provided code on the server.
• It should not be possible to do SQL injection.
• API should not return secret or personal user data if it’s not needed by the frontend (email, password, credit card number, phone number, address).
• It should not be possible to use a “brute-force attack.” Add some counter to prevent such an attack from happening.

Errors Processing

• Is the error handling technique defined in the project? You should define the format of errors (messages, status codes, ..) and where to send errors (console, file, AWS S3, etc.)
• Is the error-handling code centralized in a single module that can be replaced if necessary?
• Are errors being ignored anywhere? (Empty “catch” block means that error is ignored, and debugging that will be difficult.)
• Do you return too much information to the end-user when an error happens? Have you checked system paths and parameters of database connections?

Logging

• Is there enough logging? It’ll help to debug problems on production.
• Isn’t there too much logging?
• Are you sure that you don’t send sensitive information to logs (passwords, API keys, database credentials, etc.)?
• Is the logging library centralized in a single module that can be replaced if necessary?
• Are messages divided by severity level (info, warning, error)?
• Can you disable some levels of logging (info, debug)?
• Can you easily add new destinations where logs are sent (console, file, AWS S3, etc.)? 

Input Validation

• Do you have protection from incorrect input?
• Did you define a format of validation errors that you return? (HTTP status code and error response format)
• File upload: is there a protection against too large files?
• File upload: do you check that file type is the one you expect?

Race Conditions

• Can race conditions arise?
• Can you tell the complexity of the algorithm? O (n), O (n2), O (n * n * n)?
• How will the program behave if a large amount of data arrives unexpectedly?
• How will the program behave if an unusually high number of requests arrive?
• What happens if a person accidentally performs the same action twice?

Compliance with Requirements

• Does the code do exactly what is indicated in the requirements?
• Did you miss any small but important details?
• Have you done your own assumptions instead of asking the customer or PM?

Backward Compatibility

• Do the changes break backward compatibility?
• Are the APIs and interfaces extensible if extension is expected in future?

Third-Party Libraries

• Is this library really needed?
• Is the library distributed under an open-source license?
• Are there any known vulnerabilities in the library?
• Are maintainers and community active? Is it easy to contact them in case of issues?

Documentation

• Are there complex pieces in the code and do comments describe them?
• Are data structures and units of measurement explained (at least with Flow annotations)?
• Is any unusual behavior or edge-case handling described?

• Is there unfinished or temporary code? If yes, it should be deleted or marked ‘TODO’.

• Commented code should be removed.

Automated Tests

• Does each requirement have its own test case?
• Does each element you designed (module, class, function) have its own test case?
• Has each line of code been covered with unit tests?
• Have all simple boundaries been tested: maximum, minimum, and off-by-one boundaries?
• Do test cases check for the wrong input data—for example, a negative number of employees in a payroll program?
• Is compatibility with old data tested?
• Is the minimum configuration tested?
• Is the maximum configuration tested?
• How fragile are tests? Don’t tests depend on each other? 

General Recommendations to Reviewees during Code Review

1. Review code once again before submitting it. You might still detect some issues and save the reviewer’s time.
2. Be tidy, keep in mind that your code should read well from a 14-inch screen, without any horizontal scrolling. So try to avoid long, unreadable lines of code.
3. Don’t postpone code reviews, make them regular. Otherwise, a large amount of code will accumulate for review, impacting efficiency.
4. What will also help save the reviewer time is your descriptions. Often when you write the code and everything is in your head, you might be under the impression that your code is self-explanatory.  It’s not. If you describe what is going on and why you’ve made those changes, it will be much easier to follow your logic.
5. Be open to suggestions, during code reviews you can get useful insights or learn a clever way to solve a problem you’re facing from a colleague who has solved that problem before.

General Recommendations for Reviewers during Code Review

1. Don’t work too hard. The piece of code you’re reviewing shouldn’t be longer than 400 lines at a time and limit yourself to 60 minutes of continuous focus. If you go over that limit, the review will no longer be efficient.
2. Set clear and measurable metrics and goals for your code review. Use SMART to set comprehensive metrics for evaluating a peer’s code quality. It will help you to provide well-structured and justified feedback to your colleague and team.
3. The feedback during code reviews should be constructive, containing arguments to justify changes and an alternative solution. Good feedback takes the form of a suggestion, not criticism or personal comment.
4. Feedback should be constructive, containing arguments to justify changes and an alternative solution. Good feedback takes the form of a suggestion, not criticism or personal comment.
5. Reviewing code workarounds and temporary solutions requires as much attention. Workarounds may exist in projects for a long time and require marks from other teammates (TODO, FIXME, etc).

We believe that our approach encourages team members to take feedback seriously, regardless of the reviewer’s technical level. Even if a comment is unreasonable or weakly argued, the ensuing discussion sparks knowledge-sharing. The code review process increases code quality, as well as increasing team members’ knowledge base and understanding, which reduces the likelihood of future errors in the code. 

How do you do peer code review in your company? Do you find our process efficient? If we missed something or something is unclear, we’re open to your suggestions and questions.