Code Reviews 🤓

iteration

Welcome to Iteration, a weekly podcast about programming, development, and design.

• My name is JP, I am a software engineer at Opendoor. Today I am joined by John
• John Intro

This week on code reviews

What makes good code review? Here's a link I found on reddit a while ago:

http://cassandra.apache.org/doc/latest/development/how_to_review.html

https://www.reddit.com/r/cscareerquestions/comments/byxmk1/what_makes_for_a_good_code_review_session/

Another tweet:

https://twitter.com/addyosmani/status/1198502828425150465

• Any good / bad experiences with CR?
  • JP: It's not personal
  • John: I've worked with team members who take feedback as a prescription for every time
    • Rant about chefs, recipes and concepts
  • John: People not giving the PR in context. It's flagged WIP and then calling out a comment or a long method. Focus on the approach not the syntax at this point.
  • John: I give code reviews for my clients team or other agencies.
    • Can feel like a power struggle.
    • I have to sometimes be open minded about solutions.
    • If tests are passing and it's reasonably documented and maintainable, it gets merged.
    • Example: Very javascript heavy interaction that could of just been Markup
• What was your first CR like (receiving it and giving it)?
  • JP: it took me a while to get comfortable leaving code review for people who I looked up to. +1
  • John: It's hard getting feedback from the team who works under you, they can be shy about it. Can be frustrating. That's why at several points I've literally paid a tutor.
• How frequently do you do it?
  • John: I get reviewed once a week. I give reviews multiple times a day.

How is CR Conducted at John's agency vs at Opendoor?

• JP: Different kinds of PR's - WIP, Ready for Code Review, etc
• JP: CR Etiquette
• John: It's pretty informal — working on stronger processes around this. We "Sometimes" do a WIP review. Lead Dev or I always do a final review before deployments.
• John: For more "final" reviews, I try to summarize my thoughts into an actual checklist into the main comment body.

CR Tips

• JP: Take your time with it
• JP: Pull the code down and run it. Tinker around. This helps me see the bigger picture
• JP: Know when to leave nit pick comments.
• JP: Think of the potential test cases before you read them.
• John: Giving Good feedback
  • Consider the McKinsey Approach
    • Permission
    • Observation
    • I noticed that... Have you considered...
    • Try to take ego out of it
    • Never assume
    • Compliment Sandwich —
    • Bring it all together: Wow, this was a lot of hard work. Great job overall. I noticed that you brought in JQuery as a dependency. Have you considered using Vanila JS instead? That way we keep our site fast and avoid possibly uneccisary dependencies. Here's an article that might help. Very impressed by your CSS skills in this. Keep rocking!
• JP: Get other engineers involved +1 Mob Review
• John: Ideally the person who submitted the PR takes the time to fix their own code. Sometimes you've just got to get code live.
  • Async "Pair" — turn on screen recorder, walk through all your comments as you fix them. Do this when code is pressed for time.

---

Picks

• JP: https://www.gitify.io/
• JP2: https://www.reddit.com/r/cscareerquestions/comments/byxmk1/what_makes_for_a_good_code_review_session/
• John: Rocket Emoji's - https://matthewpalmer.net/rocket/