September 22nd, 2017

How We Do Code Review

Visual Studio Mobile Center is now Visual Studio App Center. Learn more here.   The Mobile Center team is a diverse mix of engineers, product managers, and UI/UX designers with varying professional backgrounds, experience, and countries of origin. Our team has people from over 10 countries working across 8 time zones. Some are fresh out of college, while others have been in the industry for more than 20 years, and we all came in with our own understanding and definition of “code review.”

We spend a significant part of our day doing code review in GitHub and Visual Studio Team Services, so we welcome anything that makes it faster, easier, and better. After pinpointing several common pain points, we established and widely circulated a common set of code review principles for submitters and reviewers.

I’m sharing how we’ve reduced friction and, most importantly, have more time to do awesome work. Any team can use what we’ve learned to improve their code reviews, regardless of size or end product.

Common Issues

Issue #1

One of the biggest causes of frustration? Pull requests that sit open, in limbo for days or weeks. Pull request lags result in:

  • Bottlenecks: Long-running pull requests block people from continuing with their work, especially in situations where many others depend on the code being merged and deployed (e.g end-to-end integration tests).
  • Wasted work: As pull requests age, they become stale and out of sync with upstream branches unless the submitter regularly rebases or merges upstream changes while waiting for the initial review.
  • Uncertainty: Open and unfinished pull requests add mental load for submitters; they need to constantly check status, make updates, and be on alert for reviewer comments or change requests.

Issue # 2

Pull request comments easily become a rabbit hole, drifting off and focusing on a small, irrelevant area. In our experience, this is due to differing opinions between submitters and reviewers, not objective facts, and results in a stalemate.

The Golden Rule of Code Review

Before diving into reviewer and submitter-specific guidelines, keep in mind one common, critical rule:

Respect others, yet don’t take anything personally! We all make mistakes at some point. What’s important is to learn and improve, and to treat others how we’d like to be treated.

Everyone’s opinion matters; some might be biased, some might be outdated, and some of them are problematic in a particular context. It doesn’t matter. We work together, communicate openly, self-check our feedback, and try to be as objective and fact-based as possible.

As a submitter, remember that reviewers have their own tasks, deadlines, and improvements to implement. They’re taking time away from their work to help you make your code better.

As a reviewer, remember what it’s like to be in the submitter’s shoes and offer honest, prompt, actionable, and detailed comments.

Following “Matz is nice so we are nice” has also never hurt!

Tips for Code Owners and Reviewers

Provide clear, obvious contribution guidelines.

Don’t make contributors wait until they get review feedback to learn basic guidelines, such as style requirements, test requirements, and so on.

Document your guidelines in a clearly visible spot, like your Readme, and update it frequently, especially when things change, or many contributors raise the same questions. This clear documentation extends to things such as your general architecture, where to look for re-usable code, and support infrastructure (e.g. Make, Rake, or Gulp tasks to automate things). Why? This gives new code contributors an easier entry point, so they get up and running faster and don’t have to spend cycles trying to figure out how to implement certain functionalities.

Clear contribution guidelines set contributor expectations from the get-go, simplify adding changes, and result in better code overall. If it’s easy to get started, you’re more likely to get more (and more frequent) contributions.

Helpful advice for how to navigate the codebase and get started faster will very likely earn you an extra gold star 🌟!

As a submitter, it’s frustrating to see your code sit for undefined periods of time. Document when and how quickly the team reviews code, so submitters know what to expect.

Don’t criticize the submitter, point out flaws in the code.

We’re told not take it personally, but being told that you’re wrong—that something isn’t right with the code you’ve spent hours on—isn’t a great feeling. Don’t criticize the person submitting the code, point out issues or technical problems. You’re there to act as another set of eyes, provide a different perspective, and notice changes that cause unwanted side effects. Is the code unnecessarily complex? Do you see performance issues? Point those out!

The language you use matters, so be mindful about how you phrase your comments. For example, we don’t use “you made a mistake” opting instead for phrases like, “If we leave this as-is, we might run into [rationale or possible undesirable outcome] problem down the road,” or “This line could cause us trouble because…”

