Latest / DevQuestions with Tim Corey / 270. How To Give And Receive Code Reviews Well

Transcript

270. How To Give And Receive Code Reviews Well · DevQuestions with Tim Corey

• 0:00
  SPEAKER_01: Code reviews are an important part of maintaining a good code base with a team.
• 0:05
  SPEAKER_01: Otherwise, your code will start to drift and the results will be a mess of mismatched formatting, poor code reuse, and differing methods of solving problems with code.
• 0:16
  SPEAKER_01: But what do you do to perform a good code review?
• 0:19
  SPEAKER_01: What's important to review and what should be ignored?
• 0:22
  SPEAKER_01: And how do you give a review in a way that a person will respond to well?
• 0:28
  SPEAKER_01: On the other hand, how do you respond well yourself to a code review?
• 0:33
  SPEAKER_01: That's what we're going to discuss in today's episode of Dev Questions.
• 0:44
  SPEAKER_00: Welcome to the Dev Questions Podcast with Tim Corey.
• 0:48
  SPEAKER_01: Software development is more than just writing code.
• 0:51
  SPEAKER_01: So let's talk about the rest of it.
• 0:52
  SPEAKER_01: Specifically, let's talk about how to do a code review well.
• 0:57
  SPEAKER_01: Let's start by focusing on giving a good code review.
• 1:02
  SPEAKER_01: So, number one, I recommend that you understand the context first.
• 1:07
  SPEAKER_01: This is understanding the problem.
• 1:08
  SPEAKER_01: What's the issue you're trying to solve or that person is trying to solve?
• 1:12
  SPEAKER_01: And then what's the existing code around that issue?
• 1:16
  SPEAKER_01: You see, if if you don't understand the context, you may be making judgment calls based upon the fact that you think something should happen in a certain way, but that's not the problem they're trying to solve.
• 1:26
  SPEAKER_01: Or maybe the existing code is not great, and they're not trying to solve the world's problems.
• 1:32
  SPEAKER_01: They're just trying to solve this particular problem.
• 1:35
  SPEAKER_01: And yes, it'd be great if they could change a whole bunch of other things as well, but that's really outside the scope of what they've been asked to do.
• 1:42
  SPEAKER_01: So understand the context first will give you a better understanding of how to respond to the code you're going to review.
• 1:51
  SPEAKER_01: Number two is get the big picture before going back and addressing issues.
• 1:56
  SPEAKER_01: So I like to read through all the files, see what was changed, and get a big high-level overview of what was done.
• 2:04
  SPEAKER_01: So this is not where you look in and see, oh, you you named that variable wrong.
• 2:09
  SPEAKER_01: You might see that, that kind of thing.
• 2:11
  SPEAKER_01: But what you're really looking for is a better understanding of now you know what the problem was, what's the big idea of how they solved the problem?
• 2:20
  SPEAKER_01: Because with code reviews, you want to start with big picture and move down to small picture.
• 2:26
  SPEAKER_01: You don't want to start with small picture and try and move up.
• 2:29
  SPEAKER_01: All right, so number three, you want to focus on items by priority.
• 2:34
  SPEAKER_01: Now, this is first of all by how you find them, but also how you respond to them.
• 2:40
  SPEAKER_01: So certain things are going to be a high priority, things that they're going to have more focus on them, and things where you're going to be clear that this needs to change.
• 2:50
  SPEAKER_01: For example, number one would be security.
• 2:53
  SPEAKER_01: Um, if you spot an issue where maybe it leaves a vulnerability in the system, maybe they allowed SQL injection to happen or something else, you're going to point that out, and that's going to be a showstopper, something that you won't let get past this stage.
• 3:11
  SPEAKER_01: This is a non-negotiable.
• 3:14
  SPEAKER_01: Another one might be a functionality where something's not working quite right.
• 3:19
  SPEAKER_01: You spot what could be a bug.
• 3:20
  SPEAKER_01: Maybe it's not a security issue, but it's still a bug where you can see where a crash could happen or an exception couldn't might not be caught, or there's an edge case that that you've noticed that the person who submitted the pull request didn't account for.
• 3:37
  SPEAKER_01: So this is where you address, hey, you know what?
