DEV Community

Cover image for Those “Pesky” Pull Requests are Totally Worth It
Nick Hodges for LinearB

Posted on • Updated on • Originally published at devinterrupted.com

Those “Pesky” Pull Requests are Totally Worth It

Pretty much everyone does code reviews. They’ve been around a long time. I remember back in my Borland days when the Chief Scientist would come in every morning and review all the code that had been checked into the Subversion(!) repository the previous day and send emails out to folks whose code wasn’t up to snuff. That’s old school.

Slightly less old school? Saving all the check-ins up until Friday for the Dev Leads and/or Dev Managers to review and approve. Both of these techniques leave a lot to be desired — the main thing being a complete lack of interaction between the developer, the code, and the reviewer.

Code Reviews have a number of purposes. Probably the most important one is preserving the quality and integrity of the code in the repository. Even the two old-school ways above do that.

But almost as important is the learning opportunity that code reviews can provide. If the only feedback a developer gets from a code review is mistakes in formatting or other trivial things like that, then nobody learns and gets better. The old school ways above provide for few opportunities for a developer to increase their skills. To provide learning opportunities, code reviews evolved into meetings where everyone looked at the code written that week and commented on it, criticized it, or otherwise ran it through the gauntlet. This did provide a learning opportunity for developers, but it took more time, as it was 100% synchronous and required all code to wait for the next scheduled meeting to be reviewed.

Alt Text

Now, almost no one is doing these old-school code reviews anymore. All the cool kids are doing pull requests. (Some folks call them “merge requests.”) Pull requests have a number of advantages over the previously mentioned methods, including:

  • Being done completely asynchronously, but in public for all to see.

  • No one needs to wait to review the code — it can happen almost immediately after a pull request is issued.

  • A history of all the comments stays with the code in a repository. This allows a developer to come back to the code a year later and see all the thought that went into writing it.

  • Pull Requests can be tracked, monitored, and measured. A whole lot of good things can come out of that .

Should you do code reviews at all?

Interestingly, some say no, you shouldn’t.

Not only is Jessica Kerr a great speaker and a good Twitter follow, but she also has some interesting ideas about code reviews in her article of March 27 entitled “Those pesky pull request reviews .” In fact, she doesn’t like pull requests, and argues that you should sidestep them by just working on a given task as a team, so that everyone sees everything as the work gets done.

She believes that pull requests work great for open source projects where a “team” is really a set of individuals coordinating work together. For true development teams, she believes that if a team all works together on a single task, everyone learns and understands the code, and thus there is no task switching between coding and doing pull requests because the pull requests are unnecessary.

Jessica’s idea is radical — basically going beyond Pair Programming and moving into mob programming. Mob programming is the idea of having whole teams work together on projects in serial rather than individually in parallel. Mob programming can eliminate the need for pull requests by causing all of the communication and learning to take place during the coding phase, without any review.

Not a Fan

I’m having a hard time agreeing with her idea for a couple of reasons:

  1. The transaction costs are too high. It seems to me that having four people work on a project together makes for many communication channels, increases the likelihood of interruptions, and reduces the amount of code that will actually get written. It’s sort of a “Too many cooks spoil the broth” notion.

  2. It doesn’t capture the discussions and history that will remain long after the code is committed. One of the most important and powerful benefits of pull requests is the learning that can take place during and even long after code has been reviewed and deployed.

  3. Finally — not all projects are conducive to multiple team members working together. Some are small and multiple people working together would be overkill. Some are esoteric and require the focus of one person. Some will match the team and can be worked on together. There’s no one size fits all solution for all projects.

  4. Finally, not doing pull requests pretty much eliminates all the benefits of metrics systems like LinearB . Tracking the progress of pull requests and code reviews through the pipeline is a critical process for knowing how your team is performing. Without that, you can't measure things and if you can’t measure things, you can’t improve.

As part of a discussion about code reviews, Rob Kraft, one of the Development Leaders in our vibrant Dev Interrupted Discord Server (you should join!) made the following comment that I agree with:

I think that what Jessica needs is a good look at LinearB. 🙂

Let me address some of her more specific objections:

  1. Let’s face it: nobody wants to review pull requests.” Well, I don’t think that is true. We here at LinearB see customers every day that are doing pull requests efficiently and effectively. Sure, pull requests can be hard and nobody wants to do them if you aren’t correctly incenting the team to create pull requests that are easy to review. No one likes a huge pull request. But through monitoring metrics like Pull Request Size, you can encourage your team to create small, easy-to-review pull requests. And voila! People don’t hate pull requests anymore.

  2. They’re a social interaction minefield!” People complain that code reviews can cause strife on a team. Well, so can conversations during Mob Programming. I’m not sure that I see a distinction. And if doing a code review causes strife, then you have a cultural problem that no development methodology is going to solve.

  3. We could blame the people. We could nag them more. We could even automate the nagging!” Well, if code reviews are small, concise, and easy to do, “automating the nagging” via our WorkerB product is usually more than enough to get the ball rolling and keep it rolling. Notifications and tracking of any reviews that do happen to languish keep things moving as well. LinearB customers have seen drastic improvements in code pipeline productivity as a result of this so-called “nagging”.

  4. Maybe instead of trying to work a bit more together, we could work together.” Well sure, but if you do that, checking in code without a process of pull requests and code reviews, well, then you aren’t getting all the benefits listed above, nor those of a metrics tool that can show you what your Cycle Time is doing. And I don’t believe that mob programming will prevent the cultural problems that can arise from code reviews. People will be people whether in a mob programming environment or in an asynchronous code review process.

