What exactly does it mean to collaborate as an engineer with other engineers? Certainly there are times when we are talking about our code and there are times when we are pair coding. But in today's episode I want to talk about another process that doesn't get as much attention as it deserves. The pull request process. My name is Jonathan Cutrell and you are listening to Developer Tea and my goal in the show is to help driven developers like you find clarity, perspective and purpose in their careers. The pull requests are a huge part of almost any engineer's job. If you don't know what a pull request is, you probably will soon. But essentially a pull request is a request to change the code base in some way. This started back at the very beginning of the history of Git, the version control system Git. A pull request was literally a request that you would send to someone, maybe in an email, to pull your code, to pull a branch that you had pushed up to the Git server and review it on their machine. Now this is evolved and basically every modern version control system, you have tools to facilitate this PR process. For example on GitHub, you can add a comment to a specific line of code. You can also add suggestions and even accept those suggestions directly into your branch. So what makes a good pull request process is an important question because this is essentially how code gets changed. In a team of any sufficient size, you're going to have code that you write in your own branch of the code base and then you're going to submit some kind of pull request. This is essentially showing the difference between your code and what was previously there. Now there's two sides to this equation and perhaps more, but at least two. One is the submitter and the other one is the reviewer. In today's episode, we're going to be talking about the reviewer side and there's a good reason that we're putting this first, you are going to review most likely far more code in your career than you will write. This is just a function of the fact that you work with more people than likely just one, right? So for all of those people who have code to review, it's likely that you will end up reviewing more code than you actually write yourself. Now if you were to ask a software engineer what their primary job kind of duty is, they would say writing code and it makes sense, right? But actually we review more than we write. So our primary job duty in some ways can be seen as reviewing code. Now, of course, if you listen to the last episode, we keep on saying the word code over and over and in that episode we talked about kind of how we perceive code and this is even one layer deeper or further away from the code itself. This pull request process is talking about the code. So really everything you're going to hear on today's episode, this is kind of a cheat code, is about communication, communicating in a situation where you are presenting or proposing something and then asking for feedback. That is essentially what a pull request is. So all of the advice that I'm going to give you today about how to be a better pull request reviewer is basically communications advice. It's not just about pull requests. You can apply the same principles that we're going to be talking about in any communication situation. So I'm going to give you four practical ways that you can become a better reviewer. So let's jump straight in. Number one, review on a sliding scale of importance, review on a sliding scale of importance. Let me explain what I mean by this. Too often a review of code gets held up by minor change requests. These are things that sometimes are even considered just about your opinion. You don't really have good data to explain why you would want this particular change and it might be something that is kind of over optimizing or maybe it's just a slightly different way of doing what they're doing. So you're suggesting this in the PR because it seems like the right place to suggest it. Here's a critical error that a lot of people make. They believe that the bar for acceptance or kind of the bar for approving a PR is I have no comments to make. In other words, in order for me to approve this PR, I need to look at it and accept that it is exactly the way that I think it should be in the end. But there's no changes that I would suggest making and I'm ready to merge it in right now as if it were my code. If you consider making the change requests that you have advisory only, in other words, don't block the PR. Don't require those changes to be made before you approve the PR. You leave the power in the hands of the engineer to decide what they will do about it. It's not just your opinion as a reviewer that should be listened to. For example, my personal default rule is to only block. If I know the submitted code will have an immediate or near term negative implication. For example, if there's obviously a missing bracket or something like if there's a very clear bug syntax error or a very clear logic bug that I know about, then I'll block the PR because I know if they merge this, it will have a very immediate negative implication. Or if I know that there is a clear performance issue that's going to immediately or very close to immediately cause a problem that I might block then. But I provide feedback on anything else that seems questionable to me. Even after I approve the PR, in other words, I'll approve the PR and leave those comments just as kind of advisory comments, things that I think we could consider but don't necessarily have to keep us from merging this code as it is. This is kind of like the cliche saying that we hear about improv, the yes and you provide validation about a direction and you provide additional comments and direction beyond that instead of trying to control everything or reduce the developers kind of autonomy by saying that the PR can't go in until they follow your exact rules, you increase autonomy and you provide direction and opinion for what it is. Now to be clear, some of these comments are going to have a little bit more of an engineering manager slant to them. And perhaps even more ownership over the code, you might even be mentoring one of the other developers directly in a technical capacity and the way that these comments come together might be a little bit different. So we're kind of leaving room for that here, understand that that's kind of my bias coming into this. Number two, number two, right? So number one was review on a sliding scale of an importance. Number two, ask open questions, ask open questions, assuming the engineer considered what you are about to bring up already. What is an open question? An open question is one that can't be answered with a yes or no as an example. Open questions, invite discussion and they invite opinion. Another simple example of this is, did you test this? This is a yes or no question and it's likely to lead to a very simplified answer about a potentially nuanced question. You might get much better information if you ask, how did you test this? Or what ways do you see this failing? Now I want to give you a word of caution here because I can hear some developers groaning because they don't want to write for hours in their PRs. So be careful with asking open questions that will lead to long discussions that aren't necessarily relevant directly to this PR, right? It may make sense, for example, if your discussion in the PR is going back and forth for too long, it might make sense to schedule a phone call and talk about it rather than spending a lot of time going back and forth in the PR. About asking these kinds of open questions allows the person to explain what their strategy was and provide some background as to the choices they made. We're going to take a quick sponsor break and then we're going to come back with the other two suggestions I have for becoming a better reviewer. Today's episode is sponsored by Headspin. Headspin for mobile, unifies end to end automated testing, full stack performance monitoring and user experience analytics. Headspin is made for any application whether it's native or on the web, running on any device on any network anywhere in the world. With the patented global device infrastructure that offers real, sem-enabled devices on real Wi-Fi and carrier networks, the 24-7 remote access this totally leaves behind that big drawer full of mobile devices that you've been trying to keep around and instead it's going to replace it with a much more mature solution, including for example AI-powered analysis. This will track your user experience metrics and your KPIs over time. You can have cold and warm starts, errors, crashes, response times. You can track audio and video quality and you can even track biometric responsiveness. This is truly a full end to end automated testing solution. Go and check it out. Headspin.io. Thanks again to Headspin for sponsoring today's episode of Developer Tea. All right, let's talk about number three. Going back number one was review on a sliding scale of importance. Number two is ask open questions and assume that the engineer considered what you are about to bring up already. Number three. Create a shared goal of resolving the PR either by shipping or closing. This is the goal of MEPR. The goal is to come to some resolution. It's not to explain yourself thoroughly. It's not to understand our differences. That's not the point of a PR. The most common mistake I've seen in PR review is the engineer reviewing it does so without knowing the context of that PR. In this scenario, an engineer might accidentally adopt the goal of adding every possible comment that they can to pick apart the PR so that their voice is heard, so that their opinion is well known. This can be extremely frustrating for the original submitting engineer, especially if they're on a tight deadline or if they plan to improve the code in a follow up pull request. Make sure you, as the reviewer, understand as much as you can about the context of the PR before tying it up with a lot of friction in that process. And once you understand the context, then using that context to resolve the PR as quickly as possible. For example, if you see the PR going way off in the wrong direction, it might make more sense to close the PR and open a new one entirely. This concept is to resolve quickly rather than leaving this thing out in the open for an extremely long amount of time. Fourth and final. So going back over the first three, review on a sliding scale of importance, ask open questions assuming the engineer considered what you were about to bring up already, create a shared goal of resolving the PR either by shipping it or closing it. And finally, use factual observations and opinions explicitly, but don't blur those lines. Use factual observations and opinions explicitly, but don't blur those lines. There's no one perfect way to write code that shouldn't come as a surprise. Hopefully, if you're listening to this podcast, you know that this is a highly opinionated highly personal, highly contextual job that we do. It's easy to confuse though, the things that we believe about code and the things that we think are factual. The ways that we prefer to write our code versus some other objective standard, a requirement to write that code that way. And this shows up in PR comments when we say you should do it this way instead. When we say you should do it this way instead, a lot of the time that should carry a lot of weight. Where is the should coming from? If you can back it up with some context and some data, then the should might make a better argument. But a lot of the time the should is actually simply an opinion. And if you call it out as an opinion, then it's much better for the context of a PR. If you say I personally like to do it this way and then explain why you like to do it that way, someone else might say, well, I appreciate that you like to do it that way, but I have a different way of doing it. And here's my reasoning. Once again, don't spend wheels too long in the context of a PR talking about the different ways that you like to do things. If you think it's going to be valuable to that person to consider the way that you might do it, then that's a useful comment to make. But if it's just beating the same dead horse, for example, if that person already knows your opinion and if they still want to do it their own way and you haven't found a common ground, then don't spend time talking about the same thing that you've already talked about. On the flip side, if you're not talking about opinion, but instead if you're talking about some objective measure, for example, using this method rather than this method produces 10 times better performance with this particular benchmark. This is hard data. This is information that you're providing and you're allowing the information to speak for itself. Another type of information that speaks for itself is if you as a company or as an organization, engineering organization, or even as a team, if you have all kind of agreed on or if you've established, whether you agree with it or not, if you've all established a particular coding style and a PR goes outside of that coding style, this is objective information. Even though it may be a collectively held opinion, if everyone on a particular team is complying with that opinion, then it doesn't really make sense for one person not to. And so instead of constantly saying that the team's opinion is x, y, or z, you can probably explicitly state that this code goes outside of the kind of agreed upon guidelines. These are four ways that I think you can become a better reviewer. In the next episode of Developer Tea, I want to talk about how you can submit better PRs. How can you get your PRs reviewed faster? How can you avoid errors and difficult problems as a result of merging your PR prematurely? What are some of the best practices around being the submitter in that process? Thank you so much for listening to today's episode of Developer Tea. Hopefully this was helpful and if it was, or if you know someone who would benefit from listening to this episode, I think a lot of developers would, then I would encourage you to share it with them directly. This is a wonderful way to help Developer Teacontinue growing as a podcast and reaching new developers every day. Thank you so much for listening. Thank you again to Headspin for sponsoring today's episode. Head over to headspin.io to get started today. Today's episode was produced by Sarah Jackson. My name is Jonathan Cutrell and until next time, enjoy your tea.