• 3:40
  SPEAKER_01: The the bug was this, and you worked on it and solved most of the problem, but you're missing part of it.
• 3:47
  SPEAKER_01: And so this is another kind of a showstopper.
• 3:50
  SPEAKER_01: It's not as big of a deal as a security issue, but you do want to point it out, and you do want to hold up the process until that gets solved.
• 3:59
  SPEAKER_01: Now, this is where you start crossing that line into the smaller issues where they still might need to be addressed, but this is not nearly a showstopper, and it probably shouldn't be as much of a confrontation.
• 4:16
  SPEAKER_01: Not that any of this should be confrontational.
• 4:18
  SPEAKER_01: But um this you know, number three probably a better way.
• 4:23
  SPEAKER_01: So this is where you say, hey, you know what, you know, you've done it this way, but there's a better way of doing that.
• 4:30
  SPEAKER_01: Maybe you wrote 50 lines of code when you could have used this.NET library and wrote one line of code.
• 4:39
  SPEAKER_01: So you kind of reinvented the wheel, or you you wrote something that's more complicated, a whole bunch of if statements instead of one, I don't know, switch statement or whatever the case may be, trying to get too nitpicky there because some of this is preference.
• 4:53
  SPEAKER_01: And you don't want to just say, well, your preferences have to be my preferences.
• 4:58
  SPEAKER_01: You have to follow my preferences in order to get past this code review.
• 5:01
  SPEAKER_01: That's not what you're trying to do here.
• 5:02
  SPEAKER_01: You're looking for, you know, is there a way to make it more maintainable?
• 5:06
  SPEAKER_01: Is there a way to make it uh more readable?
• 5:09
  SPEAKER_01: These are the kind of things that you're you're looking into and trying to identify and communicate.
• 5:15
  SPEAKER_01: The last and least important one is formatting.
• 5:19
  SPEAKER_01: This is where really if you have a good linting system or uh like a code made or something like that where it goes and cleans up a lot of the formatting, you shouldn't even have a discussion around it.
• 5:31
  SPEAKER_01: You should just say, hey, make sure you run code made before you or code cleanup before you um you know you submit, or maybe go run code cleanup and and you know submit those changes.
• 5:42
  SPEAKER_01: It's it's not a big deal, and it really shouldn't be something where you're you're arguing over, well, you called this um you know person model, and it really should be user model.
• 5:55
  SPEAKER_01: That might not be a big deal or a mountain to die on.
• 5:58
  SPEAKER_01: Um so these are the least important things to discuss.
• 6:03
  SPEAKER_01: So you may discuss all of them, but some of them are more like suggestions.
• 6:09
  SPEAKER_01: Others are like this is a hard stop.
• 6:11
  SPEAKER_01: So hard stops would be security issues, major functionality issues or bugs, and the more suggestion areas would be a better way or uh formatting of the code.
• 6:23
  SPEAKER_01: So that's all inside of number three, which is focus on the items in a priority list.
• 6:29
  SPEAKER_01: Um step number four is communicate effectively.
• 6:33
  SPEAKER_01: So communication is important, it's very important.
• 6:38
  SPEAKER_01: In fact, this is all that code reviews are is communication.
• 6:43
  SPEAKER_01: And so how you communicate is really important.
• 6:46
  SPEAKER_01: Because if you communicate poorly, you're going to get a poor result back.
• 6:51
  SPEAKER_01: So whenever possible, be positive.
• 6:54
  SPEAKER_01: Start positive, end positive, point out the positive.
• 6:58
  SPEAKER_01: Make sure that this is something where you're saying, hey, you know, I may be finding issues, but it's the 5% we're just working together to solve.
• 7:09
  SPEAKER_01: It's not the 95%, which is great.
• 7:13
  SPEAKER_01: So make sure that you're building that person up because otherwise what happens is, yeah, you may only be finding the 5% issues, but if you don't talk about the 95%, then what you're doing is just whenever they submit a code for code review, you're just picking apart the problems.
• 7:30
  SPEAKER_01: And that can feel diff feel defeating.
• 7:33
  SPEAKER_01: It can feel, well, bad, and it makes the code review process something that is dreaded instead of looked forward to.
• 7:42
  SPEAKER_01: So make sure that you are positive when possible.