Automate all the things

We’ve learned that we prefer being reprimanded by a robot over having our teammates call out our indentation style violations. Automatic linters, like Rubocop, StyleCop, TSLint, and ESLint are a code reviewer’s best friend; simply run them automatically during the build process to catch obvious flaws early.

Let robots (like TSLint) catch easy mistakes and focus on giving more valuable feedback.

More advanced tools (we like Danger) catch things that aren’t directly related to the code. For example, I’ve picked up that someone forgot to update a Changelog, if the pull request impacts critical pieces of the codebase, or vice versa when a pull request fixes documentation typos or other trivial changes.

Be a role model, and focus on what matters.

It’s easy to ignore your own rules, but the reviewer who expects high test coverage shouldn’t commit untested code.

As you review code, realize you may make similar “mistakes” that you point out in others’ contributions, and don’t obsess over minor style details. If the code doesn’t obviously violate a major style rule, there’s no reason to force a style—let the committer use their preference.

We apply the same rules to all contributors, ensuring we’re not overly critical, and we hold every team member accountable for practicing what they preach. We note obvious flaws, readability issues, or hard to follow code, which allows our teammates to quickly understand and address big-picture issues, rather than getting mired in the details, and focus on delivering a better user experience.

Feedback should be actionable and concrete, not overly opinionated and theoretical.

Watch your tone.

Tone is important, and one of the first things to go when you’re in a hurry, trying to finish another review, and just want to get back to your own code. But, how we communicate is tremendously important and makes a big difference (even more so for distributed, international teams like ours!):

  • “I have a bad feeling about this line of code” isn’t a well-rounded argument.

Instead:

  • Explain what could go wrong and why the code needs to change.
  • Communicate your context:

“We used a similar pattern in the past and it failed because of X.”

“I recently read a blog post warning against this. Here is the link…”

“Here’s something that helped me in the past…”

Engineers are problem solvers; that’s what makes our job fun, but don’t try to force your solution.

  • Point out problems. Ask “how could we make this better?” not “here’s how we should fix X.”
  • Ask clarifying questions rather than immediately concluding that the code is wrong.

Introducing the Emoji Code

Tone is complex enough, but written feedback makes is difficult to convey meaning. After a few misunderstandings about what certain comments meant, we implemented the Emoji Code™*. While it’s not a substitute for written feedback, we incorporate emoji as an indicator for the kind of comment we make whenever we can.

Our most used Emoji and their “definitions”:

  • 👍 or `:+1:`: This is great! It always feels good when somebody likes your work. Show them!
  • ❓ or `:question:`: I have a question / can you clarify?
  • ❌ or `:x:`: This has to change. It’s possibly an error or strongly violates existing conventions.
  • 🔧 or `:wrench:`: This is a well-meant suggestion. Take it or leave it.
  • 🙃 or `:upside_down_face:`: This is a nitpick. Normally related to a small formatting or stylizing detail that shouldn’t block moving forward.
  • 💭 or `:thought_balloon:`: I’m just thinking out loud here. Something doesn’t necessarily have to change, but I want to make sure to share my thoughts.
  • 🤡 or `:clown_face:`: This is a complaint about something with no obvious answer, not necessarily a problem originating from changes.

The Emoji Code helps us separate well-meant suggestions, simple questions, and must-have requests that require code changes. It also adds an additional human component to the conversation, so we don’t forget there’s a human on the other side of the screen.

*Just kidding; we haven’t trademarked Emoji Code, so spread the word far and wide and make it your own!

An example code review, complemented by the Emoji Code.

Tips for Submitters

Follow basic etiquette.

