Improve code reviews without going pro

David Stanete
May 6, 2020 · 4 min read

At some point in a Product Engineering team you will have to address some issues related to code reviews. In some teams, code is pushed without barely looking at because reviews are too light. In others, code stays there for too long because reviews are too harsh. In others, pull requests are completely abandoned because nobody actually reviews code in their day to day.

Some more mature teams like Netlify arrive at creative solutions like adopting a metaphor to give feedback on code reviews. Others, like Squarespace, create more extended guidelines to submit and review pull requests. Google as even a more extensive guide. It doesn’t have to be so pro. It can be something simpler. I want to share my approach for the teams I worked with. We actually have this written down in a Confluence page.

As a rule of thumb, a pull request must be reviewed by at least one team member from another team. It doesn’t avoid knowledge silos, but it helps to have an idea of what is happening in other teams. For that, we need to have some common rules about submitting and reviewing PRs, so the experience is as uniform as possible.

Submitting

The responsibility of submitters is to make the code as easy to understand as possible. To make the reviewer’s job easier, the submitter should:

Use a pull request template and answer the following questions:

  • What does this PR do?
  • Why do we want to do that?
  • What high level changes did you make to the code?
  • What other information should the reviewer be aware of?

Provide information like UI screenshots or GIFs, link to the JIRA ticket, etc.

More than 300 changes is a red flag. More than 10 files is a red flag. There are exceptions like renaming a package or moving files around.

Unless the code is intended to be a public API consumed by others. This is controversial, but if the code we push to production is not used, how can we ensure we are adding value?

Reviewing

The responsibility of reviewers is to ensure they accept the code as theirs and will take care of it as if it was written by themselves. Different software engineers will look for different aspects of the code, but to ensure a fluid experience for everyone the reviewer should:

First thing in the morning, before lunch, after lunch and last thing in the afternoon. Having at least four times in the day where code is reviewed, helps quite a lot. It ensures PRs don’t stay too long without anyone looking at them. It also avoids personal Slack review reminders. You can use Trailer to accelerate your GitHub workflow but it’s not mandatory.

Don’t be too light on the code review. Remember that you are accepting the code as your own. Also don’t be too harsh. Different software engineers have different ways of solving a problem. For a code to be accepted, it only has to fulfill that:

  • It achieves the given task
  • It has been tested and the tests are passing
  • It follows the agreed architectural guidelines
  • It has a manageable size
  • There is no unused, dead or duplicated code
  • The system is not going to be harmed
  • It doesn’t introduce unnecessary complexity

Return to the PR if there are requested changes and propose alternatives to solve them. Do a pair programming session if there are too many requested changes.

Wrapping up

Adapt it to what your team actually needs. Address the issues your team is struggling with and write something down. If you want to go pro, do it. Write more extensive guidelines or automate even further. But you don’t need to go that far to start improving code reviews.

Whether or not you should be even doing pull requests is a topic for another post.

Huge thanks to undraw.co for the illustrations. You can read this post in Spanish here.

Interested in working with us? We’re always looking for people passionate about technology to join our crew! You can check out our openings here.

Creditas Tech

Our technologies, innovations, digital product management…

David Stanete

Written by

Show, don’t tell. Tech lead at @CreditasTech & helping at @gdgvalencia. Formerly @MercadonaTech & @chicfy

Creditas Tech

Our technologies, innovations, digital product management, culture, and much more!

David Stanete

Written by

Show, don’t tell. Tech lead at @CreditasTech & helping at @gdgvalencia. Formerly @MercadonaTech & @chicfy

Creditas Tech

Our technologies, innovations, digital product management, culture, and much more!

Medium is an open platform where 170 million readers come to find insightful and dynamic thinking. Here, expert and undiscovered voices alike dive into the heart of any topic and bring new ideas to the surface. Learn more

Follow the writers, publications, and topics that matter to you, and you’ll see them on your homepage and in your inbox. Explore

If you have a story to tell, knowledge to share, or a perspective to offer — welcome home. It’s easy and free to post your thinking on any topic. Write on Medium

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store