How do you do a code review and provide feedback in a non threatening way? How do you receive code review feedback? What is the point of code reviews? Listen to Kris, Toran, and Brandon cover these questions in how to give and receive code reviews to level up your team.
References
Do you have feedback about the show? Want to hear more about a specific topic or say hello? Write us at [email protected] or follow us on Twitter @developfatigue
Download MP3 (28 MB)Transcript
Toran Billups - 00:04 - You're listening to Developing Fatigue. I'm Toran. Today, I'll be joined by my two cohosts, Kris Van Houten and Brandon Williams. Today on the show, we're going to talk about the human side of code review. And to start this off, we're going to have Brandon share a little story about a time he gave someone else a critical code review. So Brandon, what's up?
Brandon Williams - 00:25 - Yeah. So there was this team that I was on and we were building a platform and part of that platform was the authorization portion of that. And on this team we did 100 percent pairing. So there was two people already that had been neck deep in this problem, had been thinking about how do I do authorization and do it right. And making sure that when somebody comes in they only have access to the pieces of data that they should have access to. And they emerged quite a bit later with this beautiful piece of code. And it was, it was complete. They had awesome test for it. And I started going through it and, um, there was a little bit of a urgency. It felt like like, hey, we spent a lot of time on this. There was two of us. We're really smart. We, we did our due diligence and I was like, I understand. Like I, I just want to make sure, like this is really important to our application. I just want to go through all to read the tests.
Brandon Williams - 01:26 - I want to understand what it is that you guys were doing. So when I have to support it, I can, I can do so with confidence. So as I started reading through the tests, I started seeing something, um, and this was really hard. It was really challenging for me to say, hey guys, I think I actually found something that's not quite right here. So in this scenario I think this is what I see in what I did is I actually ended up pairing with one of the guys. I was like, I need some help writing this test. So we wrote a test and actually proved that there was a small bug in there that would allow somebody else to gather some data.
Toran Billups - 01:58 - So cool.
Brandon Williams - 01:58 - We, we wrote that test together and you know, I spent a good hour, hour and a half, like really understanding what tests they did have, what the code was doing and it was actually really challenges. Like I feel like I should just just blink it approved this, just kind of rubber stamp get it through, but I also had this desire that, you know, they spent a lot of time on it. They were really close to it and somebody else needed to be objective and make sure that we did dot all our i's and cross all our t's on that. So that was, that was really tough, but I'm glad I stuck with it because at the end of the day they were both very thankful that I've found that and thankful that I took the time, even though it felt like at the time they're just like, hey man, come on. We, we spend a lot of time on us. You trust us. Which I absolutely did. I did trust them, but making sure that we did our due diligence for the, for the platform.
Toran Billups - 02:49 - As you were sharing that story just now, I couldn't help but draw a parallel to my experiences as a father the last few years specifically you're mentioning of the stamp saying, I trust the people that worked on this. They're good folks and I'm going to just stamp this. I've got other things to do because the actual review process, the amount of effort you put in was non trivial. In fact, you found a real issue as a result. A lot of times in parenting it's the same way where my son will say, you know, one of his friends, you know, so and so can play video games all day. This isn't fair. When as a father I know the easy thing, maybe the cool thing to do, it'd be like, Oh yeah, we'll just play video games as long as we want, but as a good father I know there are limits and he may not understand his limitation, so I have to say no in those situations, but it's not easy enough. Parenting. What about you, Kris? I'm sure you've had a code review that either you gave or received. Tell us about that.
Kris Van Houten - 03:47 - Yeah, so I think the first thing that comes to my mind, but I think about this topic is so back when I was working at my first job, like in tech, I had as a part of this team, we were working on this white label software that was basically like online ordering software for a restaurant chains and as a team of of three dudes and we'd all just work on this product together and up until this point, whenever we had a PR that we're trying to merge in, if we made comments, it was because something needed to get changed or something needed. There was questions that needed to get answered and so on, so forth. And so every time at the time, like whenever I got a comment on PR is I get an email saying, hey, so and so added a comment. So and so added a comment and I remember one time it had this really large PR that had to get merged in as for a fairly large risk factor that I had done. And all afternoon as a skidding those notifications, notifications, notifications.
Kris Van Houten - 04:46 - And I. We all worked in one of those crummy, like open office space environments where everybody can see everybody else and I, I, I was at everybody at my teams on a standing desk and they could see me getting frustrated. Like, dude, like what's up? I'm like, I just, I see you making comments on my PR. They're like, have you read them yet? I'm like, no, I don't want to. Like I don't feel like counting. All mine were criticized right now. I need to get stuff done. And so the day goes on and finally at the end of the day I'm like, I need to take some time to address these comments are figuring out what the heck they're so mad about and I go into this PR and see that basically all the comments that I was getting notifications on and we're basically like one ups saying, Hey, like this is awesome. Hey, good work here. Like this is super clean. I like how you, how you solve this problem here. And there was like, by the time this PR was done, there was like over 40 comments on it I think. And almost all of them, except for like one or two were like affirmation saying hey, like this is really good, I really liked what you did here.
Kris Van Houten - 05:44 - And it was just like I need to stop being so fearful about the comments that people leave on, on my work because it's usually nothing I had to worry about. It's not like a huge refactor. And, and granted up until that point it was the opposite. But it's just one of those moments where I think we all kind of learned that we need to make sure that we're catching people doing good things kind of like, Toran, you were saying in parenting, it's always easy to pick up on things that, you know, our kids should be doing differently or be doing better, but also being a apt to point out when you catch them doing something good because it builds motivation and enthusiasm to keep doing those things. And so, uh, yeah, that, that's kinda the, the token story that I think of when I think about code reviews and my history with them. So let's see here. Um, I know most of us, I think all of us are frequently on code reviews, you know, for myself, I probably have three to five a day that I had to go through a across multiple teams that I work with.
Kris Van Houten - 06:43 - And so I was curious if you guys have any strategies that you guys try to use to avoid what we'll call it, stress responses or to try to communicate effectively without, uh, making people feel bad for the work that they did or for making them think that like, oh, I suck at what I do. What am I doing at this company? I wasn't sure if you guys have any tactics you guys employ when you're doing code reviews typically?
Brandon Williams - 07:07 - So a lot of times if I have some critical feedback, I'll kind of do what your, your coworkers did and do a compliment sandwich. Say, Hey, this looks really good. Um, here's something that I think we can, um, that I think we should look at, you know, what do you think about this? Have you thought about looking at it this way? And I tend to ask a lot of questions instead of saying, I think this was done poorly, we could do this better. That's kind of almost like an attack. And instead flip it, flip it around and say, start asking questions about it. Like, what do you think about that? Does this, um, does this apply? Is this following the single responsibility pattern or principal, um, and just start asking questions to get them to maybe come to the realization about that or say, Hey, do you know about this thing I'm younger developers may not know about some of the patterns, software patterns that can be applied to your code.
Brandon Williams - 08:03 - And it becomes a teaching opportunity to help teach them what that is and then they can see it for themselves. Oh, I see what you're trying to say that we could do this here in this situation. Um, so those are some tactics that I've used in the past.
Toran Billups - 08:20 - Yeah. One thing I think about the good code reviews or if I want a good code review to be useful is on the team. I worked with him, uh, Brandon on a couple of times. The outlook was more around mentoring and sharing knowledge, sharing through the poll, the poll request process mainly because we were already full time pairing, so it's not like we were trying to do the normal thing where I'll just get another set of eyes on it. It was like, oh, we've already had two people work on this to really great people that we trust. So what is the point? I mean I've actually raised that in those pairing culture is is like we already have a ton of time and money invested in us. Is it really worth asking to more people? Because Brandon was kind of mentioning he, he kind of pull request, reviewed that Solo, but I was certainly in conversations on that team where there was actually two of us doing a PR review that to other people worked on. So literally for people
Toran Billups - 09:15 - And one of the things that are very senior person in there kind of coached me through was, you know, don't look at this as you're trying to find something wrong with it. That's not the goal. Especially in this environment. Use it as an opportunity to be educated on the work that the other teams are doing or the other guys on this team are accomplishing every sprint because you're pretty far away from it if you're not doing that work. So I thought that was kind of a cool thing on the, uh, on the, the pro tip side, you know, one thing people know working with me that I'm pretty particular about his testing and that really just comes down to the fact that code changes. I saw a great tweet the other day that was kind of a metric saying, you know, the life of the code you write today is very limited and I think it was like 20 to 30 percent or something of code that you write today will still be around in five years. The bulk of the code that we write is truly refactored, thrown away, moved all that sorta stuff.
Toran Billups - 10:12 - So the test really for me and the PR, the review process, they act as living documentation to show me how does this code really behave? And so I am, you know, I try and find ways to be gentle about it, but I just try and make it culturally accepted that hey, if we're not writing tests, this is just flat out not going to make it into the code base so that way there's less of a, you know, I don't want somebody to feel like a jerk. So if they just understand the team culture before we ever get into a code review, they'll have the expectation, which I think ultimately is where stress comes in, you know, the, the poor requests and the code reviews that I felt like went poorly for me or went poorly for someone else's. Just the expectation was completely mismatched going into it. So
Kris Van Houten - 10:54 - yeah, I think for myself, I tend to kind of what you were saying Brandon I, I tried to ask lot of questions. I try not to assume that any input I have is the way that it has to be because it's very possible that the person who added me to this particular code review has already thought through the things that I'm objecting to and has an answer for them. And so I think by posing certain things as a question, uh, it's a potential learning experience for you and if it does turn out that, you know, the individual had already thought through this scenario and they have a response, then I save face by not looking like a jerk being like, you need to do it this way. But then I also have opportunity to learn in that same at the same time. And you know, I've had that kind of bite me in the butt a few times in the past, which is why I've really tried to shift to that, a way of doing code reviews because I'd be like, you know, you need to do it this way, this way, this way.
Kris Van Houten - 11:49 - But then the person came back and was like, Hey, no, I thought through that thing there. And that's why I didn't do this this way. I didn't do it this way because of this reason. I didn't do it that way because of this reason. So this is what I ended up on on. And I just kind of look like an idiot because I was being so assertive in my arrogance, but another thing I tried to do is try not to assume that the person's vocabulary that I'm adding a comment to has the same vocabulary, the same glossary that I do. So I try not to use like crazy fancy terms that build up my ego. I want to make sure they understand what I'm trying to say to them. So I try to just make it as simple as possible and if I can whip it up in just a minute or two, provide a code example is what, how I would do this. Uh, you know, because when I'm looking at a code review, if it's, if it's people from my team that are working on the same feature as I am, you know, they're wanting me to do a thorough analysis of, of what they, they wrote.
Kris Van Houten - 12:49 - And so I tried to give them the time to make sure I do just that. But if it's someone from a different team that is added me on a code review, it's typically because they trust my expertise in a particular area. So, uh, and I do this as well with other people at my company where, you know, I have an emphasis towards things like accessibility design and CSS and uh, so I know a lot of people will add me to code reviews for those reasons that I can get feedback as to how do I make this code more accessible. And so I focus a lot more on those things and not just the every single line of code like, oh, I don't like what you did here, I don't like to do what you did here, but just providing input on the things that I know that they trust me, they trust my input on. And so those are a lot of things that, uh, I do to kind of help ease people along the code review process and try not to make them feel like, you know, they suck with the dude because. And honestly, they don't, they're, they're great at what they do.
Brandon Williams - 13:43 - Hey, Kris, something you said earlier, you said you do three to five code reviews a day, so how do you do that without having that take up your whole day? Because I've been in places where three to five code reviews a day would. That would be my whole day right there?
Kris Van Houten - 14:00 - Yeah, no, that's a good question. So a lot of times that's a 75% of the time. The code reviews are relatively small. Uh, you know, we, we try to keep our tickets that we're working on to be as concise as possible. That way we can, uh, have the similar scope as to what each task is trying to get done. And so, you know, it usually a code review will take me five to 10 minutes if that, depending on, again, the size of it. But what I try to do is I try not to do code reviews anytime other than the morning, unless someone does a call out and chat and say, Hey, can you please check us out? I need to get this merged in. So QA can check it out. But if I don't have messages like that in my inbox, I typically try to batch all my code reviews in the morning. That way it opens up more potential time throughout the day to, uh, be heads as much as possible without those interruptions or notification saying, Hey, I still haven't done this code review. I need to get to that at some point. So that's how I tend to, uh, manage all those, those PRs. But I wasn't sure if you guys have other things you guys do
Toran Billups - 15:05 - for visual stuff. One pro tip I had was getting animated gifs of the functionality working, especially for Uis and things like that because it's super easy to, at the end of your feature, you know, bring it up in the browser cap capture with an animated gif tools. Throw it in there. That way you're not just looking at lines of CSS and going, well I hope this cascades. Well, you may not actually know.
Kris Van Houten - 15:26 - That is one thing we've started doing at our company as well. We have like a markdown PR template that we has a number of questions on, like what does this PR address, what was the problem before, and what was the screenshot of what it looked like before, if there is any visual change, and then there's like an after, you know, what did you do to solve this problem? Just like a brief one sentence summary and then if you can attach a screenshot or an animated gif, uh, you know, by all means do that and that's been really useful especially, you know, as I head up a lot of the CSS changes in our feature for data people on my team to see like, okay, this was before, this was the after looks, nothing looks broken. Okay. We, we can merge this confidently. And so yeah, that, that's definitely something that we do at our company that's been a very beneficial and it has gotten a lot of positive feedback from other developers on the team. What about you, Brandon? Do you guys have other, other things that you guys do at your company to try to increase the confidence, uh, as you were making code reviews?
Brandon Williams - 16:27 - So something that we've not done a great job with but we've talked about in the past is, um, you, you said that the code reviews are small enough that you can do them quickly in five to 10 minutes, so making sure that you break up those chunks of work. Say, Hey, I'm just going to do a quick re factor here so that when somebody is looking at this, they can see I'm not really changing the functionality, I'm just refactoring some methods to make it a little bit cleaner and get that through really quick because I can look at that as a reviewer and say, okay, I see they just re factored out this method, change a couple of variable names, everything should be good. Merge that. And then the next, next PR that comes through is actually now the new feature that takes advantage of that factor. So now I'm not having to pick a part that I'm not a code review for are two different things. Okay, I see the risk factor here and in the middle of that there's this feature change. I'm actually had one of the guys on my team do that and so that was really helpful as we as I got both of those done much quicker than if they were all slammed together in a single one.
Toran Billups - 17:30 - Yeah. Related to that too, like very closely what you're saying. If I do have a pr where it is maybe an upgrade, for example, we do amber upgrades and I can't really do some of these things as piecemeal is that like I try to have individual commit messages so you can review those step by step and they're both well named and isolated, so at least you're like, oh look, he was in this sub module of the Mondo Repo and updated this. Okay, cool. Then you can look at those all individually to review them without this big massive diff. So
Kris Van Houten - 18:00 - yeah, something else I do is if I am doing something that might seem a little fishy to somebody or it might take a significant cognitive load from the kind of parse through what all those code is trying to do, I'll typically go through my PR before I even add people to it and just kind of skim through all the changes really fast and added comments explaining what this does, why I did it this way. And you know, I'm obviously always open to feedback on it. But just to reduce that processing time for the people who I'm asking to review my code is I just try to make a little bit easier for them to understand like what this method does, what this tooling here does a. and so it's kinda gives them, especially if they're not familiar with my code or how I write it for them to easily grasp what I'm trying to do in certain places.
Toran Billups - 18:47 - Yeah, I like that because what's lacking a lot of times when someone just reading it, you know, secondhand is all the context. So in that example, Kris, you're able to go through and sort of be a storyteller. Hey, uh, so I didn't want to do this but I tried a and I try to be and then I landed on this really sorry about that or whatever. You know. So,
Brandon Williams - 19:06 - so another question for you guys. Um, have you, when you submit a pull request or ask for a code review on an open source project, how do you go about doing that in a way that's in a way that's helpful for the maintainer? Or how do, how do you feel like when you submit that first PR to an open source project, is there, is there some anxiety as you're submitting that? Like I'm putting some code out there in truly in the public, everybody can see that I committed this and now do you feel some. Is there any weirdness with doing that?
Toran Billups - 19:42 - I think the, the pro tip I have here now as you know, open an issue and kind of outline the basic idea that you're about ready to approach. That way they can sort of say, ah yeah, we don't do that here. You can find out before you spend a week doing that or if they're like, oh, this is awesome, actually, you know, this other person was going to be working on this tomorrow. Maybe you should sync up with that person and then you guys could get this PR together or one of you could do it and not waste the other person's time. So it's kind of a nice way to signal before you dump a bunch of hours into it that hey, this is a problem I'm having. I'd like to fix it. What do you think before I dive in and submit a PR?
Kris Van Houten - 20:19 - Yeah, I can say that that's helpful information from someone who has not been attributed to a whole lot to open source because I'm a little bit intimidated by the process. I've made a few hours to a couple of open source things but wasn't quite sure. Like what is the proper way to go about solving this problem? Or um, you know, requesting these changes get merged in. And so yeah, because I kind of wrestled that same fear of like, oh, there's going to criticize anything I say or anything I write and funding. Funny thing is I have never gotten that feedback from anybody from any of the open source projects I've contributed to. So, you know, I may change it changes to a, a data randomizer plugin that I use on occasion. And the guy was super helpful with everything I was trying to, you know, improve upon his code base. And then on the accessibility project I had made a couple PRs for that and the team was a super helpful and super patient with me trying to get up to speed with how this process works.
Kris Van Houten - 21:17 - And um, and so yeah, that's, that's really helpful information to so. So thank you for shedding some light on. So what about you, Brandon, you got anything, any pro tips for, for open source work or how you go about, uh, opening up PRs for that Rome?
Brandon Williams - 21:34 - Uh, yeah, I think, um, I just did this recently actually on a, on a project and I really made sure I detailed out in the pull request saying, Hey, this is the issue that I kind of like what Toran saying, here's the issue I had, um, it's a literally a one line change, but I just kinda walked through. This was the scenario I had. This is the, this is a place I believe I need to make that change here. Is that very quick change? Is there something else that I'm missing or some other, um, is there a different way that I could approach this so that I don't have this issue and I don't have to make this change? I'm just really helping them understand the context that I'm coming from and not just saying, hey, here's a poll request and that's it. That's not very helpful to the maintainer.
Toran Billups - 22:20 - Yeah, you can always tell. I think the one good example is when somebody does exactly what Brandon says, like a one liner, it's like fixed a bug and then there's just a bunch of code. Like that's very hard as someone very outside of the context you were when you found that bug inside your own code, your own project, you're trying to help them, you know, ultimately you want almost, it's kind of like a mini sales pitch. You're really trying to convince them like, I know this will fix your issue, but it actually fixes our issue, so please help me by merging this. But that's a process you have to put a lot of time into. I think
Kris Van Houten - 22:53 - for the work that I helped out with on the accessibility project, I reached out to the developer, to the developers and maintainers that project on Slack and was just like, Hey, I had this issue I'm trying to, to solve it. Wasn't sure if you guys knew about it or if there's a way around it. And their response was actually like, no, we know about this issue. Um, there wasn't a good habits you made for it yet, but it was something that they knew about. Like, Hey, if you can, if you can hack on, I figured out that'd be awesome. And so I basically got their blessing to go down this line of work and figure it out and was able to make a PR. And again, like I said there, there is super helpful about the whole process. Uh, so yeah, it's, it's, it's not all just jerks and dragons out there wanting to eat you aive.
Toran Billups - 23:35 - Is there anything left to say on this or should we wrap up?
Kris Van Houten - 23:39 - I guess one other thing I wanted to say just before we close out and go onto our, our, our big wins for the week is that, you know, it is very possible that you will make aprr for a project and you will have to deal with a jerk at some point. And I know, you know, even for me, even though I've had nothing but positive experiences making prs for open source projects in the past, uh, I, every time I've done it I've always felt like, am I going to get ripped to shreds here? Am I going to, it's going to get made fun of because I don't know how to javascript or something. And you know it, it is possible that you're going to come across a jerk every once in a while and I've definitely seen them in the comment threads and I just really want to encourage people not to be like I've been in the past and it's something I still wrestle with on occasion is to not, don't just hesitate and basically refuse to offer you up your contributions to open source projects because some of might be a jerk because it's okay to also disagree with that person.
Kris Van Houten - 24:36 - And it's also okay to agree with that person and learn from that experience, but simply refusing to put your foot in the water and get your feet wet is basically you refusing to have a potential growth experience. And so. And I think that's, that's one thing I try to always tell myself as like if I, if I push up this code and I make a PR and they shoot it down, I'm probably going to learn something in that process. Even if my feelings might get a little bit hurt. If they happen to be one of those, you know, mean or rude coders. I'm gonna learn something from it in even that thing I learned is how to defend my position a little bit better and so just want to give that encouragement out to you guys out there who might be a fearful to contribute to the open source world.
Toran Billups - 25:20 - Yeah, soft skills. You'll learn something there. I definitely have given a critical review for an open source project where someone was kind of trying to direct the project and I was trying to, as you've mentioned Kris, be very courteous, be be patient and explain to this person that just was not the direction this project is going and ultimately no matter how well I thought I spoke and a on this PR in this issue that the person ultimately just didn't agree with me and decided to go do their own thing, which is the other side of the coin, right? Sometimes you put in a lot of work and you have to see it from the maintainers perspective. They will be taxed at tasked with having to maintain the code that UPR. So think about that anytime you do work on open source is that you know, you're about ready to patch something in great, but when you're long gone into the next project somewhere else, this person, this maintainer, is now stuck maintaining your code. So do as much as you can to help them out.
Kris Van Houten - 26:18 - Yeah, that's good stuff. So should we move on to big wins for the week? What you got Toran? You got something I'm sure.
Toran Billups - 26:25 - Yeah man. I went to a graph ql workshop last night locally and that was awesome. Uh, this guy Justin who was running the workshop did just an amazing. He put an amazing amount of time into this fun little graph Ql Pokemon example that we all hacked on and I just to thank him and the Dsm Web web geeks folks who put that together. It was really fun. So
Kris Van Houten - 26:47 - awesome. Are you a graph ql convert now or are you, are you, do you have the bug?
Toran Billups - 26:52 - Oh Man, I'm definitely, I've been a fan, you know, but I still want to write the server side of it. This is more client side last night, so I'll talk to you in six months about that.
Kris Van Houten - 26:59 - That sounds good. What about you, Brandon?
Brandon Williams - 27:03 - Uh, yes. This weekend, this past weekend, uh, the family and I went out to a free event, um, archeological days here in Nashville. It was a blast. The kids had a lot of fun digging in the dirt and learning about artifacts and the history of Tennessee and what archaeologists do. Um, so it was just, it was just a really good, um, Saturday we were there for four or five hours. It was, um, it was a beautiful day out, so just couldn't ask for better weather or better experience there. So
Kris Van Houten - 27:36 - that sounds awesome. Uh, I guess for myself, it's going to be this new way of basically taking notes that I've been experimenting with the last couple of weeks and it's called the bullet journal and uh, I'm always trying to find ways to improve my method of how I keep track of information and I have yet to find a note taking app that does what I needed to do in the right way. And so I'm actually like in the process of working on one in my spare time because I just hate all the options out there, but in the meantime I've came across this method of journaling and keeping track of tasks and notes and questions and everything and it's called the bullet journal method so you can check it out at bulletjournal.com and I'm actually thinking of incorporating a lot of how it goes about encouraging you to track information and basically building that into the note taking app that I'm, you know, kind of working on late at night when I had nothing else to do. So yeah, bullet journal, that's my big one.
Toran Billups - 28:34 - Awesome. Well thanks guys for hanging out and talking about the human side of code review. Catch you guys in season two.
Kris Van Houten - 28:40 - What?
Toran Billups - 28:41 - Yeah, yeah, that's right. We're going to wrap up here with our season one a mainly to give ourselves a chance to grow professionally. Also get a break here from the summer and regroup some time probably early next year. So thanks so much guys for being on the panel each week and helping me grow personally and professionally and I hope you'll come back next year so we can talk more tech and more human side of tech. So thanks guys.
Kris Van Houten - 29:05 - Definitely catch you guys on the flip side.
Toran Billups - 29:07 - Yep.
Brandon Williams - 29:07 - Yeah. Thanks guys. Later