We’ve established a few common ground rules, or “Mobile Center Code Review House Rules,” as a baseline for all submitters.

  • Acknowledge prior feedback: It’s frustrating to correct the same issue multiple times, so learn from earlier reviews and show reviewers that you respect and appreciate their advice.
  • Be clear: If you disagree with your reviewers’ suggestions, make that clear. If you plan to implement your reviewers’ suggestions, make that clear too.
  • Stay open-minded: Don’t try to “win the argument,” instead have a pragmatic conversation.
  • Limit code review discussion: If one or two comments back and forth doesn’t resolve a problem, it won’t be solved in code review. Instead, talk to the reviewer in person, on the phone, or via chat. Remember, it’s okay to agree to disagree.
  • Embrace change: If you don’t have a strong opinion, take the reviewer’s suggestions.

… and, for all-star code reviews: ensure all team members, including those spread across offices and time zones, get a chance to look at changes before you merge them*.

*We’ve become more conscious of features that allow “auto-completion” (automatically merging the changes whenever a CI build succeeds and at least one reviewer has approved the changes). Super useful, but, depending on the scenario, you may want to disable and give your teammates a chance to weigh in.

Remember that smaller = better

Small pull requests are win-win-win. They’re easier for reviewers to grasp right away and quicker to review since there’s less code to review and comment on. For submitters, this fast turnaround makes it more likely that changes get merged faster, and any team member who depends on the code can start working on their next project.

Bundling changes in business logic with refactorings of technically unrelated code in the same file might be tempting, but don’t do it. This makes pull requests unnecessarily large and unwieldy, causing confusion for reviewers and delays in feedback.

We love when people care for code quality and want to improve it, but create a separate pull request.

Respect the codebase and the core maintainers’ decisions.

Anyone who’s contributing to open source projects on GitHub, GitLab, or Bitbucket quickly learns that maintainers rarely accept code that doesn’t fit existing styles and patterns. The same goes for different groups in the larger Mobile Center team; some members have different preferences than others. Therefore, it’s important to respect the rules of the team that owns the codebase.

Our guidelines:

  • Don’t ask maintainers to accept contributions without proper review: “LGTM” (a.k.a. “Looks Good To Me”) is the easiest, least time-consuming reviewer response, but it’s harmful to a codebase. If you know your reviewer only signed off because you applied heavy pressure (“I’m blocked by your review.”), it doesn’t help anyone.
  • Use existing indentation style: Be consistent with what’s already there; nobody wants to have the age-old fight over tabs, spaces, 2 or 4, over and over again.
  • Add tests: Any serious project includes tests, and you’re expected to add or adjust tests to cover your changes. If a project doesn’t contain tests yet, they’re always a good idea—consider being a pioneer and authoring the first ones!
  • Recognize and incorporate architectures and coding patterns: Maintainers need to establish and enforce some level of consistency in how things are structured or abstracted so projects don’t become a confusing mishmash. Identify these commonalities and adhere to them.

Be your #1 critic.

Seriously—review your own code! Create an actual pull request to see your code like your reviewer will. Similar to proof-reading paper text, you’ll likely detect more errors.

We’ve uncovered lots of small issues across the Mobile Center team when we look at our changes as a whole, and we’re able to fix them before anyone else spots them or wastes time catching a simple issue.

Respond to feedback ASAP.

Review your feedback immediately, ask any necessary questions, and make required changes. If your pull request sits, your reviewer’s feedback gets stale and it’s likely that other code conflicts will render it un-mergeable. Check for reviewer comments or requests frequently (every one or two days), and act quickly.

Winning Code Reviews

Code review is the epitome of software development: teamwork, quality control, problem solving, and continuous learning all in one. Regularly reading other people’s code makes you a better programmer, because you’re forced to understand other people’s code and decision making and familiarize yourself with common patterns, coding styles, and business requirements, and your code benefits from it, too. On the other side, submitting code is humbling, forcing you to see what you’ve built from an outside perspective and identify ways to make it better.

For us, the Emoji Code, keeping our comments human and friendly, and codifying our Code Review best practices make us better developers and a better team. The result: we stay focused, continuously committing and releasing code, features, and integrations to help you to deliver even better code and amazing apps.

Log in or sign up for your free Mobile Center account, connect your latest project, and share how you code review.

Special thanks to Sven for writing large parts of our internal code review guidelines and generally pushing our engineering culture forward 💪

Author

0 comments

Discussion are closed.