Code Review

When you are developing on Task Branch, quickly moving work to the Main Line, you want to ensure quality and consistency since changes affect the entire team. Tests and other automation help and feedback from other developers on approach, style, design, and testing approach can provide useful insights. This pattern describes a way to get prompt and useful feedback from team members before you integrate changes into the Main Line.

Balancing Feedback, Learning, Speed, and Quality

Before work related to a development task in a development workspace is ready to merge into the Main Line, the developer wants to have confidence that the code is:

• Complete, robust, and (reasonably) free from errors: does what it’s supposed to do and handles the range of inputs appropriately
• Maintainable: won’t add undue burdens or unreasonable tech debt to the code base,
• Safe to merge: won’t break other parts of the code

Some of these criteria can be addressed by automated testing and static analysis tools, but:

• Static analysis tools can provide insight into some maintainability issues, but they can sometimes be narrow in scope and not cover the entire product development context.
• The results of tests are only as good as the quality of the tests. In particular, when new code with tests is added, you want to be sure that the tests are appropriate for the task’s goals.

You can ask another developer to review the code before it’s merged as a person can provide perspectives that tools can’t, but a review by another developer adds a handoff step to the process, and handoffs can introduce delays that can significantly slow down integration into the_ Main Line._

Another option is to use a “fail fast” approach, where a developer merges code to the main line when they think it is ready, and the team places mechanisms in place to recover from errors. Having reliable rollback mechanisms is an attribute of a robust development process, and this will help improve the speed of integration but:

• Issues detected after code is merged can slow down the whole team
• While “hard failures” would be easy to detect, more subtle design issues might linger until well after the time they were introduced, making them harder to understand (and fix)

Feedback can sometimes be viewed as judgmental and critical rather than a collaborative attempt to improve code quality. You want to ensure that the feedback can focus on significant issues rather than minutia or matters of opinion.

Quick feedback is better, but feedback that is too cursory or not useful enough has little value. Spending more time giving feedback can lead to a point of diminishing returns, reducing the impact of the feedback that you can get from deployed code.

Any sort of review step needs to balance:

• Quality: the review should improve the quality of the code
• Consistency: the quality of the review shouldn’t vary much depending on who’s reviewing.
• Feedback: the feedback should be actionable
• Speed: You want to minimize how much this step delays a merge.


You want to get the benefits of collaboration with your team to improve overall code quality while leveraging tools and automation and not injecting long delays into the integration process.

Relevant, Timely, and Actionable Feedback

** Before merging code from a Task Branch into the Main Line get feedback from another developer. Identify practices that ensure that the feedback is timely and that the reviewer has the appropriate context. Leverage automation where possible to focus on human time. Acknowledge the cost-benefit for some trivial changes where cursory reviews might be sufficient.**

 Good Code Reviews are a form of collaboration that can:

• Reduce error
• Helps the code improve.
• Lead to a shared understanding of the code base.

The kind of feedback that achieves these goals is:

• Relevant: Not just general comments about coding style but comments that take design conversations and the goal of the work into account.
• Timely: Feedback is shared shortly after the code is ready for review, and questions are answered promptly.
• Actionable: Feedback gives enough information to allow the developer to act on it.

Relevant feedback addresses the code’s context, including requirements, design discussions, etc. General programming feedback about style and related issues is useful, but Automated Checks are often better for that feedback. The more context the reviewer has about the problem, the solution, and the design, the easier it is to be relevant.

Timely feedback is sufficiently prompt so that the developer does not lose context because of time spent waiting. Timeliness does not mean “immediate,” though sooner is often better. The definition of “timeliness” should be part of a team’s working agreements.

Actionable feedback is as specific as possible and suggests specific actions to take or questions to discuss. Vague comments about concerns or errors slow down the process.

Ideally, reviewers should be participating team members. Avoid:

• Limiting reviews to senior people (or requiring certain reviewers). You want the reviewers to know the code and the problem, and initially, that might be the more experienced people, but the choice is about skill and knowledge, not status.
• Soliciting feedback from individuals who are not on the team. The team can prioritize features and reviews. Those outside the team might not be willing or able to, so tying integration to the schedule of someone outside the team can be problematic.

A designated reviewer model and external feedback can slow down the process due to scheduling bottlenecks and less contextually informed comments.

It’s easy to get caught up in discussions about tools and the code review process itself when trying to improve it, but most of the challenges with code reviews come down to:

• Lack of clarity about why you are doing them.
• Lack of working agreements/culture that supports those rules.

A good code review is about sharing knowledge and Identifying gaps that tools can’t find. It won’t be perfect, and taking longer in the name of perfection often yields worse outcomes than doing a quicker (or no) check and fixing any problems, as long as you later strive to identify what the issues were.

