What makes for a great code review?
Here is one of our favorite responses to get started:
“Every commit is reviewed by at least one other developer using integrated tooling. Reviews are automatically generated and linked to the system of record (e.g. the story in JIRA). Reviews are completed in a reasonable amount of time (hours/days depending). Review feedback is provided in a constructive manner, and focuses on standards, security, best practices, easy-to-avoid pitfalls, and of course tries to point out mistakes as much as possible.
Team goals include standardized code, mentoring and training of new/junior developers, team coherence, and achieving high customer satisfaction via high code quality. Business goals include high code quality and customer satisfaction, standards adherence, security and compliance, as well as team and code standardization, and training of developers.”
We have organized the responses below by key themes to make reading through a little easier. Enjoy!
A Standard Review Process
“The ideal process for me would include PR reviews per day, per developer. Additionally, I would love it if they were fully connected. Once a week it should be combined with a team review of the last week to see problems. For business, I always want to achieve the best solution, with the lowest possible cost, for maintenance and feature creation. For our team, I would love to improve our source code quality and the joy to work with our code.”
“The ideal review process is the one that ensures having an application where internally (the code), and externally (the functionality), meet the requirements, do not contain errors, and are developed in the most efficient way possible under the standards (which there should be) of the company and the market. The goal is to achieve high quality, low cost, predictable times, and customer satisfaction.”
“From my experience with customers / projects, in order of importance:
- Prepare a super-detailed design of the requested feature / system / whatever. It happens too often that my customers describe what they want but fail to understand what they are actually asking for, i.e. the outcomes of their definitions. They don't understand the future implications, like, if you do X then you won't be able to Y. Additionally, they miss too many "edge cases" thinking that "this almost never happens," but "almost" is not "never" and then programmers just assume what they think makes sense and ...assumptions are the mother of all ...you know :)
- Whatever the programming process is, unit tests are critical. I'm not even talking about running automatically etc. (which is of course better). Having them and being able to run them, every programmer within his/her own workflow is WAAAAY better than not having them at all.
- Customer acceptance test. No matter how good the design is, how good the code is etc. In the end, I had multiple cases when the customer were "surprised" with what they got. Although everything was done exactly by the definitions and all.
- Yes, only at this stage, code quality / code reviews, etc., come into play. Personally I just don't believe there should be a real "process" to code quality. I even refuse to the idea of pull requests! Beginner programmers should maybe work together with a more experienced person (as much as needed) but more experienced programmers should just know when they need help or just have another eye to take a look at their code.”
“Every team member is really trying to understand what that piece of code is doing, taking into account the system as a whole. Tries to find bugs, security issues (which were not covered by unit tests or manual testing), before merging it into the master. Keeps in mind code conventions, security guidelines, SOLID and checks the code adhere to those principles. Each member is doing code review at least every morning before taking new tasks or continue with current tasks. Check if there is anything to review when they finish their current task. Comment on others comments (likes included). That will help to increase code quality overall.”
“Of course it begins with my own unit testing, after that with peer revision, next with QA revision process, finally a functional checking, and the process is cyclic continue until the product is ready for deploying to the client.
Of this way, we can minimize the possible errors in test and production environment, because it means time and money, and the most important thing the client satisfaction.”
“First of all, we have to understand the context and the task goal. So we have to do a process review, regarding the quality standard and well-known practices. We have to control if all the possible tests are implemented. If something may be done well, we have to talk and share our point of view, and decide what to do. When all seems ok, we can set our "green" feedback and allow merge.”
- Quality standards
- Sharing knowledge
- Sharing context
- Team building
“The code review process is on multiple stage levels. 1) on first stage developer review his/her own code by seeing whatever he/she written and by writing unit test cases and integration test cases 2) on second stage, the developer creates merge request at that time a senior person can review and give suggestions so that if someone misses something or need any improvement on code, they will guide to the developer. 3) on the third stage, when your branch is merge into master branch, code quality tools like sonar Qube, run on master branch and give suggestions. so we are working on this three-stage code quality environment. so mostly all things are cover and improve code qualities and achieve the goals.”
“For me the best review process looks like a meeting: you should enter knowing what will be discussed (or reviewed), and left with a note that it was done in the review. In fact, documentation is the key for a good reuse of code, if nobody knows what was done, probably someone else will do it again.
With code review we spread the system understand across the developers, reduce the number of bugs and increase components reuse, bringing more software delivered with less bugs.”
“Ideally the process should be asynchronous, web-based, and tightly-coupled to the software repository, requirements, and test management systems.
The main business goal is to reduce software defects as early as possible, but with the side benefits of sharing knowledge and ensuring consistency and conformance to conventions.”
“All code changes would be reviewed by a developer familiar with the feature being worked on. That is, the reviewer would be able to fully understand the code being changed. Too often, the reviewer is clueless and the review is just a check-off without substance. Having consistent substantive reviews results in fewer bugs and in fewer forgotten or misinterpreted requirements, leading to a better product that satisfies customer requests. That leads to reduced tech support efforts and increased bandwidth in the development team to add even more new features. And ultimately, it's the addition of new features requested by customers that keep our customers happiest.”
“Ideal review that the Pull request already comes with all the points agreed on checklist pages made.
Main objective is to facilitate the maintenance of the code and mainly in the problems encountered in production.”
“Our current process is ideal, PR submitted, auto assigned to 2 or more members of the team for review, approved review required for merge. Achieve the business goal of ensuring high quality tested code and the team goals of collaboration, learning & high quality code.”
“Review process should have functional as well as non-functional requirements review. It should have peer-to-peer, over-the-shoulder review along with a proper tool-based review. The ideal code review would be done by the development environment where at the time of development itself, there will be guardrails/alerts/warnings shown that will be needed to addressed to proceed with the development. At the time of tool-based code review there should be flexibility to override the code review in case of any exceptions, but that should require the next level approval. Before the start of development and whenever any new developer is on-boarded, there should be a proper training provided, specifying the best practices and coding standards, and then the complete step-by-step process of code review and the criteria to look for. There should be very few exceptions in the code-review process, if we are seeing multiple exceptions, then the review process needs to be updated. Proper review of requirements and simultaneous code review during the development will help meet all business and team goals by eliminating the over-head of peer review.”
“Coding standards should say:
- Set your warnings high.
- Prefer fixing warnings to marking them ignored.
- Throw exceptions liberally.
- Catch exceptions sparingly.
- Generally assume your input parameters are valid. (Except when constructing objects.)
- Assume any method you call actually works fine. (Don't preemptively catch for bugs.)
- Never return null if the method failed.
- Don't try to prevent exceptions in production code. Try to prevent bugs instead. Exceptions are the trail of breadcrumbs that lead you to bugs. Don't suppress them, use them to catch bugs.”
“Peers must review, lead/senior must merge after approved. Stories must be clear, exact, concise. Reviewer easily compare what needs to be done from story and what is done. Recurring code flaws can be shared all team members in a meeting. Review must consider logic of the code, implementation style, design flaws and patterns.”
“The code should be reviewed by more than one person, but it should be fast, one work day for a merge request get reviewed is acceptable, more than this people will start to complain about the process.
Developers should look for:
- Design of the code, if the code is in the right place and is correctly interacting with the remaining parts of the system.
- Functionality, if the code does not have any side effect and the functions do what their name says.
- Complexity, if there is a way to simplify the code.
- Naming, to ensure the developer is following the project naming conventions and is choosing good names for the elements.
- Style, to ensure the developer is following the code styleguide adopted for the project.”
“If we follow these steps for all the stories, then we will have great quality of code on our hands.
- Code analyzer/coverage (ex: sonar)
- Technical code review
- Functional review
- Unit and integration tests
- Performance test”
Attention to Code Maintainability
“Review process: 1 person other than developer reviews using specific guidelines (line clean code). Goals: improving quality, learning on the job, improving maintainability of code.”
“Analyze user requirements -> Review Requirement docs -> Analyze existing code -> Prepare and review FTSD -> Code -> Unit Test -> Review -> Unit Test -> Integration Test -> Release.
This will make sure all requirements are covered and code quality is maintained without bugs.”
“While reviewing, consider these points: 1. Maintain coding standard 2. Methods or file are created as reusable component. 3. Minimize db hit and get limited data required. 4. Better to use library functions until you maintain Big O notation. 5. Consider performance as most important part of your software. 6. It's always helpful to maintain Test case for each functionality and keep it updated.
These points will help you to maintain concise, reusable components of codes and, last but not least, less number of bugs introduced in each development cycle.”
- “High-level style rules at write-time
- Automated build, unit test, integration test
- 1-2 peers do line-by-line and architectural review, including basic test
- Merge to main branch
The sooner you catch issues, the less time is wasted. The more consistent the codebase, the easier it is to extend and maintain.”
Fast Feedback Loops
“It would involve a fast feedback loop, so would start with a review of pseudo code (design/architecture), continue with pair programming, and the final review would be more about sharing team knowledge.”
“Every commit is reviewed by at least one other developer using integrated tooling. Reviews are automatically generated and linked to the system of record (e.g. the story in JIRA). Reviews are completed in a reasonable amount of time (hours/days depending). Review feedback is provided in a constructive manner, and focuses on standards, security, best practices, easy to avoid pitfalls, and of course tries to point out mistakes as much as possible.
Team goals include standardized code, mentoring and training of new/junior developers, team coherence, and achieving high customer satisfaction via high code quality.
Business goals include high code quality and customer satisfaction, standards adherence, security and compliance, as well as team and code standardization and training of developers.”
Sufficient Time for Reviews
“A healthy time to talk about user stories, preferably spread out between a couple meetings so people can really think about the work. Many things are discovered during development -- which is normal -- but discovering these sooner would really help our productivity.”
“A good review needs a mediator who knows the objectives of the project very well to conduct the review. Artifact participants should develop in a very explanatory manner.
In this way it is possible to optimize the review meeting time.”
- “Know What to Look for in a Code Review.
- Build and Test — Before Review.
- Don't Review Code for Longer Than 60 Minutes.
- Check No More Than 400 Lines at a Time.
- Give Feedback That Helps (Not Hurts).
- Communicate Goals and Expectations.
- Include Everyone in the Code Review Process.
- Foster a Positive Culture.”
“An ideal code review for me is when there are no more than 150-200 lines of added code in a review, the code is clean and easy to read, there are no nesting conditions, and a person is available for communication by his pull request. Most importantly, all the goals set in the ticket were completed and tested. If possible, a screencast by code should be recorded. The goals of the company are tested, working, readable, and supported code.”
“I think it is a requirement to have a formal process with well-defined rules, in a timely manner. In some way, members of different areas should know whatever happens in the new releases, and what a better time than sharing the details at a review phase. I'm including operators in the infrastructure team, checking for permissions and monitoring process, as well as stakeholders to know what's new.”
Automation Analysis and Code Review Tools
“Automatic static and dynamic evaluation. Easy configuration, applicable on continuous integration process.
Goals: standardize code, elevate average code, quality code.”
“Should be tool driven as much as possible coupled with a lot of automation (e.g. Jira integration) and have a strong foundation in review guidelines. Such a process would improve code quality, number of features the squad is able to deliver per release, and share the knowledge within the squad (legacy software projects need that).”
“A good review process should be easy to kick off, integrate with review tool, have sign-off gatekeeper, convenient to provide review comments, and clearly display the review conversation, etc.
Review process can improve the code quality, reduce product bugs, share knowledge among team, and track the developing history data.”
“A team review, used to improve the quality of the automated review workflow (e.g. automatic analysis, testing). The code gets reviewed, and when issues are found the team discusses how to automate detection / fix similar issues in the future. That way over time the team's knowledge gets coded and their time is used more efficiently.”
“The review process should use tools as much as possible to avoid human error. 'Pull Requests' from Git / ADS and SonarQube can be used to automate most of the standards in the review process.
It will allow reviewer to concentrate on the other aspect of the code, e.g., security, performance, any functional bug, etc., which will be a value-add to the business for that release.”
“Lean, objective, as much automated as possible (e.g. linting, spell-check, merge and test pipelines), checklist, documented guidelines, minimum approval criteria in-place for code merge.
- A certain level of code standards are always maintained
- Reduces probability of merging breaking changes (P0) to the mainline
- New members can learn to write production-ready code quicker than in case of continuous manual knowledge transfer etc.”
“Ideal process - code should be evaluated by other teammates. Once it has coding standard, valid code and test cases, it can be accepted.
- To have shared knowledge and strong foundation”
“Code reviews are where engineers keep improving how they do code reviews. These code reviews look at the code change in the context of the codebase, of who is requesting it, and in what situation. These reviews adjust their approach based on the context and situation. The goal not only being a high-quality review, but also to help the developers and teams requesting the review to be more productive.”
“I would like a meeting review of requirement in the first phases of the project, and also meeting reviews of core functional code through all the development process. I think it's ideal to share knowledge of fundamentals of the project with the whole team.”
“ 'In-person', co-located review, side-by-side, using a tool to present the changes for easy review. This improves knowledge transfer of the system operation and design as well as coaching developers in coding standards and common design patterns used.”
“A big part of why we do code reviews is to help educate more junior developers on best practices and code standards (our own), as well as more quickly identify areas for optimization or potential issues. That is the goal with every review, particularly while fully remote since we don't have as many opportunities to work 1:1.”
“The review process should be part of the development culture so that we can fail fast and rectify easily and share knowledge across team members. This will avoid silos and create higher confidence.”
“Focus on helping more junior devs learn from their mistakes and do better next time. It should be less about squashing individual bugs and more about making the team better.”
“Plan, Commit, Review, Commit, Review, Merge. All team members share coding standards and knowledge. Faster feature integration, happier costumers..”
“Automated tooling to pick up as much as possible in terms of code quality issues, bugs, and particularly any security concerns (RIPS is a great tool for security scanning). This is important to maintain confidence in the code-base.
Buddy system for senior devs to mentor juniors. This allows juniors to learn more quickly from the experience of their colleagues, and gives a friendly atmosphere for mistakes to be picked up, rather than a confrontational one.
A merge request system that forces all code changes to be reviewed by another developer before being merged in. This is more confrontational, as bad changes have to be rejected and sent back to the original dev for re-work, but most problems should be picked up from the above before it gets to the point of becoming a merge request; this serves as a final gatehouse to prevent bad commits getting into the master branch. ”
“Ideal review process will be 1-1 code review with one mentor and 1 review from a senior team if there is a major change. Reviewing together with specs and user stories in place is better.
Team bonding. Bug-free code. Compliance.”
“Junior does the work, senior #1 reviews and adds comments or guidance, senior #2 reviews and adds comments or guidance, senior #2 merges.
Junior gets educated on what the business considers 'best practice' as well as technical coding help. Seniors get to understand each others' views and share ideas.
Business benefits by having consistently applied coding standards and best practices.”
“First, schedule a meeting for a Code Review. Then in meeting, let members introduce their respective modules for the upcoming release. When a person introduces, the whole team should review their code and discuss in what way it could be better or cleaned. Do this until everyone's code has been reviewed for the upcoming release.
In this process, the business and team achieves a knowledge about the formality of code review, this also strengthens each others relationship (by discussing stuff), maintains cleaner and higher quality of code, gives awareness for each other's coding styles and approaches, and most of all, educates everyone.”
“Everyone should participate in the review process to understand the code changes so that they can suggest something and one can fix the bug or code vulnerabilities before check-in.
Also, new developers can learn a lot from this type of activities.”
A Learning Culture
“Developers provided with enough time to write tests to achieve suitable test coverage. Code review managed throughout the development process via review of pull requests of individual contributions, and team reviews of code and projects for learning and onboarding.”
“Ideal process would be to get code reviewed from most experienced developers from the team, as it will be a learning experience for juniors. Code review makes sure to adhere to coding standards used by industry, so that hand over of code from one developer to another will be hassle free. It will be easy to add or remove features because of clean code. It will improve the maintainability of code.”
“My ideal review process would reward effort and focus on learning. The goal would be to improve people’s understanding of what good code looks like.”