Build better code reviews

Crafting Better Code Reviews

This article is authorized by Vaidehi Joshi

The Nuggets translation Project

Translator: bobmayuze

Proofread by: SareaYu, eat dirt small 2 fork

A presentation from the Rails developer Conference 2017

The interaction between people and technology is always bright and dark. This is especially true for people who develop technology products. As an experienced coder, THIS was particularly evident to me during code reviews.

Most developers are used to thinking of their code as a work of art, like a painter’s painting, and our code is always relevant to us. We’ve always been taught to be altruistic code farmers, reviewing not only our own code, but our colleagues’ code before merging it into the main branch. We all know that such scrutiny is good for everyone, that it’s something we should all do, and that many of us already do these highly recommended things.

But who can remember the last time we measured these methodologies? Can we really guarantee that our code review system is effective? Can we ensure that our code review system stays true to its original purpose?

If the answer is no, how can we solve the problem?

Don’t review codeCan not pass

When we can fully understand the practical implications and benefits of code review, we can begin to understand why the tradition of code review exists. There is plenty of research on best practices for code review online, but I suggest starting with Steve McConnell’s work in Code Book (1993).

In his book, he writes about what code review should do:

Part of managing software engineering is capturing the “lowest value” phase of a problem, when the least investment has been made and the least money is spent to solve the problem. To achieve this expectation, we can use the concept of “quality gate”, which is periodic testing or review to determine whether to proceed to the next stage of development.

The most important concept in McConnell’s code review research is “collective ownership in code construction.” All code belongs to the team, not just one person, and can be accessed and modified by all members of the team.

The original purpose of code review is to help us adopt the idea of collective ownership in software development. In other words, each of us becomes a shareholder in the development process by participating in the control of product quality.

In his book, McConnell proposes several different types of code review processes that can be adopted by any team in their daily workflow. I highly recommend McConnell’s code book, it’s really great. But here we’re going to talk about three very briefly to help you understand.

Detailed inspection is a relatively long process, which usually takes about an hour. The whole process will include a company gatekeeper (the gatekeeper), a code writer (the programmer), and a reviewer (the product dog).

When this review is used effectively, it usually finds about 60 percent of the program’s problems (whether bugs or bugs). According to McConnell’s research, this system reduces errors by an average of 20 to 30 percent per 1,000 lines of code compared to less streamlined code reviews.

2. The walkthrough

Walkthrough, which usually lasts 30 to 60 minutes, is often an opportunity for senior programmers to teach the novice, but also an opportunity for the novice to explain new methodologies and challenge stale and possibly outdated assumptions.

Walkthroughs can be useful sometimes, but in general they are not nearly as effective as more formal code reviews. A walkthrough usually finds 20 to 40 percent of errors in a program.

Like the name, the vetting is very short. But this is all very in-depth vetting, and it’s quite difficult. Sometimes it’s just one line of code that’s changed, but it causes a lot of problems.

McConnell’s research found the following facts about small code reviews:

A group that reviews single-line changes found that errors dropped from 55% to 2% after code reviews. One communications agency in the late 1980s saw its code accuracy rise from 80 percent to 99.6 percent after review.

McConnell’s set of data seems to suggest that every development team should combine these three approaches to code review.

McConnell’s book, however, was written in 1993. By now, our workflow has been updated, and so has peer review. But is our current approach to code review sound and comprehensive enough? How should we apply what is theoretical to practice?

To answer these questions, I did what most digital farmers do: Google!

Well, that’s good. Above is a survey I launched on Twitter.

How do programmers feel about code review

Before I go any further with this survey, I’d like to say something: I’m not a data scientist (I wish I was, but I might be more comfortable with this post’s feedback, and I might be ok with drawing in R). The other thing is that my data set is actually very limited. First of all, I picked this data from Twitter myself, and the other data is from a branch/pull Request based team.

Okay, so here’s the thing: What do programmers really think about code reviews?

Let’s take a look at the quantified data first.

First, the answer to this question depends a lot on which programming monkeys you ask. By the time I wrote this report, I had received over 500 responses.

Here are some of the results, broken down by the language used at work, including JS, JAVA, and Ruby.

After I asked them one by one, everyone agreed that code reviews were good for the team.

Overall, on a scale of 1 (strongly disapprove) to 10 (strongly approve), the Swift development team rated code review approval at 9.5. The Ruby team came in second with a score of 9.2.

While 70% of respondents told me that their pull Request is reviewed by someone else on the team before being merged, 10% of respondents (about 50) told me that their pull Request is reviewed only when they request it.

This illustration illustrates the depth of code review in different languages. In general, there is not much difference between languages. In other words, determining the depth and frequency of code reviews has nothing to do with what language you use, but what team you’re on.

Finally, for teams that review code only when required, most teams typically only need one person to review the code before it is merged into the main branch.

Qualitative description of data

So what do we know about things that are not quantifiable? In addition to multiple choice questions, the survey also allowed respondents to fill in their own answers. This part is also the most important part of the survey, the most revealing part.

These answers focus specifically on the following key points. �

In general, there are two factors that have a big impact on code review: the resources consumed to perform code review and the sustainability of the code review process.

A very resource-intensive and poorly sustainable code review can make a code review very bad. If a code review is not very resource-intensive and is highly sustainable, it makes a very good impression on both the reviewer and the subject.

