I thrive on critical feedback. Be it from automated tests as I drive the interfaces and interactions of code I'm writing or perspective, advice, and coaching from coworkers. Code Review is an opportunity for team members provide critical feedback during the evolution of projects. In some contexts, I've worked on teams which struggle to extract the true value of Code Review. The following are opinions I use to help teams realize more productive uses of Code Review. This is not a prescription, but rather a set of meditations to encourage a constructive, elevating atmosphere of honest discussion about the work we do.

I claim no special ownership of these ideas. This is merely a distillation of the concepts I've found useful in evolving my understanding of the practice of Code Review.

Code Review is the discipline of discussing code with peers. It is a popular belief that the focus of Code Reviews should be to uncover defects. While true that certain classes of defects can be uncovered during the Code Review process, Microsoft's "Expectations, Outcomes, and Challenges Of Modern Code Review" research shows that we overestimate our ability to discover errors in Code Review. If Code Review is not an effective tool for finding defects, then what value is extracted from the effort put into a Code Review? Microsoft's research concludes that Code Reviews are a likely conduit for context sharing; making teams more likely to be aware of how a system is changing, find alternate solutions, and share knowledge and experience in improving the design of the design of the software. At its best, the practice of Code Review makes us more engaged teammates.

There are many ways for teams to organize and execute code review. Some organizations have a more formal and scheduled Code Review process; think weekly meetings and a collection of "senior" - or in the best case "context-ed" - staff gathered to review potential change.

Another, less resource intensive way, is to have teams self-organize, asynchronously to perform Code Review. Usually, once the work is functional, the author request an asynchronous, informal review of the changes by one or more teammates. Each reviewer has responsibility to understand the context of the change, offer constructive criticism, and work with the author to progress the work to an acceptable state. Why a less formal process? Efficiency. Usually an author and reviewer pair is adequate to resolve problematic design issues. The focus and quality of discussion tends to degrade as the audience size grows. However, there are times when seeking more points of view are warranted and we should be free to seek the input of our peers if we have reason to believe it would be worth the cost of their attention. Including another point of view can help resolve long-running discussions if the author and reviewer cannot reach an agreement on their own - this is usually the "exception", not the rule.

• Assume that everyone has good intentions. Use the most charitable reading of any received message.
• When faced with a conflicting set of ideas Steel Man/Person the other's arguments: understand the strongest possible version of the argument and see if there's still room between your arguments for valuable discussion.
• Accept that many programming decisions are opinions. Discuss trade-offs, which you prefer and why, and reach a resolution quickly. It is OK to disagree and commit to a path forward.
• Ask questions; don't make demands. ("What do you think about naming this `:user_id`?")
• Ask for clarification. ("I didn't understand. Can you clarify?")
• Avoid selective ownership of code. ("mine", "not mine", "yours")
• Avoid using terms that could be seen as referring to personal traits. ("dumb", "stupid"). Assume everyone is attractive, intelligent, and well-meaning.
• Be explicit. Remember people don't always understand your intentions online.
• Be humble. ("I'm not sure. Let's look it up.")
• Don't use hyperbole. ("always", "never", "endlessly", "nothing")
• Don't use sarcasm.
• Keep it real. If emoji, animated GIFs, or humor aren't you, don't force them. If they are, deploy them with aplomb.
• Talk synchronously (e.g. chat, screen sharing, in person) if there are too many "I didn't understand" or "Alternative solution:" comments. Post a follow-up comment summarizing the discussion.
• Most importantly, don't forget to give praise and thanks.

• Be grateful for the reviewer's suggestions. ("Good call. I'll make that change.")
• Don't take it personally. The review is of the code, not you.
• Explain why the code exists. ("It's like that because of these reasons. Would it be more clear if I rename this class/file/method/variable?")
• Extract some changes and refactorings into future tickets/stories.
• Link to the Code Review from the ticket/story, or use a bot which does this for you.
• Push commits based on earlier rounds of feedback as isolated commits to the branch. Do not alter history until the branch is ready to merge. Reviewers should be able to read individual updates based on their earlier feedback.
• Seek to understand the reviewer's perspective.
• Try to respond to every piece of feedback.
• Wait to merge the branch until Continuous Integration (TDDium, TravisCI, Jenkins, etc.) tells you the test suite is green in the branch.
• Merge once you feel confident in the code and its impact on the project.

