Pull Requests: Merging good practices into your project (part 1)

<blockquote>Disclaimer: this article has tons of examples, so I've divided it in 3 parts to make your reading experience more practical. In this first part, I’ll do a quick intro to Pull Requests (PRs) and share some data about it. The <a href="https://www.vinta.com.br/blog/2018/pull-requests-merging-good-practices-your-project-part-2/">second part</a> is entirely focused on Best Practices when it comes to dealing with PRs and the <a href="https://www.vinta.com.br/blog/2018/pull-requests-merging-good-practices-your-project-part-3/">third part</a> brings some tips, tools and real-world examples of how big projects deal with PRs.</blockquote>There’s no shortage of publications about pull requests on the web, so writing yet another one might seem counterproductive. However, that’s exactly why this post is relevant - while there’s a lot of information out there, it ain’t easy to compile everything, filter the duplicated content and have the most important bits of each post laid out in front of you. This blogpost aims to bring you a comprehensive summary of what’s to know about Pull Requests (while, obviously, not trying to be the single source of truth or the ultimate guide, since there’s always new stuff coming out). Now enough chit-chat, let’s dive into the actual content.<h2 id="intro">Intro</h2>Although known by most, Pull Requests are often not dealt with in the most effective way. Believe it or not, there are teams that don’t review code at all! People may assume that a senior developer is experienced enough to not make any mistakes, or that merely changing those 3 lines of code couldn’t possibly do any harm to the system. In these cases, it’s not uncommon to skip the code review in order to cut some time. Unreviewed (or badly reviewed) code can be extremely dangerous, resulting in huge risks and unpredictable behavior.A <a href="https://www.codacy.com/ebooks/guide-to-code-reviews">survey</a> says that, on average, developers spend 45% of their time fixing bugs and technical debt, while they could be developing new features instead. Defining simple guideline files, adopting certain behaviors and setting up repository configurations are steps that can increase manyfold the code review performance (in both time and quality), while also decreasing the need for rework in the future. Using review tools both on server (e.g. Heroku Review Apps) and locally (e.g. linters) can also greatly increase the process’ speed. Creating templates and checklists ensures no step is overlooked or forgotten. The list goes on, but enough spoilers for now.<h2 id="why-review-code">Why review code?</h2>So why review code anyway? Reviewing code is sometimes a boring, time-consuming wasteful task that we’re allowed to overlook if the constraints (time, money or both) require so, right? Wrong. I won’t be a hypocrite and pretend that reviewing code can’t be a bit boring sometimes, but other than that, this previous sentence couldn’t be more wrong. And here are 13 a few reasons why.Let’s begin from the end - skipping code due to constraints. Surely it’s a time-consuming task, just like writing documentation also is. But you know what? A well-reviewed code usually means a well-written code, which tends to require less documentation, since the code itself is already so clear. There we are, already saving time - so much for being wasteful, right? But wait, there’s more!The aforementioned survey, called “The Ultimate Guide to Code Reviews” (quite a bold name, by the way), has asked a bunch of questions to almost 700 developers regarding the development processes where they work. They found out that, on average, these developers spent almost 50% of their time fixing bugs and solving technical debts, as can be seen in the chart below.<figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/12/3a/123ad142-bd70-4558-9b73-6a6c6f34ef61/study_percentage.png" class="kg-image" alt="alt text" title="Percentage of time spent on various tasks"><figcaption>Percentage of time spent on various tasks. Source: <a href="https://www.codacy.com/ebooks/guide-to-code-reviews">"The Ultimate Guide to Code Reviews"</a>.</figcaption></figure>From this chart alone you can already see what I was talking about before: by properly reviewing code, you reduce the chance of letting bugs be deployed to production, which in turn could save you up to 28% of time. If you add “solving technical debt” to the equation by requesting code to be done properly the first time (instead of letting it pass “for the sake of the deadline” and creating a ticket to improve it later), there’s another big chunk of time you free up in your schedule. Take those two actions combined and you gain a lot of time, which could be used to deliver new features faster, speeding up the development process and advancing in the roadmap.The survey also asked the developers what was the change in their process that had the biggest impact regarding code quality, to which the majority answered “Code Reviews” (the detailed percentages of the answers can be seen below). So it’s not just my opinion written in a blogpost here, we’re talking about real-life examples of code review being beneficial to the development process and actually being something developers embrace (instead of looking at it as a chore).<figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/8f/9d/8f9d96f8-8427-43a5-ae3f-9fdf411ab40f/study_change.png" class="kg-image" alt="alt text" title="Changes with most impact"><figcaption>Source: <a href="https://www.codacy.com/ebooks/guide-to-code-reviews">"The Ultimate Guide to Code Reviews"</a>.</figcaption></figure>As a bonus information, the survey also found out that reviewing code both before and after the deployment tends to be less beneficial than just doing it before. The researchers’ theory is that knowing that the code will go through a second pass in the review stage makes the first reviewers overlook certain things because they’ll be caught later, while also makes the second reviewers understand that their task is to simply confirm that everything’s been reviewed, but not actually reviewing the code.Finally, the survey came to a mind-blowing conclusion that “reviewing code before deploying is far more beneficial than not doing code reviews at all”. Who would’ve thought of that, right?!Jokes aside, instead of explaining through theory and beliefs, this survey is extremely useful for demonstrating in practice how reviewing code can be a game-changer in any code development process. But something tells me that, if you’re reading this post, you already knew that, so my apologies for taking so long. Let’s move on!<h2 id="most-common-errors">Most Common Errors</h2>When it comes to code reviews and all things related, there are some common topics that can be observed quite often. Probably the most common one is thinking of code review as a chore, which is something I’ve mentioned in here quite a bit already. Although it can seem like that sometimes, especially if you’re in a hurry, you should try looking at it from a different angle: you’re actually getting paid to learn. Yeah, that’s right. Reviewing code is not just about finding bugs in someone else’s code or trying to spot every mistake the pull requester has made. It’s also (and some people would argue that this is the main benefit) a moment for you to learn more about the code and about how other people approach certain problems. You might learn about a new library you had never seen, about a not-so-common feature that the given programming language offers or even just about the coding style used in the project, so you can submit your own PR’s using that same style.The second most common error is actually sort of a derived behavior from the first one: not reviewing code at all. I won’t go much into details here because it’s been explained extensively already in this post, but just keep in mind the survey’s conclusion: reviewing code before deployment (even if not in the most thorough fashion) is always better than not reviewing code at all. Period.The third most common error also somewhat relates to the previous two, but it can happen in a few different scenarios: skipping code reviews for the sake of the deadline. Sometimes the company or team developing a software has a well-established code reviewing policy, with rules for this and that, terms of conduct and whatnot, but when the deadlines get tight for some reason, the first thing they skip is the code review. It may seem like a reasonable trade-off, delivering the feature 1 or 2 days sooner (maybe 3-4 days, if there’s a weekend in the middle). However, as happy as the customer may get by receiving the feature earlier, no happiness will compensate finding a critical bug in that payment feature when it’s already running in production. Those 2 days in advance where users were already able to use the fast-checkout feature will certainly not pay off for the tens of users who could not complete the checkout process because someone missed a validation and it passed unreviewed.Finally, remember, code review is not just for finding bugs. While you’re actually getting paid to learn (which is awesome, from an employee’s perspective), <a href="https://martinfowler.com/bliki/CodeOwnership.html">you’re also making sure that more people get in contact with that part of the code</a>. Instead of having a single person responsible and aware about a certain feature, you’ll now have 1 or 2 more people who know at least a little bit of how it works (which is awesome from the employer’s perspective as well). By doing this, you’ll be grateful (both as an employer and as an employee) when an issue happens with that feature and the person who developed it is in a sick day leave.And remember, the knowledge sharing goes two ways: both for the person who’s reviewing the code, as he/she can understand the project better and get to know new things, as well as for the person who submitted the PR. The reviewer will likely give him/her tips and suggestions on how to improve the code, how to make it cleaner and more accordingly to the project’s coding style, how to use a well known library for doing something that the requester is struggling to develop with optimal quality, etc."Reviewing code is sharing knowledge"<h2 id="coming-next-">Coming next...</h2>With that said, I'll end this introductory part here. In the <a href="https://www.vinta.com.br/blog/2018/pull-requests-merging-good-practices-your-project-part-2/">next part</a> I'll be presenting tons of best practices to be put into practice during the code review process, along with real-life examples whenever possible. Stay tuned!

Pull Requests: Merging good practices into your project (part 1)

Join the Tech Forward newsletter

Related articles

Zero to hero: User-facing documentation

The Vinta Approach to Software Development: Meet Our Playbook

Agile Feedback Dynamics: how to continuous learn from your team