Bottom Line

Okay — so what rubber is hitting the road here?

If pull requests and code reviews are hard and people don’t want to do them, then you are doing them wrong. So the trick is to make them easy to do.

We here at LinearB see many, many customers improve their Cycle Time and their overall software development process by using and tracking pull requests. By combining metrics tracking around pull requests with tools like WorkerB, many, many development organizations have seen smaller pull requests, better reviews, shorter Cycle Times, and an overall sense that things are really humming.

Monitoring things like the size of pull requests, when pull requests are assigned, picked up, and commented on, as well as monitoring the depth of reviews that take place all create an environment of small, discrete, easy to review pull requests.

And of course, if you want to find out more about what our customers already know, you can book a free demo of LinearB .

In the end, while her ideas are intriguing and thought-provoking, I can’t say I agree with Jessica’s argument. There doesn’t seem to be any good reason not to do pull requests with code reviews.

Jessica’s blog post can be read on her Jessitron blog and you can follow her on Twitter at @jessitron

If you haven't already heard, Dev Interrupted is hosting INTERACT: The interactive, community-driven, digital conference that takes place September 30th. Designed by engineering leaders, for engineering leaders, INTERACT will feature 10 speakers, 100s of engineers and engineering leaders, and is totally free.

Register Now


If you haven’t already joined the best developer discord out there, WYD?

Look, I know we talk about it a lot but we love our developer discord community. With over 1500 members, the Dev Interrupted Discord Community is the best place for Engineering Leaders to engage in daily conversation. No salespeople allowed. Join the community >>

Originally published at https://devinterrupted.com on June 30, 2021.

Oldest comments (8)

Collapse
 
conorbronsdon profile image
Conor Bronsdon

I really like the bit about mob programming and company culture. I agree, if reviews are causing big problems on a team - there's probably something else going on there...

Collapse
 
nobilitypnw profile image
NOBILITYPNW

The point about it acting as a sort of historical repository is interesting and reminds me a bit of writing laws. Years later, when updating/amending laws sometimes you really want to know why in the world someone wrote something the way they did. Like, "Who tied this to this other thing in a different agency?" or "Why is this set to expire in 3 years if there's no budget constraints?" Basically, context is really nice to have when looking back on work performed.

Collapse
 
andreidascalu profile image
Andrei Dascalu

Nitpick: pull request is a Github concept when you ask a repo owner to pull your code from your repo into theirs. In most workflows we talk about merge requests

Collapse
 
grahamthedev profile image
GrahamTheDev

Quick heads up, your cover image has the word "shit" in it.

I got pulled up on having a cover image with swearing where I forgot to censor a swear word (OK, 3) so I thought I would let you know (to be fair, my swears were far worse!)

Great article, I think the answer is balance (Thanos was right!), mob programming is good for getting things done quickly and picking up issues early, pull (merge) requests and code reviews are good for long term maintenance and remote working etc.

Collapse
 
nickhodges profile image
Nick Hodges

Thanks for the kinds words and for the heads up - I appreciate it! I completely missed that when editing!

Collapse
 
elmuerte profile image
Michiel Hendriks

I'm a big fan of continuous integration. And therefor I quite dislike a Merge/Pull Request process that basically puts people's work in a jail.
MR/PRs (or pre-integration reviews) are often used as a faux gatekeeper to quality, but I think their net effect is zero. Many bugs slip past those reviews, and quickly fixing issues which slipped past is not possible as you have to wait again on those reviews.
Further more, I have yet to see a MR/PR UI which isn't terrible. They focus on changed lines and actively hide context. The presentation gives a really confusing look at what the effective code will be.
I'm trying to optimize our pre-integration review process as much as possible, having tooling there to review the code first.
I hope to eventually come to a place where we can adopt a rule that allows people to self-merge changes if tooling approves.

Collapse
 
linearb_inc profile image
LinearB

Michiel --

You make a good point -- We at LinearB can definitely give this a look and see if we can do something about a better UI.

What suggestions do you have? What context is being hidden -- the code around the changes? How would you improve things?

Nick

Collapse
 
elmuerte profile image
Michiel Hendriks

The problem with diffs is that they focus on changes lines, not changed behavior. As far as I know there are no diff tools around which understand the language and can therefor provide better contextual information. So, it's up to the "viewer" of the diff. The viewer is in most cases a website that just pretty prints the diff. Highlights changed lines and line segments, and adds syntax coloring. Changes can either be presented side by side or unified. Both have major issues:

Side-by-side/split:

  • serious line wrapping

Unified:

  • block/control-flow changes are obscured

Both have the problem that they do not understand the language so changed cross method borders. They do not indicate that a method was removed and a new method was introduced. Instead it makes it look like a method was changed/replaced. Especially when the line(s) before the method have identical content.

So structural representation of the diff is line if it was a flat text file, rather than structured source code.

Then there is the issue that you can spelunk the code. You can not open the implementation of a method that is now being called, or is no longer being called.

For example:

  function fooBar() {
-   initQuux(true);
+   initQuux(false);
    calculateQuux();
Enter fullscreen mode Exit fullscreen mode

initQuux() was change... but what is the new behavior?

Obviously you can check out the branch. But then you loose the MR/PR context. Sure various IDEs can highlight code based on commits. But a MR/PR can be a collection of commits.