Embedded Files
Why we need code review?
Why we need code review?
Finding errors/bugs before staging/pre-prod/production
Learning by seeing other’s code.
Writing code is an art and different developers/coders/programmers can write the same piece of business logic in its different way. It is always fun and an optimized way to learn different ways of coding, something new about a language, a framework, or general software design principles.
Maintain consistency amongst projects following code styles, readability, simplicity etc.
How will you define a good piece of code, what is the checklist?
How will you define a good piece of code, what is the checklist?
Style Guide
Every organisation, team and project has its own preferred and opinionated style guides. This is a must before starting to code any piece of code.
This helps in avoiding the conflicts amongst the team at a very early stage.
This helps to understand the code base when code is written in a consistent style.
Style guides might include items like below
Never silently consume errors or exceptions.
Always panic if the core go-routine fails.
Define class names in camel case and json variables in snake case.
Never push commented code as part of pull requests.
Every language/framework can have its own style guides. A set of guides for mostly used languages can be found at google’ styleguide.
Code change should be small.
Long pull requests are hard to review, takes a lot of time and there is always a possibility of losing the context while reviewing a long pull request.
Short pull requests can be easily reviewed in a short span of time.
It's totally okay to humbly reject the review request if the pull request is too big to review and developers must be requested to make the pull request smaller by splitting it into multiple smaller pull requests.
Comments/Documentation
Make sure pull requests have documentation to test, run, debug, deploy or release the code. A good documented code will always have a good README file.
Make sure that if any change in the pull request is adding or deprecating the feature, corresponding README is updated as a part of pull requests.
Why do we need comments/documentation on methods, classes , interfaces?
This helps the reader of the code to understand the responsibility of the methods, classes and interfaces.
Example of good documented method
/** Returns an Image object that can then be painted on the screen.
* The url argument must specify an absolute <a href="#{@link}">{@link URL}</a>. The name argument is a specifier that is relative to the url argument.
* This method always returns immediately, whether or not the image exists. When this applet attempts to draw the image on
* the screen, the data will be loaded. The graphics primitives that draw the image will incrementally paint on the screen.
* @param url an absolute URL giving the base location of the image
* @param name the location of the image, relative to the url argument
* @return the image at the specified URL
* @see Image
*/
public Image getImage(URL url, String name) {
try {
return getImage(new URL(url, name));
} catch (MalformedURLException e) {
return null;
}
}
Why do we need comments/documentation on constants, enums or properties of entities?
This helps the reader of the code to understand the use case of contents, enums or properties in case the name of the constant is not self explanatory.
Example of code with docs on variables
// Authentication rules for the service.
// By default, if a method has any authentication requirements, every request
// must include a valid credential matching one of the requirements.
// It's an error to include more than one kind of credential in a single
// request.
// If a method doesn't have any auth requirements, request credentials will be
// ignored.
message AuthenticationRule {
// Selects the methods to which this rule applies.
// Refer to [selector][google.api.DocumentationRule.selector] for syntax details.
string selector = 1;
// The requirements for OAuth credentials.
OAuthRequirements oauth = 2;
// If true, the service accepts API keys without any other credential.
// This flag only applies to HTTP and gRPC requests.
bool allow_without_credential = 5;
// Requirements for additional authentication providers.
repeated AuthRequirement requirements = 7;
}
Reliability
Well designed test suite covering most of the edge cases.
Maintainable and Consistency
Code style is recommended to adhere with the agreed checklist. In case the change is not consistent, then an intelligent action needs to be taken considering other parameters like impact of change on the health of existing code, maintainability, readability and complexity of the change.
Example
As per checklist variable name must be in camel case by PR has variable name in snake case at only 1 line of a class. In such a case, the reviewer can suggest by commenting on pull requests to change the code to maintain the consistency but it should not become the blocker for approval of the pull request if there is no hard check as per guidelines. Comments by reviewers must be clear that it's just a humble suggestion to maintain a good piece.
Design
Low level documentation.
It is always helpful not mandatory to share the low level documentation along with the PR so that the reviewer or reader of the pull request can get the context of the change that is done in the given pull request.
Reuse of code.
Less code is always more maintainable, make sure PR is not redefining things from scratch and reusing the existing piece of code wherever possible.
Following Design Principles
What are the responsibilities of a reviewer?
What are the responsibilities of a reviewer?
Making sure that code quality of the code base under review is good is not decreasing the quality of the existing code.
How to check that ?
Use tools which can quantify the code quality like sonarqube. Tools like these can quantify the code in terms of metrics like Complexity, Duplications, Issues, Maintainability, Reliability, Security, Size
Maintainable
Prefer readable, maintainable, and understandable code over perfect and most optimised code.
Why to accept less optimised or less perfect code?
A key point here is that there is no such thing as “perfect” code—there is only better code. Reviewers should not require the author to polish every tiny piece of a CL before granting approval. Rather, the reviewer should balance out the need to make forward progress compared to the importance of the changes they are suggesting. Instead of seeking perfection, what a reviewer should seek is continuous improvement. A CL that, as a whole, improves the maintainability, readability, and understandability of the system shouldn’t be delayed for days or weeks because it isn’t “perfect.” [1]
Design Review
As part of the product/feature development lifecycle, the team spends significant time designing the feature before actually starting to code. Design involves UI design, low level and high level designs.
While doing the code review, the reviewer needs to make sure that the design in the code adheres to the design approved during the design review process.
Most of the design flaws are noticed and fixed in design review processes but there are some times when design review processes are skipped due to urgency, emergencies or criticality of release timing of the feature. In such cases it becomes one of the core responsibilities of a reviewer to review the design of the code.
Commonly used design principles like solid principles, single responsibility principle, open close responsibility principle, interface segregation principle are used in well designed codebase.
What to look for in a code review - book by Trisha Gee explains in detail what to look for while reviewing the design of the code.
Complexity
Simple rule for this is if code is easily understandable then it's simple else it's complex.
//Simple piece of code
public Booking createHotelBooking(@NotNull @Valid final CreateBookingRequest request) {
//extracting the customer details from client request
CustomerDetails customerDetails = extractCustomerDetails(request);
//extracting the hotel details from client request
HotelDetails hotelDetails = extractHotelDetails(request);
//extracting the booking details from client request
BookingAgent bookingAgent = extractBookingAgentDetails(request);
//creating booking entry
return createBooking(customerDetails, hotelDetails, bookingAgent);
}
//Complex piece of code
public Booking createHotelBooking(@NotNull @Valid final CreateBookingRequest request) {
//creating booking entry
return createBooking(extractCustomerDetails(request), extractHotelDetails(request), extractBookingAgentDetails(request));
}
Make sure there is balance between generosity of the code and over-engineering.
Understand the impact of change on performance or security
Though to get a clear number around the performance impact of the code, performance testing of the code is done but reviewers must try to catch any possibility of bad impact on performance and suggest to improve the code.
Few common performance impact that can be checked are
Bad queries/Long running queries.
Usually the queries involved in the pull request are well defined in the low level documentation by keeping DBA in sync with the feature changes. In case it's not covered in the doc or pull request, reviewers can ask the developer to share the execution plan of the query mentioned in the pull request.
Latency Impact.
Some time there involves a chain of calls in an API which might lead to latency increase of the APIs.
Memory Spikes.
There can be many use cases which can lead to memory spikes like multi threaded code without correct thread pooling, holding a lot of objects in memory in a transaction.
Throughput Impact.
Make sure that the resource which might be impacted by the throughput change is able to handle the change.
Deadlocks or race conditions.
Never ending goroutines without deadlines.
long running queries, bad queries. Using explain. ask the dev/DBA for execution plan
Latency, memory, or throughput
Correctness of the feature?
Reviewer needs to make sure that change is in accordance with feature requests.
Make sure that the test suite is well designed.
Feature is tested?
Make sure tests are not complex and are well maintainable.
Make sure enough tests are written considering different edge cases(happy and failure scenarios) of the feature.
Make sure each test is separated and not combined in a single test function. This will give better assertions and documentation for the code.
How to quantify how reliable this code is ?
Unit Tests
Why do we need unit tests?
Unit test gives the confidence at a method/class level. This makes sure that the class/method is working as expected.
Functional/Integration Tests
Why do we need functional tests?
While unit tests cover the correctness of smaller units like classes and functions, we need to make sure that when these classes of methods call each other then the feature works as expected.
Why do we need both unit and functional tests?
When a functional test fails it's hard to debug the RCA of the failure since the functional test involves many classes and function calls in the flow.
In this situation a well written unit test comes to that rescue since these are written at the function/class level so in case any functional test fails corresponding unit tests will also fail helping us figure out the RCA of the failure.
Analyse the impact of change on system/business
Sometimes a pull request or change involves just 5-10 lines of code. Reviewers must understand the impact of these 5-10 lines of code on the system/business by thinking beyond the pull request. Some times even a log line can bring the system down due to memory issue.
Reviewers must review the abstraction introduced and how it fits into existing systems/architecture.
Applause a good piece of code.
If the reviewer finds a good piece of code in pull request, the reviewer must appreciate the developer and should encourage the team to follow the similar good practices by giving the example.
Give opinions with data and facts
A reviewer is not supposed to emphasise on his/her opinion just based on his experience or designation. It is really helpful to give opinions with data, fact or example as a part of pull request comments.
Opinions without data and fact leads to people being defensive and sometimes leads to unhealthy discussions.
Behavioural Language
Respect the developer and be empathetic.
Be patient.
Criticise the code not the developer.
Code Review Guidelines for Human covers a lot of nice ways on how to put harsh sentences in a very polite and humble way. This also covers how to suggest changes without hurting the ego of the developer.
Take 3rd opinion if required
Don’t approve if you don’t feel confident about the code
Sometimes reviewers are supposed to do the review as fast as possible depending upon the situation. In such cases the reviewer must not feel under pressure and should take his/her time to review the code nicely. Failure to do so will lead to buggy code in production and might jeopardise the reputation of the reviewer and developer.
If the reviewer ships the good code by taking enough time no one might approach him, but if the reviewer ships the buggy code without properly reviewing it. Definitely he/she will be pointed when the bugs are raised in production.
Make sure all the comments which need to be addressed get addressed at the end before approving the code.
Figure out common bad/good patterns while reviewing and share with the team.
Reviewer must keep a note of good/bad practices found in frequent pull requests. Reviewer can present these good/bad practice in weekly sync ups to the team so that it can benefit the whole team.
Test run the pull request
Though it is not mandatory but some time reviewers cannot tell if the given piece of code will work. Or sometimes the pull request involves the changes in critical business features. So review must try to do the test run of the code if possible. If a test run of the code is not possible then the reviewer must sync up with the developer to give the demo of the change.
What are the responsibilities of a developer?
What are the responsibilities of a developer?
Have correct ego
Should have not have too much ego so that they can't digest the suggestions for better solutions.
Attach the design and feature docs along the pull request
Sometimes reviewers might need to take a look at design and feature docs to do reviews even better, sharing the docs beforehand will reduce the wastage of time between developer and reviewer asking for the docs.
Follow up with the reviewer
Ask for the expected time to finish the review.
This is really important in cases when developers have to work on a given pull request or some other have to extend the given pull request under review.
Remind the reviewer.
Most of the time reviewers of the codes are engineers with many responsibilities and engineers by default are poor managers. So sometimes reviewers might forget to review the pull requests. In such cases rather than waiting for the reviewer to come back to the developer, the developer must humbly remind the reviewer to review the code. In such cases a reviewer can also suggest some other reviewer to review the code in case the primary reviewer is busy.
Keep the change small
Developers must try to keep the pull requests smaller so that reviews can be faster and better.
Large pull requests tend to get less and poor reviews due to many reasons.
In case the change is really very big and is supposed to change many files then try to break the large pull request into smaller pull requests.
Stick to the design
Before development, the team spends a significant amount of time in designing the product/feature. Make sure the code adheres to the design approved during the design review process.
If the design is changed during the development, this will lead to wastage of the team's previous effort and might require unnecessary discussions amongst the team to review the design again.
How to plan code review[WIP]?
How to plan code review[WIP]?
Make sure you have someone with expertise required to review your code.
Some time you work on new technology/framework as part of development of features. Make sure you have a reviewer in the team who has expertise in that technology/framework.
Discuss before development
Generally this is covered in low level design but in case you reviewer is not aware of the low level design or don't have the business context of the change, then before starting the development, the developer must plan ahead by discussing with the reviewer about the change he/she is going to do. This will help the reviewer to plan his time and gather the context that might be required to review the pull request accordingly.
How to do code review[WIP]?
How to do code review[WIP]?
Automate the review process.
Automate syntax errors, linting or language specific bad practices issues etc.
There are plenty of tools out there which can be hooked up with CICD pipelines to do the initial review of the change in code or pull request and publish the analysis report to the reviewer and developer.
Tools like sonarqube/Jetbrains can also suggest the possible bugs, duplication, issues, maintainability and reliability issues by analysing the pull requests.
These tools can run the tests on the code and provide the impact of the given pull request on the health of the existing code base.
In case pull request doesn't pass the required quality gates as per organisation or project, suggest the developer to make changes accordingly before doing the review.
Example of analysis by Jetbrains IDE, which pointed out that the below function is not throwing any “EntityNotFoundException” and documentation of the code function is not consistent.
Example of analysis of possible “NullPointerException” by sonarqube.
Quality/Coverage report generated by sonarqube
Test Summary of the codebase generated by the tools
Pair Code Review
Good with new developers.
Team Code Review
Generally driven by developer and presented to the team.
This can be a very good team building activity.
Independent Code Review
Once basic quality checks on the pull requests are done, scroll through the changes in pull requests at the high-level, getting the idea of which classes, packages or API are changed.
Find the entry point of the change like API or method call and walkthrough the call flow till the end. This will help the reviewer in maintaining the context of the flow
What to do in case of conflicts ?
What to do in case of conflicts ?
Handling push back in code reviews covers most of the root causes of conflicts between developer and reviewers. Both developer and reviewer must read through points in detail so as to have a seamless review process.
In any conflict on a code review, the first step should always be for the developer and reviewer to try to come to consensus, based on the checklist above.[1]
When coming to consensus becomes especially difficult, it can help to have a face-to-face meeting or a video conference between the reviewer and the author, instead of just trying to resolve the conflict through code review comments. (If you do this, though, make sure to record the results of the discussion as a comment on the pull request, for future readers.)[1]
If that doesn’t resolve the situation, the most common way to resolve it would be to escalate. Often the escalation path is to a broader team discussion, having a Technical Lead weigh in, asking for a decision from a maintainer of the code, or asking another senior technical leader to help out. Don’t let a pull request sit around because the author and the reviewer can’t come to an agreement.[1]
References
References
Page updated
Google Sites
Report abuse