This week we’re chatting about the importance of a far too often overlooked step in the development process: code review. It’s easy to write off the review process, especially if you don’t work in a team environment – but even then, there are ways to incorporate it into your work. Code review provides an important opportunity to fix, to learn, and to teach. Let us show you how to take advantage of it.
Followup Resources
- 12 Best Code Review Tools for Developers (2021 Edition)
- Avoid Ineffective Code Reviews by Eliminating These 7 Bad Habits
- Best Practices for Code Review
- Bitbucket Cloud’s new code review experience is now available
- Developer code reviews: 4 mistakes to avoid
- Effective Code Review Tips
- How to Make Good Code Reviews Better
- Seven Habits for a More Toxic Code Review Culture
- What is Code Review?
- Your Code Review Checklist: 14 Things to Include
Transcript
The following is a machine-generated transcript of this episode. It will contain errors until it has been reviewed and edited, and we apologize for the difficulty that may cause for screen readers. Do you want to help us speed up our transcribing process? Consider sponsoring an episode.
Hello everybody, this is episode number 85. You know it. I know it. Zorak know it. This week we’re gonna be talking about the code review process for Web developers and how it can make you a better dev. Ladies and gentlemen, Thanks for joining us. I am your host, Michael Fienen.
I’m your other other host, Aaron. How’re you doing, Michael?
I’m doing quite well. I’ve got a beard Trim on. Still, I stopped the length, but I took some of the weight out of the sides to surprise the
beard. Let you trim it.
Well, it it fought. And let me tell you, I may need a new trimmer after that, but I got there. Mm.
I would You should tell us about your plans. You can come and pick us on the twitters and instead giggles and Facebook, but on facebook dot com slash drunken ux and instagram.com/drunkenuxpodcast and come and join our chat on the discord. drunkenux.com/discord. What are you drinking, Michael?
Well, woodchuck, hard cider, canned woodchuck, no less. Not the not by the bottle. So it’s I like it this way because Chuck is less. There’s not quite as much here as a consequence of
I don’t
feel like I’m guzzling bottles of beer, necessarily, but yeah, no, I like it. My wife can drink it. She was gluten free, so this works well for both of us. At that point. As I’ve said before, I don’t really like beer. There are a few that I will have now and then, but generally I don’t like normal beer. So but a woodchuck cider I’m down with and what do you have? Good. Sir,
I’m doing my Shackleton again from a few weeks episodes ago.
Um, you’re hooked, aren’t you?
It’s nice. It’s, uh, I don’t want to say that it’s forgettable, but like it’s when you want.
Yeah,
yeah, it’s very like it’s very neutral, but like in a good and bad way, like it’s a it’s a public sector employee. It’s, you know, it’s just it just it’s there. It does its job, you know?
Well, I bought it. I better drink it.
It doesn’t say to five, and then it goes home. You know, that’s that’s the kind of touch this is,
right. There’s a lot of respect for just those good hard workers, you know? Yeah. Little, little blue collar on your scotch,
right? Yeah, that’s yeah. Even at the price point to hard to go wrong with that. Like, I would definitely buy another bottle when I finished this one. Eventually.
Nice. You got to find some. I have not seen it around here, so let’s see what we got this. Episode 85 we mentioned this is better development through effective code reviews. We’re gonna be talking about the process of looking at your code, looking at other people’s code. Why you should do it. How you should do it.
Um, And what those benefits kind of our if those words don’t mean anything to you, or you’ve heard the words code review, and you’re not entirely sure what it actually means.
Uh, the definition I stole from over there is an article at Smart Bear about code review that we have in the show notes, they say code review or a peer code review is the act of consciously and systematically convening with one’s fellow programmers to check each other’s code for mistakes has been repeatedly shown to accelerate and streamline the process of software development, like few other processes can. Sorry, practices can,
uh, What we’re what we’re talking about tonight is is more like the
process of
reviewing code. It’s kind of like like think about like a double check process, like your quick, like reviewing what you did, making sure you don’t have any like debugging code or like any like you put comments where you need him. He took comments that you don’t need out. You have any, like dead code removed, you know, just reviewing whatever it is before it gets merged into the main mind.
This is all assuming you’re using source control, of course, but you don’t have to be.
It’s peer review in the sense of articles, medical articles When, when medical work gets done and published in a peer reviewed journal, the reason that happens is explicitly so that other people will read that chunk of work and then check it right now that process is, of course, a little more drawn out, so to speak. It’s a little slower and longer because their work takes forever.
But in spirit, it’s still very similar to this. It’s just that we tend to turn out a lot of code very quickly, and so that review process just becomes part of the release cycle, as it were.
It’s especially important when you have, um, but like in a professional development, like as a junior developer, when I would do code reviews with my my more senior team members, they would, um, you know, help me see things that maybe I could have done better. And it helps me once. You can kind of check your ego and you can see it as like this is going to help me. This is a good thing.
I’m not on trial here. This will make my code better. It can be really beneficial. And as as a senior person, now I get to work with juniors. I love it. It’s one of my favorite things. I love to sit down with the junior and review the code with them and then kind of talk them through stuff, and it’s a great opportunity to both teach and to be taught.
So let’s start with, uh, explaining to folks why? Just why Code review is good in general. Um, because one of the things that I think can catch people a little bit off guard is this idea that having somebody having to stop what you’re doing to have somebody else look at it and those people are gonna give you feedback that’s going to force you to go back and change work that you’ve already done like that can feel very antagonistic in nature.
It can feel disruptive, disruptive, counterproductive. And so I think it’s let’s kind of start by emphasizing the good of of this process.
Yeah, going to something with code review is going to feel like it’s gonna bog you down like you’re going to have to move a lot slower. And it’s kind of like it’s gonna feel like it’s, you know, harshing the wind on your sales, and you’re going to not be able to move quite as quickly.
I think the benefit, though, of part of it, is that you no longer have to worry about bad or silly mistakes resulting in like breakage because if you have someone else reviewing and they’re gonna help catch like you Mrs semicolon here or oh, you misspelled this variable name. But there’s, you know, some kind of like a check from someone else to kind of help validate the stuff you’re bringing in. So it’s it should.
Theoretically, it should help reduce maintenance costs overall in the long run, so future you benefits. It’s kind of like having automated tests the heart of that.
So one of the ways I look at this and you know there’s a the concept here that I’m I’m not going to address, like, maybe directly, but I’m gonna try to weave it through some of this obviously code review as a step in your workflow is going to be more ingrained if you work somewhere with multiple people, and that’s part of your team’s process.
If you are a freelance developer, if you are a lone wolf developer, or if you just do it for fun, just because you don’t have yourself surrounded by a team of people doesn’t mean you can’t do code review. You can self review. That’s something we’ll talk about here in a bit, but you can also invite other people.
We’re gonna talk about some of the tools like git hub and whatnot and community coding, and you can invite other people in for just, you know, freelance review. Basically a little harder, certainly. But just because you code by yourself, don’t let that lock you into sort of a myopic view of your work.
Because the reality is there are more than one way more than one way to skin a cat, and it’s going to come down to like. Learning happens best when other people challenge your way of thinking usually. And if you just do things the way you do them, because that’s the only way you ever learned, you’re you’re going to have a design or coding patterns and stuff that they may work. But you may also be using twice the ram.
It may take three times as long as it needs to like there are things that you may not pick up if you aren’t talking to other people about that. So I say I’m not going to address that directly, but I just wanted to say it out loud because it is going to feel like I don’t want people feel like they’re out in the cold. If we’re talking about this and they’re like, Well, I don’t have somebody to come look at my stuff.
Sure, you do ask us. Come jump on our discord and just be like, Hey, guys, you were talking about code review. I’m trying to learn this stuff. Could one of you maybe just look at it and give me some advice? We’ll jump in there. We’ll go look at some stuff for you. As long as you’re not asking us to look at 1000 lines of code,
I regularly go onto the rails subreddit and you know people will post stuff. They’ll be like they’re stuck on something or they want kind of a I sent a sensibility check on something and you know, I’ll be happy to review that and comment on it. Um, I enjoyed that a lot.
So and the reason I say that now, specifically my one of my points about why code reviews good has to do with the fact that a code base itself can be incredibly big. Code base can be big enough that you don’t know every part of it. The people at Microsoft who develop Windows. There’s nobody at that company that knows every part of that code. That’s not a thing, you know.
Certainly there are folks who know a lot of it, but they don’t know every little tiny in and out, because you don’t know every nook and cranny that review process can help with things like stopping you from reinventing the wheel. It may shine light on libraries that you have already installed are already available to you that have been built or a P i s that you didn’t know were there but that solve a problem that you’re trying to solve.
Uh, and so that process of review is basically helping you leverage the community collective brain trust of the code base you’re working on and as a result, helps you learn that code base better.
Yeah, that sounds just going to say is like it’s also benefit for your team, and we’ll talk about that more later to, um, because you’re more of your team is getting exposure. So it helps build the domain knowledge of how your code base is changing.
You don’t need to be an expert in the whole code base Obviously, I just said that you can, right? Just knowing. And you learn things. Especially if you’re a junior Devin or an associate developer. You start to learn things about how not just what code is there, but how is it structured? Oh, how would I have gone and found this thing?
Oh, it turns out we’ve got artifact Aries set up, and we’ve got all this stuff in there that I could go look through, um, or search through documentation or whatnot. You start to learn sort of the broader structure and strategy as well, not just the actual axioms and things like that that come into the code itself. Yes, The other thing I’ll say about this is I think it’s really important and really beneficial to review.
Stuff you’re not familiar with is a lot of code. Review is kind of focused on senior members reviewing, you know, mid and and entry level developers work. But there’s a huge amount of value in entry level people looking at the work that the senior people are doing, you know, because they are going to know more and and be more involved in that, and that’s an opportunity to ask questions and to learn again. It’s a learning process.
Review is all about making you better. Um, but when I say that I say it from the aspect of it makes the reviewer and the review, we both better it when that is a functional, healthy process. It should never be one way. It’s it’s not even two way. I wouldn’t even say it’s between those two. Good review is all about the collective the team and making sure, like you said, spread that domain knowledge out.
Get everybody a little seasoned in different things and at least that way so that we can have conversations about, you know, bringing something new in, or how something, you know, change might affect other things. But people have to have some of that experience to be able to do that in my eyes.
How are we going to do these code reviews, Michael?
Well, the how depends, I think. Right on who you are, where you are, How many people are involved, you know, luckily, there is no one right way to do a code review. Um, before we had fancy tools and like you were saying, you know, in the dark ages when we didn’t have access to something like Git Hub. A common way of doing code review was email like people would literally email code back and forth and use that to monitor threads.
And we’re going to talk about tools that can help you with this. And one of the ones I listed was, Yeah, if you have nothing, you have email and a filter and a template I can give you a system to work with.
I have literally printed code off and then, like what sat down with someone and marked it up with pen. Because sometimes it’s easier to do that, you know?
Yeah,
but it’s a really large file.
So one approach that we see used a lot is and and this is one where right or say like if you’re the lone wolf programmer, self review or a self approval process, where at the very least, you’re kind of taking a moment to sort of step beside yourself a little bit and and spend time with your code, but not as the developer as the person reviewing. Basically, it’s hard. You have to. I think you have to have a good sense of self critique, I think do this. Well,
I am doing this. I’m volunteering for a local organization, um, to some development for them. And I do this because I’m the only Dev on the project. So I create issues for myself, and then when I have time, I sit down and then I just I’ll grab an issue and I’ll work on it. And then I’ll put a pull request and I’ll actually make a pull request and then I’ll look through it.
And basically, the PR allows me to look at the differential like it shows red for removing green for added, and then white were unchanged. Um, and I could kind of check for like, Oh, did I leave anything in by mistake? Or I don’t know that I necessarily apply the same level of rigor you might have if someone else was reviewing it.
But it’s still it’s still useful to me because occasionally I’ll be like, Oh, shit, I left debugging code in there. Or like Oh, I didn’t mean to add this file to this committee. Oops.
The way I think about it, too, is like you’re basically playing devil’s advocate against yourself. Um, if you want to think of it that way. Like your role in that instance is to ask yourself the questions, you know? Okay, I did this. Is there a better way? You know, or I saw somebody else can. You know, I saw somebody else do this a certain way. Can I replicate that instead? And why would I do that? Mhm. I agree with you.
I have a habit of So I mentioned last episode. We’re doing this redesign of the drinking you excite. So I’ve got a repo going, got tickets. I’m making issues for everything as I’m going. And I will frequently sort of think in issues, so to speak as a means of, like, brainstorming with myself. All right, stuff down. And then I won’t worry about it for a while. I will come back to it in a day or two when it’s not fresh anymore.
Because then it’s sort of like I’m not dealing with myself in that conversation I’m dealing with past Michael and past Michael knew less than current Michael does so So I can It makes it. Actually, it’s It makes it easier to give myself a you know, a mental disconnect from that original thought so that I can give myself room to question an idea or a decision and think about all the way. Maybe this isn’t the right way to do this, because accessibility concerns or whatever
I think the extraneous stuff kind of falls away to. Once you’ve had some time to step away from it, I I think another. Another reason that I like. I like how you mentioned thinking and issues when I have to think about something as What is this thing that I’m trying to add and I have to write about it in a text box helps me kind of focus in on what I’m actually trying to accomplish.
And so then when I’m taking the code, it helps me stay in scope because I have a scope to find. And then when I’m when I make my own pull request and I’m reviewing it myself, I can say like, you know, like, am I sticking with my finishing what this issue was supposed to be? And it’s just sort of like compact that I make with myself.
I like having the history of pull requests that I can look back on Because then, later on, if I’m trying to figure out why did I do this? I can look back and see that history and any commentary. It may have made myself, like having like a journal, but with hypertext, Yeah, this kind of leads into the next kind that we enumerated here, which is, um, review, but no approval required. And that’s sort of like what you and I are dealing with the drunk new X theme.
Because, you know, us, you’ve wrote about some issues and then I’ve been kind of grabbing a couple here and there, and then I’ll like throughout the PR, and then I don’t merge it. I don’t self virgin, because I want you to see what I’ve done both kind of as, like, a sensibility check. And also because I want you to know, like this thing now exists in the code base so that when you’re doing stuff, you know about it.
But I’m not like waiting on your approval or anything. I’m just, like, check this out and then you can merge it when you’re ready
and and likewise, you know. Hell, I’m I’m sitting there. Fucking code cowboy, committing the master. So good luck.
I think it gets kind of like who’s cleaning up the messes, right? Like fuck up, master. Like that’s that’s on
me at that point.
You to fix it. Okay,
so talk to me about then. The opposite of that. Like what? What would an approval required process look like?
This is a really common, like git hub is really great about this, But this is really common with any kind of team, or you have, I would say, more than two people, maybe more than three.
We we have three. And this is the process we use.
Yeah. Okay, um, approval required is basically the same as what we just talked about. So you have to create a pull request, but then you’re not allowed to merge it until you have at least one approval. I’ve been on teams where you have to have two approvals. Um, you do get the same benefits as with doing the ours. Just like for any any of the things that they’re doing. Self approval or not. I like this.
Because when the rest of the team has to approve it, the rest of team ostensibly has to look at your code, and then they are now aware that this code exists. So there’s an expansion of, like, communal domain knowledge, which is a great thing. One con, though, is especially with two approvals. You get into situations where you know, you maybe you grabbed some small issues this week.
We have two or three issues that are small, and you need somebody to review them. But everyone else is busy working on their stuff, and that can kind of introduce some lag and latency. So you want to make sure that you have stuff that you can work on when you have that kind of down time. Yeah, I like I like one approval that that is like a nice like both as we roll over.
Yeah, I as a developer and like as the developer of the feature and also someone else on the same team. I like having won approval because it means at least you know that if you did anything that was ridiculous, someone else also agrees that it’s OK.
So
it is
not a foolproof process. Mind you, like no,
no
stuff gets by. Stuff will always be one of the, um, there’s There’s one more, though, that we should hit on first, which has to do with command line merges.
So I I was on a team once, um, and this was intentional, but they didn’t do pull requests. So we did pairing and you prepared programming with someone else and you would work on the future. And then when you both agreed the feature was done, you would, you know, make sure the bill passes.
And then if it passes, then okay, and you merge it, it was predicated on we had an automated test suites and there was an expectation of everyone on the team that you were writing tests to cover any features you add and that your Emmanuel testing stuff as well in the staging environment before emerging it to mainline. So that was kind of like a necessary component for this to be able to work. It felt so like I I always felt uncomfortable with it. It’s kind
of like a review on demand. Almost.
Yeah, Yeah. Um I mean, it was nice to know that someone else built the code with me and, like, it definitely caught a lot of like review type commentary and discussion happening while the code is happening. So it’s not like that didn’t happen. But I don’t know. There’s just something reassuring about having someone look at what you finished and being like, Yeah, this looks copasetic like you. You’ve got all this stuff here.
We’re good. It’s supposedly faster to not do pull requests and require approvals. I don’t know. I I honestly don’t know if we actually saved anytime or gained any time I have had more. I would say more productivity on other teams, like where we did do approvals. I think that the velocity there has a lot to do with how tickets are written and a lot less to do with, um with how they’re reviewed.
Yeah, but that’s I don’t know. I feel like I have some some amount of authority just because I’ve worked on so many teams the last two years. But, um, it’s my opinion, though.
Let’s let’s talk about regardless of the process you use. I think there are some things to kind of look out for and keep in mind, um, both as the reviewer and the review eat, it just kind of be aware of some of these things because I think they factor into almost any paradigm regardless, um, the first one I’ll mention, because I was just about to say it anyway.
Yes, to be mindful of context, blindness and what I mean by that, because I have this problem with git hub a lot. We have certain chunks of our code base for some files Big 600 lines, 1000 lines. Maybe it’s not great. It would be nice if they were more abstract, and we are working on that. But in the meantime, it takes time to break some of those files down.
And what will happen is if you tweak a function and change a couple variable names, they will show you the little chunk of seven or eight lines that you change and they’ll show you three or four lines above and below that. But they won’t necessarily show you anything around it and
click on the little. You have
to expand it if you want to see anything. And there are times not every time. But there are certainly times where that context can be very valuable to understanding why decisions get made to do something a certain way. I changed one line of code, but it’s because the tin lines before that we’re doing a certain thing. You know, Um, and if you don’t see those immediately, the my suggestion for that to kind of avoid that is as the developer as the review e.
When you are making the change, that is like that comment on your code when you make your pull request or when you make that commit. And no Tate that line to say exactly that, so that when the reviewer comes in, they know that, you know Oh, hey, I changed this because if you look at the original function, I need to, like, maintain the signature for this reason,
that’s a really good point you just brought up to, um, even if you do have someone else reviewing your code, the first person to be your coach should always be you, because there’s going to be, you know, fruit that’s like barely hanging on, ready to
fall off to be,
you know, to be noticed. Um, that’s a bad metaphor.
Well, there’s there’s the phrase we easy
stuff, this catch.
Yeah, the way we say it is, leave it better than you found it Yeah. Yeah. And what we’ll bring up here in this section here in just a second? Um well, here, I’ll just jump right to that. How does that sound? Um, the way we say it at work, leave it better than you found it. This is just this idea of maybe change some bears to const. In your JavaScript. You know, instead of having everything be variable, makes the moment,
Constance, because, you know, they never change. It’s just better semantics in your code at that point. And it’s, you know, it makes the code safer. In that instance, um, you know, little things like that rewrite. And if statement as a turn airy just to make it shorter and more readable, like little things like that, that can be done in the context of your work. This should not be confused with, and and I forget which article it was.
But there’s gonna be an article in the show notes that has this. There’s a joke about things to avoid review. Time is not a developers opportunity. Two suggest that the review we fix all the problems like you review time is not the time to say, you know what? I think this whole thing would be better as a react component instead of part of this angular thing like, No, I’m not gonna take. I was fixing a variable name.
I’m not going to take 12 hours to rewrite this whole thing out of that.
I want to say that if it comes, if it comes up, it should be noted somewhere. And it’s fine to say this is a note for later. We should consider doing this. But don’t put the honest on the person who submitted their
code
because you’re just gonna push the goalpost back. And yeah,
and I would argue that if if you need the document unnecessary change like that, then you make an issue. An issue? Yeah, you need to create a ticket for because nobody’s going to go back and look at that review and see that comment and remember. And, um, anyway, so that’s that’s number one, the other one that are the other two. These two kind of go together.
One is that you shouldn’t have to spend a lot of time on reviews like Oh gosh, no, Uh, definitely less than an hour of your work day. Should ever be spent on reviews. Unless you are just a huge software company and that is, and as your QA or something that’s your job, you shouldn’t be committing tons of time. And the way you do that is individual reviews. Nominally should be kept under about 400 lines. That’s based on research that Smart bear did.
Do you mean like the submitted code to be reviewed to be under 400 planes?
Like the thing the review being requested should be under four headlines like In In Total, because beyond that, you start to get a drop off in the number of things caught for the time invested.
Yeah, definitely. And I could see that
I understand that can be hard, especially if you’re working with, like, branches and pull requests. And it may be a lot of individual pieces of work that you’ve done, but like Git, Hub says, Yeah, but we’re making them review the whole PR and the whole PR is 25 files, you know, 14,000 lines of code. I get that, and that’s where it goes back to what I said earlier as the developer comment.
The stuff that’s especially important and if you can try to submit stuff in smaller chunks as you’re going, don’t like. Try to make one big PR out of. There’s no rule that says you have to do that. Make two branches and start your development and one merge it into the other with that one being the one that you will ultimately merge in your staging branch or whatever.
If you’re if you have an issue that has like a to do, as you know, do one one commit for or one commit and pull. Request breach item on the to do list if necessary, like they don’t pull. Requests. Don’t have to. Didn’t be big. They should be focused like the pull request. You should know the same summary. What is being added to the code base, or what is being changed in the code based by this pull request and that scope you’re defining should be
complete
by the pull request.
And there will always be exceptions.
400 lines. Sounds reasonable, though that that’s I’ve been I’ve been I have reviewed pull requests that were much longer than that. I’ve written some that were longer than that, and yeah, there’s like a fatigue you get after a while.
Yeah, I talked about context blindness, but you also get a code block blind that’s trying to stare at all that stuff and read through it and see well, did they miss a bracket that they, you know, name their variables, right? And after a while, you don’t care.
I would say that. I probably spend about no more than five minutes looking at the code itself initially. And then I might spend 5 to 10 minutes total writing any comments?
2 to 3 minutes. That’s about our average code review or is probably 2 to 3 minutes. And one of my big ones is providing the why behind a review? Because, um, because again,
there’s two sides to this to go ahead and say,
But I’m just gonna say it. It comes back to this learning opportunity, right? Like if you just say, do this a different way. But you don’t explain why that person doesn’t understand what was wrong with the way they did it, you know? Was it not appropriate for the code styles you guys use? Was it using the wrong AP? I Was it not, You know, you know formatting the tabs, right? Like, what’s the deal?
So you try to explain why something should be done a certain way. There’s an article over Tech Republic. They say, um, never return null when retrieving a list of records from the database say when returning records from the database and they and there aren’t any. We should return an empty list because all the list methods work on an empty list and we don’t have to introduce error handling code.
Mhm. So instead of saying, Don’t do this, you say we shouldn’t do this because we have things to handle it. And now that person knows in the future that that’s the case
I like. I like the fact that they use kind of the plural nominative case there. They say we, um, and you can also say like us, like, let us do this instead. I think that it’s important because the the act of doing submitting code for review is implicitly like vulnerable, like you’re asking someone else to judge your stuff basically and using inclusive language that you know that sounds collaborative makes it easier to provide critical feedback because,
you know, the tribe is still accepting you and want you to be better than to produce better stuff for the tribe. Like you’re You’re still you’re not being You’re not being ostracized or ousted with criticism. You’re included. You have agency,
you have agency in the solution. And like in this case, if it was something like what this Tech Republic article said, you know, you know, we, uh, shouldn’t return empty list because all the list methods work on an empty list and we don’t have to introduce error handling code like that’s all about the code that has was written before you, probably. But you have ownership over that, too.
Like that’s That’s something that is now yours as much as anything. And you deserve to benefit from that knowledge. And that code
the only time that I would ever use language that was in a more of an imperative imperative since would be if some if something was dangerous, Um, like, uh, if they wrote something that would introduce an SQL injection vulnerability or if it was like, Oh, this is going to break stuff, I might say something like, Don’t do this, or please don’t do this and then explain why.
Yeah, Regardless, the lie should be included. Yes, the Y must
be there. The flip side of this, I was going to say, is when you’re writing your pull request summary to submit for review, you should provide the why there as well about why are you submitting this? What is this pull request doing? What is what are the features or enhancements being introduced by this?
Or what’s the bug being fixed? Um, provide the context first because the person doing the review is going to read that first, and that’s going to kind of frame what they need to look for. Otherwise, it’s just like a bunch of code. And some of it the reviewer probably hasn’t seen in a while, and it can be, like, really disorienting, So the summary is very important. So when you’re submitting something for review, make sure you have a good summary there, too,
and and even to that point just thinking about summaries. It’s also okay to ask for specific feedback, too. Yeah, like, you know, we we’ve talked about this in past episodes and and looking at code and knowing that I wrote code that works. But I would really like for somebody to help me write it better. Like just say, Could could somebody look at these wild loops and and see if there’s a better way?
Because I feel like it’s very heavy and you can draw some extra attention to those because there are a lot of cases and things like that where the reviewer, even if they are, you know, a really talented high level developer they may look at and be like, Yeah, that works. And they may not take the time to show you a better way because they do have stuff to get to.
And so they’re going to be like, You know, it’s not worth explaining at that point. But if you want to learn that, ask
sometimes. If I feel Webley about something I’ve submitted I’ll, I’ll call it out in the summary and I’ll say, like, Hey, like, or I’ll just actually go to the I’ll go to the code review or the Yeah, excuse me, the changed files itself, and I’ll find the line or lines, and I’ll put a comment there myself, and I’ll say I’m not sure about this, but I can’t figure how to do it better like you were just saying and is like, Is this okay? Or like, what’s the best way to do this or whatever?
You know, it’s the more you can provide. I like to prepare the person reviewing your code. The better your review will be. It’s very much a collaborative process.
The the next one. I need you to pay particular attention to for me, Erin. Mhm. I need you to not be a jerk.
Don’t tell me what to do. Um, yeah.
I mean, I don’t need you to help with the site redesign. If that’s gonna be your attitude saying Uh huh.
You recall one time. And if you are
the this goes So this goes every way, right? Don’t be a jerk when you write your PRS and in all these articles will have in the show notes. You know, many of them talk about this in different ways. I sort of summed them up under this, but it’s like, understand your tone, understand that text does not convey tone right and know that the people you work with want to make improvements. You know, they are part of the solution.
They are on the same team with you. Uh, they want to learn why suggestions should be considered, you know, regardless. And if you get that feedback, don’t be a jerk about it. And we’ve talked about this before in other context, this idea of you know, sometimes you need to sit down and be a little introspective and learn how to take criticism. And hopefully that is very constructive, positive criticism.
But even when it is, sometimes some folks do have trouble with that. And it’s not about you. It’s It’s not about making you feel bad. It’s not about tearing your code down. It’s just about Hey, we need to do this a little differently because and as long as that because is there, then they’re probably trying to do their best to help you.
This is where, like, you know, people often talk about how soft skills are as important in tech as technical skills are, and this is one of those places where it really is. And and again, I mentioned earlier about how submitting your code for a view is vulnerable exposure. You’re you’re exposing yourself to this person to to be judged, and that can be difficult, especially when, um If you’re a newer person, maybe you’ve got imposter syndrome.
Talk about a lot A lot on this show. If you feel like you’re a fake and you’re about to show someone the code that you just wrote and you’re like they’re going to find out that I’m shit, you know, like that can be terrifying.
So
as a reviewer, I mean a like I think the Gold Star reviewer will recognize if the person that reviewing is getting defensive and they will disarm that they will say, like, You’re cool, But we’re going to go through this and we’re going to make this better together, and they’re going to include them in the process and help to kind of diffuse the imposter. Alarms are going off for the person being reviewed, but definitely, like, don’t be shitty.
And I mean, that can be really apparent in some of those power dynamics, right? If it’s an associate developer and a senior developer, there’s a power dynamic in play there. And even if you work at a really flat organization, where yeah, you have titles, but really everybody is doing everything that’s some people
are gonna know more than others have been on it longer, but
like and that’s kind of how we operate. I mean, we’re a three man team at work, so it’s not, uh, three people team, Um, but that power dynamic, like I’m the senior Deb. But I had no point want any of my other coworkers to feel like I am trying to pull a lever over them, you know, And I expect them to come at me on the stuff I write because Lord knows, every one of us will cut a corner somewhere once in a while when we’re frustrated or agitated or just tired.
And so I expect you to call out my code to, And that goes both ways and so understanding that is really important. And in some organizations, especially really formally structured, one that’s large, those power dynamics can be a little more powerful.
And those people have to understand that you have to realize that just the fact that you’re chiming in can feel a little punchy, so to speak. So you need to kind of nurture those relationships and and be cognizant of that,
I would say on the flip side of all of that, though is like when you’re like, when you’re reviewing someone’s code, don’t not provide criticism. They call it rubber stamping. Don’t just be like, yeah, Looks great. Good. You know, if you’re right, if all of you right in the poor view, approval is L g T m looks good to me and nothing else. Um, you may want to get another like email. That person may want to get someone else to review it. Um, I just don’t review it at that
point, you know? Yeah, Yeah,
I don’t think they maybe Maybe that’s okay. If it’s a very small like you’re changing a configuration file or something, or I don’t know if I’m reviewing something, I’d like to at least provide, like, some kind of feedback to kind of show that I actually read it either. Like so One thing. It’s really cool to do on a supportive team when you’re doing your reviews.
Um, look for like, clever or cool or, um, just good code that submit er wrote and call it out and say like, Oh, nice work. This is really cool, Um, or just like anything like it’s, you know, you can help really lift your team up that way. Um,
I mentioned keeping recommendations in scope. That’s what I started with. That’s just that idea. Don’t ask them to do things that are way, way out of band for what’s happening. Um, the other part is, and this goes along more with what you were just saying, You know, don’t be rubber stamped, be engaged, right? And that goes both ways. If you are doing code reviews, that is a part of your process.
And as a result, if you’re not engaged with that process, you could be blocking stuff from moving forward. If you aren’t reviewing, then you’re making it harder for everybody else to review what’s there. And if it’s your code, you should be responding. When people leave your reviews, at least tell them if you did it or won’t do it, um, let them know that their review was, you know, picked up on, so to speak. But not every review is going to result in the action.
You know, they may just be saying something to help you know something. We do that all the time, like it’s like, Hey, you know, next time I might do this this way because you know we have some utility classes, but this isn’t like doesn’t rise to the level of going back and unwinding the work. You know it still will work fine and is acceptable. You know,
I would, I would add to this like we we were talking a little earlier about, like, the language to use and like the tone to keep. I sometimes see review commentary as passive Aggressive is a loaded word are voted term and it sounds negative. And that’s not what I’m trying to say. But sometimes a request commentary is presented as a question like, What if we did this instead? But what they really mean is, I want you to do it this way, and I I
Yeah, there’s and I don’t like that and I can’t think of what it is. Yeah, I don’t know what to call it. It’s along the lines of like, a leading question. Basically.
I mean, I think it’s like it’s one thing. If you ask someone like like a legitimate question, like, have you considered doing this or like, what would happen if this happened because you’re actually wanting to have a discussion about it and you actually want them to answer the question. That’s fine. Like I’ve done that. I’ve answered those questions.
It’s useful because sometimes, like, you know, as a reviewer, maybe you’re looking at something and you’re like, Oh, yeah, like I obviously we should be using this thing instead. This tool and maybe the person considered that and there was something problems they ran into and they couldn’t do it. Uh, this polar quester or review is a great place to have that discussion.
What I’m talking about is when it’s like like, Oh, well, what about putting a semicolon at the end of this wine? Or like, how about you include a comment, something like that. What would happen if you broke this out into, like, a function?
Um,
don’t do that. Not because it’s like a shitty thing to do, but because it’s like you’re not communicating your intentions. You want them to change it. Okay, so
but then
say why? You know, like we said earlier, say why you should change it like, Oh, um, could you maybe like, could you please break decided to a function? We think this would be better because of this reason or whatever and then that way the person who’s submitted the code, they then no, that they have an action item to respond to, and they’ll know when they’ve done it, and then they can follow up.
The very last one in this bank is, um this is the hardest one. I think it can be really hard to do this really appropriately, and it depends a lot on your workflow overall. But try, I say, I I say this don’t review at the very end, Really. What I should have said is, Try not to review at the very end, because it goes back to what we started with with this idea of Don’t spend more than an hour doing code reviews.
Try to keep code reviews under 400 lines. You want them to be fast when especially because because what happens is right. If you have a big chunk of work and that review doesn’t come until the very last PR, then that’s where you end up with 247 files. You know, 33,212 lines of code for people look through,
and
it’s just it’s daunting. And so if you can try to do it as you progress. There is another really good reason for this, though, because if you did mess something up or you did do something wrong or out of pattern and it’s not found until after you’ve done a week of work on it. Ungh, baking that cake can be a pain in the ass. And everybody knows that feeling because we’ve all had that happen at one time or another one time
or another, we’ve all done that. I would agree with the classic.
The classic example of this is Oh, hey, these buttons aren’t accessible, right? You didn’t make any of this stuff. There’s none of the keyboard controls Work on any of this page because you didn’t use the right elements.
You should do the code review for each individual ticket within the feature. Each little piece. That’s when the court of you should happen. If you if you wait until the end, I like the in baking the cake thing. That
Yeah, because I mean I mean, like, you put it on the other, the other metaphor, right? Just pulling a thread on a sweater, right? You’ve You’ve got this whole thing put together and you’ve written the a P I s into it and all this stuff and you realize, Oh, I’ve got to re factor this.
And then that snowballs into every other piece of your code, slowly like, yeah, whether it’s in baking a cake or pulling on a thread, you can cause a lot of damage by not catching something like that early enough and cost a week’s worth of development time as a result.
And and I mean more importantly, from a bigger, like a macro scale, if you’re waiting until the epic point, did you review think about all the domain that you’ve denied your co workers access to.
And when I’ve when I’ve been in situations like that, what has happened is we’ll have three different tickets that are related and all three of us all three of the tickets will be solved in slightly different ways, and now we’ve got three different approaches submitted for the same feature. And like that kind of thing can happen if you wait until if you wait until it’s too late to be doing your code review.
Yeah, so it’s close out here just to talk about some of the tools and some of what can help you with this process. Um, the first I want you to go check out code mentor dot io over on their blog. They’ve got a checklist. So if you want to build a code review process for your work or something like that and you don’t really know where to start, they’ve got a check list and you can pick and choose from it.
You know, for what’s useful for you, you don’t have to do everything, but they’ve got things like, you know, check for readability, check for maintain ability, check for security speed and performance documentation.
Make sure you know, does so one thing like I’m pushing on right now at work is whenever JavaScript gets touched and you do something with a function or make a new one, it needs Js docks so that we can, you know, automate the production of documentation for that code now. And if somebody new comes in and looks at that code, they understand what it’s doing and why. So make sure documentation is there.
Check for reinventing the wheel reliability, scalability, reusability patterns, test coverage and test quality. You’ve brought that one up, um, fit for purpose. Notice what’s missing. Zoom out That gets back to this idea of looking at context. So this is a really good list of things. You can go look at the little bit longer article because they go into each of those and why.
But, um, I think that’s a good place to start for one, if you just kind of want to understand, you know, and it depends. How formal do you want that review to be? Every review does not have to have a checklist. You know that’s not necessary. If it did, some reviews would take longer than you know, the work that actually produced them in some of those cases. So don’t feel like you have to do it every time, but it can be a starting point
on Git Hub. You can do a pull request template, and the template has sort of like refilled out headings and things that the person can then fill out when they’re ready to pull Request summary. You can put a checklist in there using markdown and something they do at my current job, and I’ve seen this of their previous jobs to is they’ll have a checklist of. Does my poor request satisfy each of these things like sort of.
The team has sort of agreed These things are important for every pull request to consider. And you just self review you going on each item? Did I do each of these things? And once you’ve done all those check west items, then that’s the point when you have someone else for the recode.
Um, the next thing this gets into a couple of different kinds of tools a little bit, but make sure you’re using winters and a static analysis tool and something like a like, prettier NVs code.
Uh, I like I love and hate winters. I like
their opinions.
They’re opinionated, their pain in the ass, but like,
But they also they’re a pain in the ass because they do the things so to make the
code look consistent. And that’s why I like them. I like that about it. Because you have you never have to worry about whitespace weirdness or, um, you know, stylistic issues about spacing,
like you were saying, right? Oh, a missing semi colon or a missing bracket or something like No, that no longer is a thing like those. And what some of the articles will talk about is anything you can automate. You should. You shouldn’t be relying on somebody else’s eyes to notice that you missed a semi colon in your code. Especially when javascript now doesn’t care. Like
right, Right. And a lot of them can even do auto correct, like ice Robocop a lot. And Robocop, you just do Robocop dash a and it will run through all of your files according to its very detailed syntax lending. And then if it can, it can safely do it. It will fix the lines for you,
and you can take If you use V s code and you have the prettier plug in installed along with 11 million other people, you should have it if you don’t, um, you can set that when you save a file to run prettier on your code just automatically, and just that’s the way it works. So you never have to think about it then.
So the only thing I will say about this is auto correct is fine and even automating having run it is fine. But always always check what is being changed before you commit it? I had a coworker who had rubber copying auto run and committing like we just auto running, commit after you save a file. And it ended up introducing a Bugatti production because of just as of a weird thing. But it introduced a bug.
His production unintentionally, Um, and it was overlooked in the code review. I have personally had methods that looked similar to other things, and Robocop says, like, Oh, you did this. But really, you should have done it this way. It’s like, Well, no, actually, that’s totally different when I’m trying to do here.
So just commit to saving, commit your code first, then run your winters and do auto correct or whatever and see what changes it’s making. Make sure that everything looks okay and then commit the lynching separate. I just That’s my soapbox
there,
the
other the other tools. I’m gonna just lump these together really, because they’re all the same. Crucible, get hub bit bucket. Um, these are tools. Crucible isn’t at LeSean tool. They’re the folks make the era and confluence and bit bucket, for that matter. But crucible is like a standalone tool, um, that they make really. It’s only gonna be good for enterprise setups, but I include it specifically because of the way it integrates with all of those other tools.
So it’s kind of one of those, like if you’re in the ecosystem, it can be really beneficial because of how how tight in it is. But get hub. We’ve already referred to its free A. You’re probably already doing your code versioning and management and issue tracking and stuff there so that even though the review tools are pretty rudimentary, it’s there. Um, and you can do a lot to kind of beef those up bit bucket has added it recently.
Um, I’ll have a link to the instructions there, but it’s very similar to get Hub at that point, Um, in terms of how they tied it in, um, there are. And for what it’s worth, I know some of you are probably gonna be like, but you didn’t mention this tool or whatever. Yes, there are a lot of code review tools out there. There are a lot of them.
The problem is most of them our apart from everything else, and they tie in the guitar they tie into s V N or they tie into any of these systems, and that’s all well and good. But the reality is, if you’re going to do that, you might as well just use what’s in git Hub. That’s my personal opinion on that, Um, one thing I always worry about is adding yet another tool right to a workflow process.
And it’s like, Yeah, okay, now, you know, now we’ve got this code review tool that’s out there that we have to go log into and check that on top of it. And it’s not in our ticketing system. There’s no tracking. It’s not in git Hub. So I just I say that for that purpose that, yeah, I know. If you wanted to go out there and and grab, grab a review board, grab fabricator, grab code sense. Yes, I know these are all things Garrett road code.
There’s a
million tools for it, and I’m sure many of them are incredibly good. But I tend to feel like the best tool is the one that’s right in front of you. And since most repose now have that option, you’re in pretty good shape.
There was a quote from Linus Torvalds, the guy that the initial creator of Lennox And it’s I guess it’s not called the Venus as well. Given enough eyeballs, all bugs are shallow, and I think that that’s sort of it’s kind of like What’s behind the importance of code review is you have enough people Look at your code and weighing that against, you know, the time lost or having more people look at it. But even if people look at your code and it helps to make sure that there’s like fewer bugs,
this is
referring to like the basis of like, open source contributions, like having enough people working on a project and eventually, like all bugs, become visible when resolved. But I think it applies for code review, too.
I agree, folks, I hope you enjoyed this episode of drunken new X podcast. I hope that you’ve learned something about code review or you’re excited about code review are interested in it. Let us know you know how your experiences go with that or, you know, horror stories. Maybe you’ve had a really bad time with it. I would be interested in hearing that to understand why you know what went wrong in those situations.
And, you know, is there advice we could provide you or, you know, help we could offer? I’m happy to do that. So, um, if you want to share any of those stories or have any questions about that or like I say, do you want to have somebody review some code for you so you can get a feel for it? Let us know we’re on Twitter, Facebook at /drunkenux.
You can get us on instagram.com/drunkenuxpodcast or on our discord at drunkenux.com/discord. I will make Aaron do all the work if you do that. So
I like the encoder views.
If I
know the language, I can help you.
Even if you don’t.
If I don’t, then I can read it. Yeah, and it’s not
how will you ever learn a new language if you don’t sit there and read it? I don’t know how I don’t It wasn’t a joke. I’m just saying, like, you gotta You gotta get in there like I can read java. I’m not gonna write it, but I can read java. Usually I understand it.
I’m gonna I’m gonna put a few qualifiers on here? I don’t expect unless you’re a very, very early academic level person. If you’re writing C plus plus, I probably don’t want to review that. I did that enough years ago. And if you’re doing JavaScript, I’m gonna make Michael review it.
I’ll actually give you. Give one last piece of sort of parting advice here, and that’s a if you are a teacher, have your classes review each other’s work. Don’t grade them entirely on your own. Instill in them like use tools that allow your class to collaborate with each other. So even if it’s public, get HUD repos. That’s good enough.
Get in do code reviews and get them used to that process because it’s going to make them better when they get out of school. And it’s going to get them used to getting that feedback and learning how to respond to that feedback.
You really get like multiplication of returns on that, too, because you know you’re it’s a lot easier to learn something. You’re teaching it to someone else because it to communicate. The idea is you really have to get down to the crux of it. Yeah, and I think you really get that a lot if you have. If you like. Do your reviewing your editing. Yes. That’s a
great idea. I think at the end of the day, it really just drives home this idea that if you want to produce the best code and the best work that through that process, through that good code review, you will inevitably keep your personas close and your users closer. There’s been,
like, five times already. When I was like he’s gonna say it, I’m sure he’s gonna say it.
You’re welcome. Bye bye.