code-review.md

Code Review

Polyglot Unconference 2018

John, from Mobify kickoff presentation

Initial cofounder of mobify, did a lot of early development, solo. Added more developers to the team, got more experience looking at others' code. Learned a lot of things by looking at others' code. As code review policy brought in, junior developers picked stuff up faster.

In Code Review you have an author (making the changes) and a review (checking out those changes) Need a safe space to give and receive feedback Should feel like a collaborative thing

Process

1. Author creates a changeset.

2. Ask reviewer for feedback.

3. Changeset is either accepted or rejected.

Reviwer checklist

• Does this change deliver business value? Does it make sense? Is this something that we as a team actually want to do. Is it solving the right problem? Understand why the change was made.

• Does it work as expected?

• Is this change maintainable? Does it fit into the codebase?

Giving great feedback

Give feedback about things that could be done better, but also mention things that you learned by reviewing it.

Give feedback about the code, not the person writing the code.

Assume positive intent. Be friendly and patient, welcoming and respectful, mindful of the words you choose. Easy for context to get lost in comment text.

When you disagree, try to understand why. Explain thought process.

Anti-patterns

Code in review delivers no value. Code review is an important activity that should be prioritized. Most antipatterns are things that slow down code getting to production. The best teams aim to complete code reviews the same day they are opened.

• The change is too big: 10 line change vs 10000 line change will probably get the same amount of review & feedback. Smaller changes will get better feedback.

• Rewrites: "I think this problem could be solved in a different way" Probably should have talked more with team about solution before implementing.

• Bouncing reivews: List of things that need changing before it will be accepted, but moving the goal posts. As a reviewer: Differentiate between blocking and non-blocking changes. You should call out whether your feedback is blocking or not.

• Blocking disagreements: Disagree about what the best course of action is to move forward. Bring in a 3rd person, or just talk through the change in person.

Experiences with code review

• Eric from Sensu (100% remote team)

  We need 2 developers to sign off before it gets merged. Needs to be done within 24 hours (ideally)

  Better question than "does this deliver business value" is "Does the business value offset the debt it creates"?

• Chris from Wealthbar

  Code reviewing is something we really increased over time, from ad-hoc to policy. When not getting good results from code review, often comes from wanting to minimize time spent reviewing

  Code reivew is an expression of communication, but not the only one. Don't lay too much emphasis on the code review itself, don't use a code review to be determining business value. Code reivew is the "last check", and business requirements review should be done much earlier in the process. Need to devote more time to design/requirements review and upfront architecture ahead of time.

• Jonas from Hootsuite

  Code reviews for ops (Terraform) and getting quality code reviews is challenging

• Chris from Segment

  During onboarding, responsible for presentation on communication, and code review is a type of communication

  On giving feedback: Both parties should be looking to extract good things out of it. Regardless of the feedback given, it is up to the receiver to try to find the good aspects in it.

  "Code sharing sessions": Code we admire, code we don't understand, code we have questions about

  When code review is "overhead" instead of a primary important function, where's pressure to get code reviews done. People feel like they've wasted their time if they get feedback.

• Colin

  Seemed like John was coming at it from a "money on the line"/deadlines perspective, but open-source side is different. Might not want to be rushing things, priority order might be different depending on the context.

How would you measure the effectiveness of your code review?

• Codecy that provides "objective" score of code quality
• Tools can be "nannies", part of Continuous Integration. Enforcing pre-agreed shared values.
• Look at complexity, coupling, and cohesion ("When in doubt, reduce coupling, increase repeated code")
• Look at patterns, "legacy" patterns that we don't want to use anymore
• Would you recommend this code change as an example to others? Was the change a surprise to anyone? If used in a retrospective, would it have been caught in a code review?
• Do you learn something from it? "Growth mindset"

Best/Worst experience

Some of the best have also been some of the worst. Learned a lot from critical feedback, the reviewer probably wants to help you improve.

Working on a large Rails codebase, got questions like "What are you doing?" "We don't do it this way"

Best reviews explain why, not just what or how. Having feedback be educational is very important.

Getting review fom people not involved in the original discussions, and without context

What about code review that leaves no comments or suggestions? Should always provide some sort of feedback? Ask for areas that you want feedback

How to review code that you're unfamiliar with?

Who does the code reviews?

Small team, can have senior dev review, but what about a larger team?

• Peered reviews where everyone on the team reviews, but then you get the same feedback multiple times

• Review "trio": Author, Reviewer, and Review Coach

• Code ownership and code maintainers, but that eats up time from other teams

• Whoever wrote the code should be able to give a review or recommend someone else to review

Questions

What have you done to make code review "cheaper"? Limit PR size

How to do meta-review (review of the code review)?