But what do we mean by resources and sustainability?

Another way to find out exactly how many resources a code review is consuming is to ask yourself questions like: Who is performing the process and how much time are they spending doing it?

A large number of respondents were productive code reviewers, but all expressed frustration with those on the team who performed the process and the amount of time they spent waiting for their code to be reviewed.

Here’s some anonymous feedback:

We had a developer who blindly gave every PR a like and didn’t leave a comment. I can tell you this is true because they can see 5 or 6 PR a minute.

I found that the second or third censor was mostly just playing around.

Sometimes the results of the same code review will vary depending on the submitter.

Everyone on the team should be treated the same. Senior engineers make mistakes, and they want someone to mention them. Junior engineers can’t be hacked. People are always judged by things.

The Commits are too long, causing PR to take a long time to review. People don’t test the code locally first.

Long PR takes a lot of time, and that PR can have a significant impact on future features.

There are three main comments on the relationship between code review and consumption of resources.

Everyone was upset about code reviews and didn’t think it would work.
Reviewing code you’re not familiar with is a pain in the ass.
Mistakes are all human. We are all human. We should treat them equally and not say that the code they wrote is ok just because they are the boss.

sustainability

Code review sustainability is mainly influenced by the following factors: what does the executor say and do when performing code review, and how does the person being audited feel

It’s all about what people say and how they say it.

Let’s take a look at the jokes:

In the face of PR, I think you can change the variable name directly if you don’t like it. I think this is not the most important thing, it’s just personal preference! As in the IDE, I was easily fooled. I don’t care why I’m unhappy, just make this thing stop making mistakes, it’s really unbearable to keep making mistakes.

Don’t talk about big ideological mistakes in public. Having a friendly conversation offline can be very helpful. Direct PR said it would be unpleasant, and it would end up making everyone unhappy.

I get very upset when requirements keep changing, especially if they don’t explain to me why they changed requirements, or leave the possibility for them to make mistakes. Especially when someone tells you to rewrite your code to make it their version, it makes you want to hug.

When a reply is too long, we might as well have an offline conversation.

I think the question of personal preference and whether the feature works are two separate issues. For a junior engineer, this is very difficult to distinguish. Sometimes it’s even more confusing when several senior engineers give different feedback.

In general, code review focuses on the following:

Feedback focused too much on grammar and habits, resulting in a lot of frustration on both sides. Code habits and style and code functionality errors are two different things.
The way you speak is also important. Aggressive language can undermine confidence and is not good for the team.

This data may not be the most complete, detailed, or accurate representation of the structure of code review, but we can still learn something: we can review and examine the code review process of our team and the community as a whole.

The following anonymous survey shows how code reviews affect a team member:

One really bad code review almost made me quit. A good code review process will give me confidence to tackle more difficult projects in the future.

Indeed, having a streamlined code review system seems to be very effective and helps teams grow quickly; Steve McConnell and this article’s survey prove it. But simply refactoring the code review process and never changing it is not enough. In fact, the code review process should be tailored to the overall team to ensure that the team grows.

Rather than simply adopting a code review approach, we need to rethink our code review process and streamline our work to help us face future problems.

In other words, it’s important that we think about whether our code reviews are effective enough to benefit both the team and the individual.

Some tips for improving code review

Here are some quick tips to help improve the feel of code review:

Use Linters and other code analysis tools to avoid syntax problems.
Use GitHub’s template to produce each PR. It’s very helpful for both authors and reviewers to have a list of changes with your PR.
Add a screenshot to your PR to help people who aren’t familiar with the question understand it.
Improve commits, trying to be short and skillful.
For each PR examiner, it is better to have more than one if possible. Make sure that the distribution of code writing and review is balanced across all levels of engineers.

Here are some of the more important things to consider once you’ve gotten started with code review. In fact, this is what matters most to the refactoring code review process.

Warning: high energy ahead, the content may be difficult

Understand your team. This task is usually undertaken by senior engineers, we hope you can help the new people.

More subtle communication. That means thinking about what you’re saying or saying before you post a comment and whether it will upset others. It is very bad to criticize others in public. It’s better to have a private conversation.

Have an oral communication. Sit down with your entire team and start a new channel on Slack, where people can communicate anonymously (whichever works best). People are willing to tell the truth under the condition of anonymity.

I’m saving the most important point for last, because if you have the patience to read this, you really want to change your code review structure, which is a good thing. Team communication is really important, and it’s a step that almost every team must take if they want to improve code review.

The following sentence basically includes what I most want to say:

In theory, I love code reviews. In fact, it depends on the team building the process.

The related resources

If you want to see more anonymous feedback, you can check out the following website about the project:

Thank you

First of all, I would like to thank my engineer friends who have supported me all the way. Thank you for your time and energy to participate in my investigation.

A big thanks to Kasra Rahjerdi for helping me sort through the feedback and generating so many images.

Thanks to Jeff Atwood for his article on cross-checking, Karl Wiegers for his humanized cross-checking process, and Steve McConnell for his research on Code Complete. I hope you will support them by purchasing their books.

The Nuggets Translation Project is a community that translates quality Internet technical articles from English sharing articles on nuggets. Android, iOS, React, front end, back end, product, design, etc. Keep an eye on the Nuggets Translation project for more quality translations.