Remember that Code Review is complete when parties agree that the change is "Good Enough." Perfect solutions never ship, and all decisions are trade-offs. We are professionals. We use our best judgment to determine if a change is publishable as swiftly as possible.

Everyone brings their own experience and perspective to their reviews. The following is an in-exhaustive list of areas a reviewer can consider during a review.

• Readability: Do you believe that any team member could understand how the change accomplishes the goal in a reasonable amount of time? Is there perhaps a more clear way of expressing all or parts of the solution? Are example, object, and method names intention revealing and expressive?
• Maintainability: Are the appropriate concepts and abstractions present? Are the appropriate uses of method and object composition being applied to increase flexibility?
• Testability: Are the components of the change sufficiently covered by automated tests? Are the tests exercising the pieces which are likely to break? Do the tests document the behavior of the system? Are the tests immediately understandable to a reader with limited understanding of the change?
• Supportability: Is the solution robust in the face of likely failure? When the change-set breaks in production, how does it break? How does do we know when it's broken or under-performing? How do we measure the impact of the change to the business?

Some notes on code style and style guide violations. Call out style guide violations as you see them, but don't spend too much time seeking them out. Rely on the automated style checkers to catch these types of issues; your human brain is more apt to deal with nuanced aspects of review. In the event conflict over style is raised which is not addressed by the style guide create an issue in the guide repository to deal with the issue, follow the style guide as closely as possible or defer judgment to the author of the change, and move on with the Code Review.

A few additional things to keep in mind. Understand why the change is necessary (fixes a bug, improves the user experience, refactors the existing code). Then:

• Communicate which ideas you feel strongly about and those you don't.
• Identify ways to simplify the code while still solving the problem.
• If discussions turn too philosophical or academic, move the discussion out-of-band. Meanwhile, let the author make the final decision on alternative implementations.
• Offer alternative implementations, but assume the author already considered them. ("What do you think about using a custom validation here?")
• Seek to understand the author's perspective.

Pull Requests are temporally relevant, but commits are forever

The clear expression of intent and context sets the stage for a great Code Review. While good commit messages are an excellent tool for authors to set the tone and direction of a Code Review, commit messages serve another valuable purpose.

One often sees commit messages explaining WHAT a change is; "Add email to User". This adds little value as it is a restatement of information already contained in the change's diff. Good commit messages capture information about WHY a change exist - what's changed in the business to necessitate the change, had we make an incorrect assumption that forces us to make this change now, what alternate solutions were considered, how were trade offs measured, how do we measure the success or failure of THIS change, etc. We can use commit messages to convey and preserve such context. The goal is to produce archaeological artifacts that capture as much of WHY a change is being introduced. Over time the history becomes an excellent context loading resource for future Code Reviews.

When no clear comment / description standards are present, I like to use a style heavily influenced by Chris Beams' article "How to Write a Git Commit Message". The following template illustrates this style:

Summary
---------------

This should clearly and concisely convey the change's reason for
being. Use a paragraph or two to explain the business need, technical
issue, etc. that the change intends to fix. This gives the reviewer
a quick way to gain the requisite context needed to understand the
change.

This section could be used to provide any additional context the
author feels might be relevant to the reviewer to better understand
the changes. Examples might include links to articles which inspired
design decisions, notes on alternate solutions, or approaches that
didn't work out and why. This allows the author to bridge context gaps
and anticipate the obvious questions a reviewer might ask.

In addition, this provides a section for the author to call attention
to a particular section of the change they'd like to be thoroughly
criticized.

References
----------

Lead with a hypertext link to the discussion from where this change
originated; could be a JIRA ticket, Pivotal Story, Github Issues
ticket, etc. The goal is to provide a connection to the originating
idea for the change, discussion, and any relevant context
therein. This is important for archaeological reasons as we aim to
provide insight into the state-of-mind of the organization, business
considerations, alternate approaches which were considered, etc when
the problem or feature was first identified.

Include hypertext links to topical articles, influencing white-papers
and academic discussions, contributing computational concepts, etc. to
help the reviewer understand the frame of reference from which the
change is wrought.

• Expectations, outcomes, and challenges of modern code review
• Implementing a strong code-review culture by Derek Prior Rails Conf 2015
• Thoughtbot code review guide
• Ruby Rogues Podcast 216 - Code Review Culture
• How to Write a Git Commit Message