Code Review feedback can be delivered either interactively or asynchronously. Interactive feedback, where code is discussed in real-time, can take any number of forms:

• Pair Programming
• Desk Review with a Feature Partner.
• An interactive review after sharing a Pull Request

Synchronous feedback doesn’t preclude some preliminary time to review and comment; much like preparing for a meeting, reviewing code “offline” can make for a better review, as long as the interval between the request for feedback and the start of a conversation is short.

Asynchronous Feedback is often associated with a_ Pull Request_ Process, though it can occur in other ways too. Asynchronous feedback can make sense for:

• Larger changes
• Trivial Changes, where feedback with comments is likely to be sufficient
• Distributed teams, where there is a working agreement setting out expectations and commitments about timeliness.

Even when feedback is asynchronous, it’s essential that it be prompt and that conversations (as opposed to self-contained comments) move quickly to an interactive forum.

Remember:

• The goal is to improve the work. Yes, you want to identify problems, but you also want to suggest solutions.
• The participants are all creators; if you give feedback now, you may get feedback later. As a reviewer, consider how useful you might find a comment or request before making it.
• To the extent that the code fulfills the requirements and meets team norms and general design and implementation guidelines, trust the code’s author to decide how to apply comments.
• A code review is not the time to impose personal preferences on the code, though it could be a time to identify gaps in guidelines. If you were not part of the design process for the code, acknowledge that you might be uncertain about some element of requirements or design,

If your process requires approval before merging and feedback is asynchronous, err on the side of permissive approval (“approved pending these changes,” for example). Permissive approval avoids delays and demonstrates trust in team members.

Example

After a development task is completed, a developer posts a message to the team Slack channel asking for feedback and indicating the location of the change. (The mechanism could be a Pull Request, a Task Branch, or simply a request to meet, though it’s often useful to give people a short amount of time to read the code before gathering.)

Team members offer feedback, and the developer makes appropriate changes and then merges the code.

Other Forums

Narrowing the scope of code review is an essential part of meeting the time to integration goals. But not all review and feedback has this purpose. Some goals are better achieved in other forums:

• Learning and knowledge sharing can happen in other feedback sessions or “code workshops.”
• Giving a new developer closer feedback as part of training could be done in an “out of band” review session by a manager or a mentor. This pattern is about Code Review as part of the deployment pipeline. You can ask for reviews at any time.
• Informal requests for peer review when code is preliminary or exploratory.

Review Effort (and Skipping Review)

Not all code needs the same level of review; consider the risk of a change. Some changes you can identify as low risk (either via a tool or heuristic) might not need a peer review — passing automated tests and checks can be sufficient.

Metrics

If you feel like your code review process is bogging down, there are some metrics to track to help you identify bottlenecks:

• Time to first comment:
  • If the wait for a review frequently exceeds the team’s working agreement, work to identify the issue. It could be over-commitment, or perhaps the expectation is unreasonable
• Time between interactions (if you do asynchronous reviews):
  • Asynchronous reviews can be useful for simple direct comments but don’t work well for conversations. If the team feels that it takes too long to have a feedback conversation, consider defaulting to synchronous reviews.
• Time from Code Completion to Merge: This is the key metric for an agile codeline. If Code Review becomes the bottleneck in integration, understand why,

Using a Pull Request mechanism makes it easier to collect this data, though it is still relevant for any feedback process.

Cautions

If the team loses track of the purpose of Code Review—collaboration—it can easily become a bureaucratic, low-value process.

In particular, avoid the following traps:

• Treating code reviews as “evaluations.” The goal is to improve the code. If a pattern emerges that leads to the thought that someone needs coaching, address that in another context, ideally with an “elevate the person’s skills” frame.
• Having reviews require explicit approval gates. If you have a team dynamic where someone will blithely ignore show-stopper issues, this points to larger team dynamics issues. If you must have gates, make them part of your automated tests.

While speed is important, do not skip steps in favor of speed. If the feedback cycle is taking too long, work to improve the process.

When gathering metrics, use them for understanding rather than evaluation. If the metrics look bad, the reasons might be out of individuals’ control yet easy to fix.

Next Steps

• Facilitate relevant (and timely) feedback by ensuring a Team Focus so that Team members can feel comfortable prioritizing reviews and finishing work in progress over any individual goals. Avoid emphasizing individual tasks over larger goals.
• Use Automated Checks to catch mechanical issues with the code so that team members can focus on design and purpose.
• Make sure there is a reviewer with good context, such as a Feature Partner who understands the product scope and technical design.
• Enable automation and support remote team members using a Pull Request.
• Smaller tasks are easier to give good, prompt feedback on. Enable timely feedback with Small Development Tasks