• 7:47
  SPEAKER_01: Um try as much as possible to ask versus tell.
• 7:52
  SPEAKER_01: So when you spot an issue, ask about it.
• 7:56
  SPEAKER_01: So you say, hey, I see you have 30 if statements that are nested inside of each other.
• 8:02
  SPEAKER_01: Um is there a reason why you didn't do something different?
• 8:06
  SPEAKER_01: Um may say, you know, I didn't know any way different to do it.
• 8:10
  SPEAKER_01: Um hey, can you help me with that?
• 8:11
  SPEAKER_01: That'd be great.
• 8:12
  SPEAKER_01: You know, but you're asking instead of saying, I see 30 nested ifs, that's bad, don't do that.
• 8:18
  SPEAKER_01: Um that's telling.
• 8:20
  SPEAKER_01: And that can have a different reaction from the person being reviewed.
• 8:25
  SPEAKER_01: Um so be specific.
• 8:28
  SPEAKER_01: When you are identifying issues, don't say things like um, your formatting is bad.
• 8:35
  SPEAKER_01: Well, that's not specific.
• 8:37
  SPEAKER_01: That's very general.
• 8:38
  SPEAKER_01: It's also one of those suggestion things instead of the imperatives.
• 8:41
  SPEAKER_01: Um but you know, or it's you know, I found a security bug.
• 8:45
  SPEAKER_01: Well, great.
• 8:46
  SPEAKER_01: Tell me.
• 8:47
  SPEAKER_01: Um, you know, or this method has a security issue.
• 8:51
  SPEAKER_01: What's the issue?
• 8:52
  SPEAKER_01: Tell me, be specific.
• 8:54
  SPEAKER_01: So say, hey, with this text box right here, you're putting it right into a SQL statement.
• 9:00
  SPEAKER_01: That will allow for SQL injection, which is a bad thing.
• 9:03
  SPEAKER_01: And so you will make sure that you clean, clean that text before you put it into a SQL statement.
• 9:10
  SPEAKER_01: You want to make sure that you are being very specific on what the issue is that you're spotting and why it's a problem.
• 9:18
  SPEAKER_01: Be concise.
• 9:19
  SPEAKER_01: So no one wants to hear a, you know, get a book back when they submit a 30-page uh pull request, or I'm sorry, 30-line pull request.
• 9:28
  SPEAKER_01: Um, you know, get 30 pages back for a 30-line pull request is frustrating.
• 9:33
  SPEAKER_01: And it's one of those things where it's overwhelming and no one wants to read that.
• 9:37
  SPEAKER_01: So try to be concise.
• 9:39
  SPEAKER_01: Um, I know that's hard, but you're also trying to be specific, but being concise is important.
• 9:44
  SPEAKER_01: And finally, this is kind of the overarching idea behind communicating effectively and really the whole code review process entirely.
• 9:55
  SPEAKER_01: And that is work to collaborate, not confront.
• 10:00
  SPEAKER_01: Your job is not to confront your fellow developer.
• 10:04
  SPEAKER_01: Your job is to work with them, to collaborate.
• 10:09
  SPEAKER_01: That's the whole thing, is you're working together.
• 10:12
  SPEAKER_01: And if you forget that, you start to put yourself above them and start to demand from them and start to be miserable towards them and start to be confrontational, you're going to hurt, damage, and even destroy the code review process and your relationship.
• 10:29
  SPEAKER_01: That's not going to be helpful for the team.
• 10:31
  SPEAKER_01: And so, really, at that point, you're the problem.
• 10:35
  SPEAKER_01: So, if we're doing a code review, we should do a review right back of your ability to do a code review because it's not a good code review when you're confrontational.
• 10:45
  SPEAKER_01: So, that's how to communicate effectively.
• 10:48
  SPEAKER_01: So, those are the kind of the four things to think about.
• 10:51
  SPEAKER_01: Um, understand the context first, get the big picture before addressing issues, focus on the items by priority, and then communicate effectively.
• 10:59
  SPEAKER_01: Now, let's kind of switch gears and look at what to do when receiving a code review.
• 11:05
  SPEAKER_01: Because when you receive a code review, especially if you're not in charge or if they are in charge, you might not have as much agency or as much power.
• 11:16
  SPEAKER_01: You might be able to turn it into a collaboration.
