Effective Code Reviews: Bettering Products, Teams, and Engineers
Code reviews are an integral part of the software development process. Along with manual and automated testing, they help minimize risk and improve code quality. They also help the team share knowledge and to grow in their careers. This post lays out why, what, and how to review code, and will give you the knowledge you need to build a positive and effective reviewing culture in your team.
What’s in this post
Why Review Code?
— Team Cohesion
— Free Career Improvement Training
— Save time and money
How To Build an Effective Reviewing Process
What To Look For In a Review
— Coding Mistakes
— Security Concerns
— Performance Issues
— … & many more
Reviewing Responsibly
Why Review Code?
Before we get into the whats and hows, let’s talk about why code reviews are so important. We all know that code reviews are a great way for teams to ensure a high-quality product. They help reduce bugs, increase performance, and ensure consistent standards. But there is a much greater advantage than that. Code reviews can strengthen a team and make you a better engineer.
Team Cohesion
Code reviews help build trust in our team members. Not in our technical abilities or that we’ll get the job done, but that we have good intentions. This is important because trust makes it okay to be vulnerable with each other. Being vulnerable promotes healthy conflict. Conflict is how we solve hard problems as a team. Healthy conflict could also be called a “passionate debate of ideas.” This happens when team members feel they are being heard and respected. It’s not enough to be heard. We have to feel heard.
Code reviews also promote individual accountability. In the book, “The Five Dysfunctions of a Team: A Leadership Fable,” the author writes:
The most effective and efficient means of maintaining high standards of performance on a team is peer pressure….More than any policy or system, there is nothing like the fear of letting down respected teammates that motivates people to improve their performance.
By reviewing our code, the team pushes us to be our best. We raise our personal standards. As we reach for higher standards, we begin to work together on a common goal to make the product better. This puts the focus on the team’s results.
Trust + healthy conflict + individual accountability + working together to better the team = team cohesion.
Formal code reviews make for better products, better teams, and better engineers.
Free Career Improvement Training
When I ask software engineers what they felt would help the most in their careers, many said, “I wish the senior engineers devoted more time to me personally.”
Code reviews allow us to:
- Learn from each other’s quality and ideas (regardless of experience levels).
- Practice reading code, which helps us remember how to make our own code more readable (thus more maintainable).
- Practice writing and otherwise communicating clearly.
Engineers who have these skills will do better in their careers than those who don’t.
You might be thinking, “I have really good test coverage. Do I have to invest in formal code reviews as well? Aren’t good tests enough?”
Unit and integration tests are essential, but they’re not enough!
IBM found that adding formal code reviews leads to a 38% reduction in bugs detected after unit tests.¹
HP found that 80% of the errors detected during code reviews were unlikely to be caught by testing.¹
So, now you could be thinking, “Our customers demand great new features. Isn’t a formal code review process a waste of time?”
There’s a widely held perception that code reviews take too much time and slow the project down. In reality, it can save a lot of time in the long run.
One study showed that each bug identified during a code review saves around 9 hours of time downstream.¹ Each. Bug. Nine. Hours.
In the IBM study I mentioned, researchers found that a formal code review process yielded a 23% increase in productivity.¹ What would you and your team be able to do with an extra development day every week?
Now that it is clear that code reviews have tremendous value, let’s talk about how to be most effective when we implement a code review process in our teams.
How To Build an Effective Reviewing Process
To start with, everyone on the team should participate in the process. Create a plan (round-robin, randomized, or other) so that everyone can have their code regularly reviewed by everyone else on the team. That can’t work if each engineer only reviews the area they are most comfortable with.
Start reviewing quickly (on the order of hours, not days). If you think you can’t start a review in time, let the team know right away so a replacement can be found.
Integrate code reviews into your development process. For example, if you’re using Scrum, instead of creating a “Definition of Done,” create a “Definition of Ready to Review,” and include the things that make reviewing easier:
- Unit tests have been written and passed
- Static analysis tools have run and passed
- Documentation has been created or updated
- Screenshots or videos have been created
Add a PR template to your repos. If you’re using GitHub, you can follow these steps. You can add tables, lists, checklists, headings, links, and images. It’ll make reviewing easier and improve the historical record of the change.
Before starting a review, work to understand the change from all sides. Know the purpose (read the user story’s acceptance criteria), and see the result (video, screenshots, walk-through, or try it yourself). Make a connection between the story, the code changes, and the effects of the change.
Don’t rush through a review. For smaller reviews, spend at least 20–40 minutes. For more sizable or impactful changes, spend at least 60–90 minutes.
Split larger reviews into chunks, if needed. 60-minute chunks have been found to be most effective.
Go deep. Think critically. Find lots of ways to comment. Reviews are not just about specific lines of code that need changes. Also review the architecture/design. Include code that has not changed in your review, as well. How well does this change fit with the existing architecture?
Look for learning opportunities and conversation starters. When reviewing code, you should not be a gate-keeper. You don’t have to hold up a change but take the chance to start a process of sharing or learning (in either direction). Every team member, regardless of experience, has something to teach.
Find ways to show that things were done well. We’re looking closely at the hard work someone has done. It helps the author feel better knowing that someone sees the value in good code they have written.
What To Look For In a Review
Coding Mistakes
Coding mistakes can be anything from syntax errors to typos to errors in logic. While some are caught by a compiler, others make sense within the programming language but do not do what was intended.
Security Concerns
Security concerns are issues that could assist would-be-attackers in their efforts. Look for code that allows such things as injection attacks or expose information that could make an attack easier (see the OWASP code review guide for more information).
Performance Issues
Performance issues are those which can cause a portion of an application to be slower than is acceptable. Some examples are inefficient algorithms, heavy network transfers, and unnecessary computations. For web applications, this also includes the appropriate construction of HTML, so as not to slow down the rendering of the page in the browser.
Best Practices
Best practices are those found, either through experience or research, to be those most likely to lead to the desired result. Examples include side effects in what should be a pure function and using descriptive but concise variable names. One thing to remember: best practice is not always best. Use judgment in determining when not to follow the crowd.
Code Quality
Code quality is about creating code that is maintainable, extensible, and consistent in production. Code should be unit tested, decoupled, cohesive, modular, and clean. Much of this could be discussed with the team, decided and agreed upon in advance, and followed as a set of coding standards. This process will reduce unhealthy conflict during the review.
Readability
Code is for humans first, machines second. Readable code is quicker to understand and to modify. Code is more readable when it's scannable. Consider things like line-groups, indentation, brackets, line breaks, and comments when determining readability. Code is more easily scanned when patterns don’t change from one file to the next. Encourage your team to stick to a previously agreed-upon set of coding standards and practices.
Simplicity
Simple is not the same as easy. In fact, it is often difficult to attain. Simple code is boring (not clever), modular, cohesive, decoupled, and has smaller functions. Watch out for code that tries to do too much on a single line. If you can’t figure out what it does at first glance, it may need simplification.
Testability
Testable code is modular. It favors composition over inheritance and polymorphism over conditionals. It follows the single responsibility principles and uses pure, deterministic functions (those with no side effects, whose result will always be the same given the same input).
Validation
Validation is about realizing a strict lack of trust. Client-side validation is helpful for a good user experience, but to rely on client validation on the server can cause major issues. Code should validate all input. It should never trust anything outside of itself. You may have written the calling function, but you may not write the next one.
Business Analytics
Capturing usage patterns is most helpful when complete. Make sure that all analytics requirements are met before releasing code.
Error Handling
Error handling is important but often forgotten. Make sure user input errors are correctly handled. Ensure promises are not used without catch. Catch global errors, and use error boundaries whenever possible to isolate errors.
New Methods
New language features or practices can often make a big difference in readability, quality, simplicity, and reduce the likelihood of coding mistakes. Offer knowledge of new methods when appropriate.
Story Alignment
Make sure the code you are reviewing aligns with the user story’s acceptance criteria. Are there missing requirements? Was there a misunderstanding of requirements? These are better caught now than after the product has been released.
Accessibility
Accessibility is important, yet often ignored or forgotten. Make sure all new components are attributed correctly to ensure all users can successfully navigate the application.
Traceability (Logging)
When issues arise in production, often the logs are the only source of information. Ensure the application is logging all necessary areas of the code.
Globalization / Localization
Look for hard-coded strings that are not translated and numbers that are not formatted correctly for international users.
Tests Align with the Story
It is not only important that the application fulfills the requirements of the user story, but that future changes will continue to fulfill that promise. Tests give us the confidence that future changes will not break existing functionality. Tests should be written that validate the functional requirements found in the acceptance criteria in the user story.
Architectural Improvements
Ensure that the newest changes follow the same architectural patterns the application is based upon. Look for things like separation of concerns, testability, maintainability, and simplicity. Do all modules have an easy to assign “role”?
Dependency Weight
Dependency weight refers to the cost of including a dependency to the application. Some dependencies are larger than others. Think about how each new dependency increases the size of the downloaded application or adds unnecessary computation. Look for alternative ways of accomplishing the desired goals without the added cost of a large dependency.
Missing Things
Sometimes a problem is in the code that was NOT added (but should have been).
What NOT To Look For In a Review
We should not be spending time doing what automated tools can do for us. Let’s talk about automated static code analysis.
Static analysis tools (linters) scan your code for things like syntax errors, usage of deprecated functions, formatting inconsistencies, security risks, best practices, etc.
Modern compilers are taking on more and more responsibility in code analysis. They can check for syntax errors, misuse of types, undeclared variables, etc. Some also optimize for performance. If your language runs through a compiler, you can still benefit greatly from a separate static analysis tool.
Interpreted languages (like JavaScript or Python) are not passed to a compiler, so linters are required for automated checks like these (incorporate them into your “build” process).
These tools can help avoid issues that are hard to see. They shift quality checks to the earlier stages of development, which can save a lot of time and money.
Take a look at this wikipedia page, which is full of static analysis tools for a lot of today’s popular programming languages. Many can be integrated into your editor. Some can even fix common mistakes automatically. Rely on these tools. Spend your time reviewing other important issues. Don’t be a human linter.
Reviewing Responsibly
A good reviewing culture is the most important thing. Let’s look at the responsibilities of each role in the process.
Submitter
Ego
Don’t let your ego stop you from accepting suggestions. When a peer catches a bug or makes a suggestion, try to think of it as a “good catch,” or a “nice idea,” and not an “attack,” or a “personal failing.” You cannot know more than everyone about everything at every moment. Your worth as an engineer isn’t tied to your work. It’s tied to the improvements you make between works.
Respect the Reviewer
Respect the time and efforts of the reviewers. Give them some direction. Point out areas of concern. Describe the code areas to make it easier to navigate. Annotate your code in GitHub (as a PR comment). Code comments help but are permanent. Provide temporary help to reviewers using comments in the tool.
Reviewer
Understand the decisions made by the author
Maybe the author didn’t write it the way you would have. That’s okay. Understand their reasons, and THEN review it. If you still feel you would have done it differently, understand their way, and yours, and evaluate both. Create a proof of concept before making a suggestion.
Comments
Provide clear, concise, friendly, and actionable comments. Comments should be atomic. Don’t add comments like “same here,” or “ditto.” It’s hard to be sure which other comment they are referring to. Indicate importance. Use prefix words like “minor:” or “blocker:”.
Create a Safe Place
Provide a safe place for the author. Never make negative remarks in a review. Never make it personal. Make suggestions, not demands — wording matters. Have passion and sensitivity.
Avoid Words that Blame and Trivialize
Avoid “You” (Direct your comments to the work product, not the author). Avoid “Just” (Don’t trivialize).
Instead of “You didn’t include…”, you can write “This doesn’t contain…”
Instead of “Why didn’t you…”, “What is the reason this doesn’t…”
Replace “Just import this module…” with “Would this module help?”
Everyone
Trust The Team
Trust your team members to have good intentions. Trust that they have your best interest (and that of the team, product, and company) at heart, and that they will be receptive to your comments. Trust their decisions. Respect their talent and hard work. Ask “why.” Don’t tell what. It’s the author’s decision in the end. If they choose not to take your suggestion, don’t take it personally. It's their decision, but still our code. If you feel strongly about something, politely stand your ground. Accept when a final decision has been made.
Teach and Learn
Look for ways to teach your team members. Look for ways to learn from them, too.
Manager
Allow time for reviews. Set goals, policies, and expectations around quality reviews. Make review training available to your team, and attend that training yourself. Never use review results to evaluate engineers. Hold team members accountable for their participation and for contributing constructively.
Most importantly: Grant final editorial control to the original author.
Track Effectiveness
As with other process changes, it is important to ensure that these costs are worth it. We can do this by tracking things like bugs per line, bugs reported per sprint, etc.
Don’t Hesitate
There’s a Chinese proverb: The best time to plant a tree was 20 years ago. The second best time is now. Don’t leave here saying, “We should do that…” Go on. Start. Now.
Formal code reviews make for better products, better teams, and better engineers.
