No matter how experienced we are, we are bound to make mistakes in our work.

This simple fact is one of the reasons why Code Reviews have become the cornerstone of software engineering processes around the world.

Part of my job is building teams and establishing best practices for them to operate efficiently and collaboratively.

But team practices shouldn't just be coming from an individual, which is why I first interviewed a bunch of our engineers at The Browser Company around what is the goal of PR's and Code reviews.

In the spirit of giving back to the community, we've decided to share those practices.

Code Reviews and PR’s

We aim to turn PRs around in under a few hours for normal-size PRs, and no longer than a day for more challenging ones

Code Reviews

What is the goal of Code Review?

The goal of Code review is to maintain a high quality of codebase and product.

Code reviews also give us opportunities to share knowledge about the codebase and give PR authors the confidence that their design approach is sound.

Think about the reviewer role as someone that double-checks your parachute before you are about to jump from a plane, we know you checked your straps but we’ll double-check it for you because we care (paraphrased from Ei-Nyung Choi).

Since we expect long-lived code to be read more than it is written, we want to optimize for readability & understandability. Code-review focuses attention on this goal before code is merged.

Pull requests also serve as documentation of how to do things and what our team culture is, often times you’ll find yourself looking at ‘How do we implement X’, finding the PR that something similar will be a great help to you in those cases.

Understanding how things are done and seeing the discussions and considerations that went into them will help newcomers understand the approach and trade-offs better.

Embrace and Encourage a positive culture

Peer review can put a strain on relationships, especially in asynchronous written communication when we lack signals like tone of voice/face expression can be misinterpreted as crude/rough, etc.

Code review is also a place for sharing and applying our company culture in practice. From "how to communicate" to "code base styles", it's a big part of the communication between devs in the company, and a way to learn & align with common practices and improve them.

Assume the good intentions of the reviewer and their comments, they are only reviewing your code and in no way is it a reflection of your skill or value as an engineer. We all make mistakes.

Code review presents learning/teaching opportunities for all participants. If either the code being reviewed or changes being suggested by the reviewer seem mysterious, ask for more information.

For example, a newcomer reviewing a subject-matter expert’s PR is a great opportunity both to learn by asking questions and to suggest new/better ways of doing things from an outsider’s perspective

Remember that we’re building a team that values learning over the process.

Examples:

Good feedback:

• “It looks like this might run in n^2 time – is there a way for us to improve it? maybe we could leverage hash map to make it linear?”
• Why it’s good → describes the potential issue, suggest an alternative approach that can improve it
• “I think this is making a network call within a tight loop. Maybe add a bulk API on the server side so that we can do the work all at once in a single call?”
• Why it’s good → describes the potential issue, suggest another option, and why it’s better
• “Could we leverage memoization technique to improve performance here?”
• Why it’s good → ask questions, provide reference to what you mean rather than assuming the person knows the technique. It uses “team words” like we, making it explicit that we are a team, doing this work together
• “I know you’ve given this a lot of thought and I am probably missing something here. Can you fill me in on why you decided to do X, rather than Y? Thank you!”
• Why it’s good → acknowledges that the writer spent more time than you as the reader, and asks them to provide more detail about their choice, before asking them to take approach Y instead

And not so good feedback

• “Style guide calls for two newlines between top-level file comments.”
• Why it’s not good → We should adjust auto formatter / linter to do this kind of work for us, not engineers.
• “I didn’t understand how this worked but if you’re sure it produces the right results, then ok.”
• Why it’s not good → If a code can’t be understood by our engineers, we should request it be simplified or add additional documentation in the code to make it understandable.
• ”This way is not right. You don’t understand how this should work. Use XYZ function/whatever instead.”
• Why it’s not good → This uses very absolute wording, where’s most of the time there is no one right solution for an engineering problem, it doesn’t provide the reason why this approach might be incorrect or inferior to alternatives. It also doesn’t encourage shared ownership by using ‘You’ instead of ‘We’.
• “Can we do X? ”
• Why it’s not good → This is almost good but a teammate can’t tell if this is a blocking change, a nit, or a suggestion up for peer debate/judgment call.

Priority and timing of code reviews

Code reviews should be prioritized above writing new code because it unblocks other developers. But don’t feel the need to stop what you are doing when a new PR request comes in, finish your focus block and do a review in the natural spots when you are not in deep-work e.g.

• After scheduled meetings
• Allocating some time at the start/end of your workday

People should DM via Slack if it’s urgent but that should be a rare situation.

We aim to turn PRs around in under a few hours for normal-size PRs, and no longer than a day for more challenging ones

Code reviews are expected to happen during reviewers’ normal business hours, so adjust the above intervals depending on relative timezones.

Kicking off feature work

Code reviews typically go a lot faster if major changes & feature designs are discussed ahead of implementation with prospective reviewers, and minimize the odds of painful rewrites being suggested.

Sometimes writing a rough draft of the code can help flesh out ideas and GitHub’s Draft PR functionality can be used to discuss the results.

Feel free to reach out to subject matter experts for informal discussion or tupling.

Pull Request’s Best Practices

Describe what and why changes are made

Leverage our PR template, a good PR description usually has the following structure:

• High-level goal and reason for the changes
• Start by providing context for the reviewers, e.g. why you are changing something within this PR. Don’t skip this part, it’s more important than the changes themselves as they are easy to extract from the git history, whereas the reasoning behind them is often lost to time.
• Add any relevant context links/references
• Adding bug reports/feature requests or discussion links is going to make the PR easier to understand and make the reviewer job easier, and shorten turnaround time.
• Context is not only helpful to reviewers but it will also serve as historic documentation when in the future we need to understand the code related to the changes in question.
• Describe the changes that were performed as part of the work (e.g. had to refactor X to accommodate for Y requirement)
• Test scenarios/steps listed out for verification of the changes