• 11:19
  SPEAKER_01: It might be a confrontation.
• 11:21
  SPEAKER_01: You have to figure out how to work these things through.
• 11:24
  SPEAKER_01: So let's go through what to do when you're on the receiving end.
• 11:28
  SPEAKER_01: Number one, remember the point is better collaboration as a team.
• 11:33
  SPEAKER_01: Even if that person's confrontational, even if that person points out formatting issues as a high priority, even if that person is only negative towards your code.
• 11:45
  SPEAKER_01: Remember this is about better collaboration as a team.
• 11:48
  SPEAKER_01: Think of it that way and see how you can work together.
• 11:52
  SPEAKER_01: Number two, detach your ego from the process.
• 11:57
  SPEAKER_01: So this is hard, but you are not your code.
• 12:02
  SPEAKER_01: Your code is not you.
• 12:03
  SPEAKER_01: When your code gets reviewed, it's not you, it's your code.
• 12:08
  SPEAKER_01: Now, you need to expect feedback, you need to expect suggestions for change.
• 12:14
  SPEAKER_01: So if you put yourself in that mindset and say, I'm looking for changes, I'm looking for what I can improve on.
• 12:24
  SPEAKER_01: If you're saying, hey, how can I be better?
• 12:27
  SPEAKER_01: that's going to make for better feedback from the other person because they they can recognize that and they can start to see, hey, this person actually is looking for positive, you know, feedback that they can work on, and I'm going to help them with that.
• 12:42
  SPEAKER_01: Um, it'll start to improve that better collaboration.
• 12:46
  SPEAKER_01: And again, it starts with you.
• 12:47
  SPEAKER_01: It starts with separating your ego out and being expectant of feedback and not feeling attacked or feeling hurt because you got feedback.
• 12:58
  SPEAKER_01: Uh, number three, review your own code first.
• 13:02
  SPEAKER_01: This is kind of a pet peeve of mine.
• 13:04
  SPEAKER_01: Um, this comes with a number of different things, asking for help or um or asking for a person to review your code and then, or even just submitting code for a code review, is that I go through it and go, wait, didn't you did you run the the you know the code cleanup first?
• 13:22
  SPEAKER_01: Oh no, I forgot.
• 13:24
  SPEAKER_01: That's kind of on you.
• 13:25
  SPEAKER_01: That's your first step to do.
• 13:27
  SPEAKER_01: Did you look over your code before you told me to do it?
• 13:31
  SPEAKER_01: Or am I your first step?
• 13:33
  SPEAKER_01: Because I don't want to be your first step.
• 13:35
  SPEAKER_01: So I want you to review your own code first.
• 13:40
  SPEAKER_01: Also, number four, submit small changes.
• 13:43
  SPEAKER_01: If you submit a massive pull request that has thousands of lines of code, the odds of you getting a good code review plummet to the point where you probably won't.
• 13:56
  SPEAKER_01: You'll probably get that real quick, looks good to me, sign off.
• 14:00
  SPEAKER_01: And that may feel good, but it really isn't.
• 14:03
  SPEAKER_01: That's not working together as a team, that's not being collaborative, that's trying to avoid the problem, and also writing bad code really, because you're just setting out big clumps that um aren't being properly reviewed.
• 14:17
  SPEAKER_01: So submit small changes rather than massive pull requests.
• 14:21
  SPEAKER_01: Number five, seek to understand feedback first.
• 14:26
  SPEAKER_01: This means asking clarifying questions, taking time to get past the ah feeling of, you know, I don't like this, to get to the point where you're saying, you know, okay, maybe I don't like how they said it, but let's understand what they're saying and understand what I can learn from this.
• 14:44
  SPEAKER_01: Understand the feedback first before you start defending, before you jump in and say, yeah, but understand it first.
• 14:52
  SPEAKER_01: Ask those clarifying questions.
• 14:54
  SPEAKER_01: Number six, choose your battles.
• 14:57
  SPEAKER_01: So no way is perfect, but consistency is better than any one way.
• 15:05
  SPEAKER_01: So, what I mean by that is you may have a certain way you write your code, somebody else might have a different way of writing their code, and when you come into a team environment, you're going to have to compromise on how you write your code because there may be naming conventions you don't agree with, or there may be layout ideas that you don't think are the best idea, or there may be ways of writing code that you think could be done better, but consistency is better than any one way.
• 15:35
  SPEAKER_01: You want to be consistent in your code base.
• 15:38
  SPEAKER_01: So when your coworker says, you know what, we use Hungarian notation for our variables, that's hard.
• 15:45
  SPEAKER_01: We don't like Hungarian notation.
• 15:47
  SPEAKER_01: And it's not the way that anybody recommends anymore, practically.
• 15:50
  SPEAKER_01: Microsoft used to, and now they're like, no, don't do that.
• 15:53
  SPEAKER_01: But if your company does, then you need to be consistent.
• 15:58
  SPEAKER_01: So do that.
• 15:59
  SPEAKER_01: Um, so choose your battles.
• 16:01
  SPEAKER_01: Don't battle over everything.
• 16:03
  SPEAKER_01: And sometimes people will suggest stupid little things.
• 16:06
  SPEAKER_01: They'll be like, you know what, I want to have two line returns before the end of the method or after the end of the method.
• 16:14
  SPEAKER_01: Okay, that's kind of like pointless, but sure.
• 16:18
  SPEAKER_01: You know what?
• 16:18
  SPEAKER_01: That's not a battle you need to fight.
• 16:21
  SPEAKER_01: That's not a discussion you need to have.
• 16:23
  SPEAKER_01: Just go, sure, and put the second line in.
• 16:26
  SPEAKER_01: Not a big deal.
• 16:27
  SPEAKER_01: So choose your battles, choose the things that are important versus the things that are just nitpicky.
• 16:33
  SPEAKER_01: Don't focus in the nitpicky things.
• 16:35
  SPEAKER_01: Just do what is consistent with everybody else.
• 16:41
  SPEAKER_01: Number seven, be appreciative.
• 16:44
  SPEAKER_01: Yes, it's hard sometimes, especially when a person tears your work apart and says, there's so many problems, here's all the problems.
• 16:51
  SPEAKER_01: They don't give you any positives, they you know kind of focus on the nitpicky things, et cetera.
• 16:57
  SPEAKER_01: But be appreciative because you're working to collaborate.
• 17:02
  SPEAKER_01: You're working to build a better relationship.
• 17:05
  SPEAKER_01: And so by being a bigger person sometimes, and by saying, you know what, thank you for your help.
• 17:13
  SPEAKER_01: Um, I appreciate your help.
• 17:15
  SPEAKER_01: That can go a long way.
• 17:18
  SPEAKER_01: And then with that, number eight, fix the issues and learn for next time.
• 17:23
  SPEAKER_01: If you clean things up and the next time you have less of those issues or even none of those issues, because you've learned and changed, even though you don't love to do it that way, you figure out how to do it the way that that your reviewer wants or that your team wants.
• 17:39
  SPEAKER_01: Well, the reviewer who reviews your next bit of code will go, wow, they learned.
• 17:46
  SPEAKER_01: What I said had an impact.
• 17:47
  SPEAKER_01: They'll feel better and they'll they'll look more highly on you because you are conforming to the team.
• 17:54
  SPEAKER_01: You are being a team player.
• 17:57
  SPEAKER_01: So all of a sudden, you're having better collaboration as a team.
• 18:03
  SPEAKER_01: So this is all about working together with people.
• 18:06
  SPEAKER_01: It's not about being your own solo island who does things a certain way and everybody else has to just give in.
• 18:13
  SPEAKER_01: This is about working together as a team.
• 18:16
  SPEAKER_01: That includes compromise.
• 18:18
  SPEAKER_01: So by doing these things, you're gonna have a better time being reviewed and you're going to have better results out of that review.
• 18:27
  SPEAKER_01: So working together with others, it's an important skill that includes knowing when to compromise and how to take feedback well.
• 18:35
  SPEAKER_01: So thanks for listening.
• 18:36
  SPEAKER_01: And as always, I am Tim Corey.
• 18:39
  SPEAKER_00: Thank you for joining us for this episode of Dev Questions.
• 18:42
  SPEAKER_00: When you're ready to learn to think and code like a professional developer, head over to IamtimCorey.com and enroll in a course.