By following that structure it gives good context to the reviewers:

• is this safe to merge / what would it break?
• does this do what it's supposed to do?
• is there another way we could achieve the same goal?

Remember that more context means faster review and hence faster approval.

Optional mental exercise: Ask yourself if you could send this to a new engineer to help them understand this area of code, our best practices, and how we make decisions.

Scope and Size

The goal is to make PR’s that can be treated as independent changes, usually we should strive at making PR’s not larger than 200-400 lines of code changed.

We understand that sometimes it’s not possible but that should be the goal for efficient effect discovery.

Smaller PR’s have lots of benefits:

• Less strain on the reviewers time
• Faster turnaround and quick feedback loops
• Fewer merge conflicts
• Can lead to better abstractions and modularization

Submitting for Review

Self-review before submitting the PR, it helps you spot obvious mistakes and areas for improvement, saving time for other reviewers and shortening the feedback cycle.

Favor discussion in PR rather than direct messages on Slack, as this will help with documenting the changes and reasoning for historic/debugging purposes, as well as encouraging other people to join in with suggestions for improvements.

How many people should review a PR?

We ask for at least one reviewer, but generally recommend assigning 2 people since code review also serves to share knowledge which is worth considering when choosing (possibly “extra”) reviewers.

If you are going to change any fundamental architecture or functionality of our product, assign at least 1 subject matter expert as a reviewer, ask in #eng-swift #eng-chromium or #eng if it’s unclear who knows that part of code well.

For small changes, any single engineer is fine as a reviewer.

Writing clearly labeled comments

Try to be clear in your comments/suggestions, provide sample code or links to documentation/best practices.

Start your comments with a Tag to your comments to make it easier for the author to understand how important they are:

• Nit: small change e.g. re-naming function to be more clear
• They can be implemented as follow-ups
• Optional: what the name says, can be ignored
• Blocker: Mistake or wrong code, this is a blocker. The PR should be marked as ‘Request Changes’

Dealing with feedback and updating PR’s

If a PR is marked as requested changes by someone, the requested changes have to be discussed and resolved before merging.

If your PR has questions posted by one reviewer, and another one approves it, you should still answer the first reviewer's questions and clear any confusion before merging.

As an author, you should always respond to or acknowledge each review comment even if it’s just saying “Done” or resolving it. It’s difficult as a reviewer to tell what happened when the comment lingers and the author didn’t respond to it.

If your PR gets approved you shouldn’t be modifying it non-trivially. Sometimes you might realize a mistake in your code or the request lead to significant code changes, in that case, add a request to review the PR again for all the people involved in the original process.

A note about complexity and tech debt

In addition to being a complex application with many running parts, our project is also long-running unlike iOS applications.

It doesn’t get killed off by the system regularly, the app session for many users can be until the next update is released, which can mean weeks. This has serious implications for tech debt and code quality:

• Memory leaks will keep adding up
• Any leaked objects can be listening to side-effects causing additional performance issues

In addition tech debt will make understanding the codebase harder and harder over time, slowing our ability to develop and making the logic of the app hard to follow

All of the above adds up to a requirement that we maintain a high-quality bar for our codebase. When writing or reviewing code it’s good to keep this in mind, and err on the side of quality over expedience (within reasonable limits, obviously).

Best Practices for Reviewers

• Always start with the big picture, read the description, and understand what the change is and what’s the reason behind it
• Look at whether the general architecture approach makes sense?
• Think about how you would solve the problem
• If it’s different than the author solution consider why is that, this might be a knowledge-sharing opportunity
• If your idea can be simpler/safer with the same functionality or covering more cases, suggest it as an alternative and discuss pros and cons
• Does the code work as described?
• Are all the requirements fulfilled?
• When someone asks you for a review they care more about architecture/clarity and quality suggestions than code style adherence, we should automate our code style via swiftlint/swiftformat not put the strain on the engineers writing or reviewing the code
• If some piece of code seems complex enough to require extra review time, consider asking the author if it can be simplified.
• Optimize for code reading, not writing. Abbreviations, character-count golfing, etc. are usually worth pointing out as opportunities for improvement.
• Explain your reasoning behind suggestions
• Ask questions rather than pass judgment, assume competence and good intent
• Explain and discuss the pros and cons of different approaches
• If a review takes longer than an author feels comfortable with, consider suggesting following up in another PR or create a bug to track remaining improvements. This typically requires case-by-case judgment; feel free to ask a teammate for another perspective if you’re unsure what to do.
• Be kind: compliment good design decisions and efforts to improve existing code, and approach opportunities for improvements as such.

Code Review Checklist

What you should be looking for in a code review?

• Finding defects before they go into production
• Readability and understandability, being able to maintain the code we merge into main is crucial to the longevity of our product
• Follows our established architecture unless there are explicitly stated reasons to not do so
• Fulfill the feature requirements, meaning the code is correct from a usability perspective.
• For user facing features there should always be test steps provided for the scenarios we are trying to implement and they should pass. Additional unit or UI tests are always encouraged.
• For other work we should leverage unit tests.

Attributions:

My teammate's at The Browser Company:

Ami, Ei-Nyung, Piotr, Maya, Stephan and Adam

What do you think?

If you'd like to join our mission of building a better way to use the internet: we are hiring!

Do you disagree with any statements in our practices, anything you'd do differently or improve? I'd love to hear your opinions, hit me up on twitter: