Episode 5: Code Review

Welcome to the Dev Team Show my name is James. 0:00 In this episode, we're gonna talk about code reviews. 0:02 Joining me is Nicholas Hampton. 0:09 Nicholas is a member of Treehouse's technical success team. 0:11 Welcome to the show, Nicholas. 0:15 >> Thanks, James. 0:17 >> So I wanted to start just by kinda giving a basic definition of what a code 0:19 review is just to make sure that we're all like on the same page. 0:22 >> Of course. 0:25 >> So from my perspective, 0:26 a code review has a component of reading code, of course. 0:28 That's sort of the, one of the review parts. 0:31 But you also need to like, run the code typically too, right. 0:34 So you're executing whatever it is that the code is written- 0:38 >> Yes, you've gotta unpack the project 0:41 deploy it and try a trial run of the program, 0:44 of course check for any errors that might occur. 0:48 >> So there's a validation part in there too. 0:53 So you're validating the behavior of the application and making sure, 0:55 does this thing do what it's supposed to be doing? 0:58 >> Exactly, and making sure that it satisfies some user stories. 1:00 >> Right, and then you take whatever you've noticed, 1:03 whether it be positive or negative, and you're reporting that back to the user. 1:08 And then how you do that, 1:11 there's a variety of different ways that that's done too. 1:12 >> Yeah, but for the most part, you wanna keep in mind that 1:15 this is somebody working on code, that's just trying to improve it. 1:18 So you want as much as you want to point out anything that goes wrong, 1:22 you also want to point out anything that they're doing right. 1:25 >> Right, positive and negative comments, not just criticism all the time. 1:28 >> Exactly. 1:33 >> Which is easy to fall into that habit. 1:33 So in terms of the value of code reviews, if you search the web, you'll find 1:35 opinion after opinion that people are on board with the idea of code reviews. 1:40 I don't think that that's, an idea that's challenged very often, 1:44 but I'm gonna read a Steve McConnell quote. 1:48 >> Yeah. Because I think that he kinda 1:52 gets to the point of some research that he did, 1:53 some studies or that he's paid attention to that really drives the point home. 1:55 So Steve McConnell from his book, Code Complete, which is an awesome book says, 2:00 software testing alone has limited effectiveness. 2:06 The average defect detection rate is only 25% for unit testing, 2:09 35% functional testing and 45% for integration testing. 2:13 And in contrast, the average effectiveness of designing code inspections are 55 and 2:19 60%. 2:25 So substantially higher than like other forms of testing that you can do 2:26 >> Those are amazing numbers and 2:31 I completely agree with those results. 2:33 But I think the focus of those numbers are misleading and in the wrong place. 2:36 >> Interesting, so I've got a confession. 2:41 I'll just get it, I'm getting into the habit of doing this on the show. 2:43 >> [LAUGH] >> Get it 2:46 out of the way right off the bat. 2:47 I've probably done about everything wrong that you can do about code reviews. 2:48 >> Father, forgive me. 2:52 >> [LAUGH] So for instance, I have found myself in 2:54 various situations being I think I've heard you call it the GateKeeper. 2:59 >> Yes. 3:04 >> So what code reviews meant in that situation was that everyone who committed 3:05 code to the project and made changes, I was reviewing that. 3:09 Furthermore it wasn't really integrated into our process. 3:14 So I kinda would have to review the commit history. 3:16 >> Yeah. >> [LAUGH] 3:20 >> The project, and 3:21 be like right, I left off here on Tuesday, it is now Thursday, 3:22 I'd better read some of these commits, some of these check ins. 3:25 So I would start to do that and 3:29 of course the tool didn't support providing feedback, so I'd be writing 3:31 things down in an email typically, which is disjointed from the code. 3:36 >> A mess. 3:40 >> A complete mess and over time >> And it wasn't like we weren't getting 3:41 any value, but gradually over time because nothing really worked well and 3:45 it was always one direction, like me giving feedback to members of the team, 3:50 it breaks down and we would quit doing it, so whatever value we were getting out of 3:55 it, we stopped getting it because we just quit. 4:00 >> Yeah, and I think a lot of that is being able to address 4:02 code review as you would just any part of the development workflow. 4:07 You have to create a workflow for yourself. 4:12 And GitHub does that really well in terms of you could that daily peer reviews 4:15 based upon pool requests and just going over that even as a group. 4:21 And going through and approving each pool request. 4:26 On then, at the end of the day. 4:30 Maybe having someone take a quick look over at, over all the poll request that 4:32 have been made just a quick look at the reviews that were done on that. 4:36 >> Right, so real quickly poll requests, 4:40 which I think that's terminology that others are starting to adopt. 4:42 But originally, what I think was a GitHub idea, right? 4:45 >> Yes. 4:47 So get is the source control that they're using, but 4:48 they're layering on this idea of a PR or poll requests. 4:51 >> Yes. 4:54 >> And so you're working on a branch that's separate from whatever your main 4:54 and master branch is. 4:58 >> Yes. 4:59 >> You make all your changes and 5:00 then you say hey I'm ready to merge this upstream or into main branch. 5:02 You do a PR which basically is sort of a fancy way of saying hey, 5:06 can you review this thing? 5:11 At the point and time that you say, I'm ready for 5:12 a review, Nicholas can you review this for me? 5:14 >> Yes, and a lot of times, the beginning step of that whole 5:16 process is to fork the repo that you're working on in the first place, so 5:22 forking the review and making a copy of it for yourself. 5:26 Making changes to your copy and 5:30 then making a poll request to ask whoever's overseeing this project. 5:32 Hey, I've made these changes for these reasons, could you look it over? 5:38 And commit it if it's good, or make comments if it needs improvement. 5:42 >> Right. >> And it's a solid way of introducing 5:47 a workflow for your code review. 5:52 But also another problem is, I would get annoyed if I had to do all the. 5:56 >> [LAUGH] >> All of the pool requests for 6:02 a particular project. 6:05 Or if I was reviewing all of those pool requests. 6:06 >> All right so let's dig into that a little bit because, 6:08 like I just mentioned a minute ago, 6:11 that's something I've absolutely been guilty of is being that gatekeeper person. 6:12 But in fact, to make code reviews as effective as possible, 6:17 it should be something that's equally shared across the entire team. 6:20 Yes, and I think guilty might be a key word there, right? 6:23 >> [LAUGH] I wear guilt well. 6:28 >> [LAUGH] But you're hogging all this learning material. 6:30 Everyone in your team is studying things. 6:35 >> Yep. >> Some of them are going to overlap and 6:40 some of them are going to be way out on a left field. 6:42 >> Right. >> And so everyone has this 6:45 pool of knowledge to themselves. 6:48 But, when you designate one code reviewer, 6:51 that one code reviewer gets little pieces of all that knowledge. 6:54 >> Right. 6:59 >> And then no one else dies. 7:01 >> It stops there. 7:03 >> So every team member is isolated from every other team member's poll of notch 7:03 if you were to split that up where peers are reviewing peers, 7:09 then like a pool of information just puddling on itself, 7:15 we get information from everyone else. 7:19 And actually I can in that situation slightly slow down 7:21 the amount that I'm taking in from outside sources. 7:26 Because I'm getting all that from the sources inside my own team. 7:29 I'm seeing all that play out in a code base that I'm currently working on 7:33 instead of trying to decipher someone's example that's out of context. 7:37 >> Yeah, exactly, which is great. 7:41 And it also it helps just like start those conversations about the code you know and 7:43 people say, hey I noticed that you did this thing over here. 7:47 Can you explain that to me and you can use that as a starting point and 7:51 jumping off point to have some of those deeper conversations. 7:54 >> Exactly, or even deeper than that. 7:57 I notice you're throwing this exception error. 8:00 I don't really understand what the wording is. 8:03 Is there a better way to communicate to our team and 8:05 the user what this actually means. 8:08 Is this in the right format that we wanna look at? 8:11 >> Right. 8:14 >> Even inside their own project. 8:14 >> Now, you wrote a blog post recently about some of the current 8:16 misunderstandings or misconceptions around code reviews. 8:19 And one of them that I thought was really interesting was, 8:23 you talked about this expert like mindset. 8:25 So for instance over time you develop a way or a habit of looking at things and 8:30 saying, I recognize this problem and this is the solution that I do. 8:36 But you might miss an opportunity to think about it differently. 8:40 And that's where if you have your entire team involved in the code 8:42 review you made the point that someone, maybe even with less experience, 8:46 they're actually going to have a better chance at seeing something differently and 8:51 asking the question that says well, have we considered this? 8:56 Did we think about that? 9:00 >> Yeah, and I think part of that is someone who's fresh and 9:01 new to the table, often times is often just thinking 9:06 a lot of detail about something that we've taken for gratitude for a long time. 9:11 >> right. >> We learned that two years ago, 9:17 I don't have to reassess that. 9:19 I'm just gonna glance over that. 9:21 That's just a meta tag. 9:23 >> Right, you can kinda short circuit the thought process you originally went 9:24 through maybe multiple times. 9:27 To establish that pattern or that habit and 9:29 you just sort of just cut to the chase. 9:31 And say, hey, I know how to do this. 9:32 >> Example of that, I was trying to do a code review for 9:35 one of our students and I kept on getting the same error. 9:39 And I was like, nothing is wrong with this chunk of code, then I spent like a good 20 9:43 minutes on this chunk of code in for I looked directly above that piece of code, 9:48 and they had actually written the same piece of code 9:52 on a different configuration that was overriding their lower piece of code, 9:56 because it was then a document on ready. 10:01 So, this was running first, then that, and this didn't matter all. 10:04 >> Right, right. 10:07 >> It's something I would've never thought about, because I wouldn't think that you 10:08 would have to write those two pieces of code twice you don't. 10:11 But fresh eyes will catch that pretty quick. 10:15 I wouldn't expect it to be there, and so I wouldn't look for it. 10:18 >> Yeah, yeah so you make the point about, you don't have to be an expert. 10:23 And that helps reinforce the idea to spread the responsibility around the team. 10:27 Everyone has something to contribute, right? 10:32 >> And I wanna clarify. 10:35 >> [LAUGH] >> I don't believe in experts. 10:37 I think there's beginners. 10:39 I think there's people who are an intermediate level. 10:41 I even think there's experienced programmers but 10:45 even when I've looked at experienced programmers coming to a new topic, 10:48 the best I've seen don't approach it arrogantly, 10:52 they're in there trying to learn as much as possible from a beginner's stand point. 10:57 And to say that you're an expert, that you determine what 11:04 code passes and what code doesn't pass is to miss a lot of good ideas. 11:09 The code determines that. 11:14 Our Results determine that. 11:15 >> So being more open minded and flexible, 11:17 those are all real positive things in a team situation of course. 11:20 We also touched upon, and you made the point, that code review is not 11:25 specifically about finding mistakes, so sharing knowledge and information. 11:28 And then the third thing, again, 11:32 was I think an important point to make is that it's easy to think of code review as 11:34 this other thing that you have to do and it's distracting from writing code. 11:38 So, you make the point that code review, 11:43 it's a mistake to think of it as not part of the coding process. 11:45 >> Yeah, so those two are very connected together. 11:49 Doing code review Is like being able to jump into a project 11:55 that's being finished and understanding, and 12:01 being able to interpret how it's working without actually having to build it. 12:06 So whereas your fellow student might be building each project from scratch and 12:11 trying to like divine some great way of programming over and over again. 12:19 You're actually spending 30 minutes each 12:26 time going through the entire project over and over again. 12:29 And as long as you've done the project before, as long as you've attempted it, 12:34 then you get to see all these different Approaches and 12:38 ways to take on the problem. 12:40 And that inevitably gets absorbed into how you think about the problem in a way 12:42 that's much faster than trying to build it over and over and over again from scratch. 12:47 I think that's a lot of where innovation comes from in the tech industry 12:52 is that we are more open to other people collaborating with us and 12:57 more open to accepting good ideas from elsewhere. 13:02 >> Yeah, and I really enjoyed the point, to, of the reminder that becoming 13:06 a better developer which is something that obviously we're all typically 13:11 tuned into or wanting to do that writing code is an aspect of that but 13:16 reading code is something that we do or should be doing more often. 13:21 >> I remember reading that early on in my development. 13:25 You have to read source code. 13:28 You have to read source code. 13:30 There's only so far you can take your skills and abilities without 13:32 having that moment to go, this is how I've been doing it. 13:39 How does it weigh up against someone else's way of doing it? 13:45 And what are they doing well? 13:48 What am I doing well and how can we merge these two ideas? 13:50 That's innovation. 13:53 >> Yeah, absolutely. 13:54 So in terms of actions, 13:56 to be successful at code review, first of all get everyone involved. 13:58 >> Yeah. >> It should be part that's equally shared 14:02 across the team integrated into your process. 14:04 >> Yes. >> The more that you can make it just 14:07 part of what you do the more likely you are to keep up with it. 14:09 And actually stick with it. 14:13 >> And make it easy and smooth for yourself. 14:14 Like when we started doing these code reviews for students, 14:18 we didn't have automatic validation. 14:22 We didn't have automatic downloads. 14:24 All that had to be built. 14:27 And the amount of time it saved me the amount of frustration it saved me 14:29 over the past year has been extreme. 14:35 >> Right, yeah and I guess related to that is like instead of checking syntax, 14:38 put a renter as part of your build process, so 14:43 that's not where you're spending your time. 14:47 Let a tool take care of those repetitive things, that tools are really great at. 14:49 >> Exactly and if you're working on the same project you could integrate a series 14:52 of just basic tests just so you know that you're meeting those user stories. 14:58 But what we're concerned about is ow we're architecting the entire code. 15:03 What we're actually doing to organize it so it's easy to read, and 15:09 so it's efficient to function. 15:14 >> Excellent, have you ever used a checklist? 15:16 Do you find checklist helpful to remind you like, 15:19 hey I'm about to do a review these are the things I need to do? 15:22 >> I hate to say it, James. 15:26 >> [LAUGH] >> I'm not naturally an organized person. 15:28 We do use user stories, I use user stories all the time and 15:32 I have things that I'm constantly looking for in my own mind, but oftentimes for me, 15:36 revealing students, I have to change that based upon which student I'm reviewing, 15:42 because I know where they're at, and I try to follow that. 15:48 >> I see so you already are thinking about, 15:53 like I know this person tends to make these kinds of mistakes or 15:55 needs this kind of support; >> I try to keep that in 15:58 mind as much as possible. 16:01 I think it's a good idea to keep involved Where your team is at relative 16:03 to yourself and try to introduce things that you think the team can latch on to. 16:08 So we can step up to a higher level. 16:15 >> Or support that person- >> Exactly. 16:17 >> In their growth and learning. 16:18 >> Yup. 16:20 >> Excellent. 16:21 Well, thanks for joining me on this show on talking about code reviews. 16:21 This has been great. 16:24 >> I hope it was useful for you. 16:25 I hope there's more people that get more from their learning by doing code review. 16:26 I think it's an excellent way to go about 16:31 building your skills as a new developer and as an experienced developer. 16:35 >> For additional information about code reviews, be sure to check 16:39 the notes that accompany this video for links to additional resources. 16:42 Also rate this video let us know how we are doing or if you have a topic that you 16:45 would like to see us discuss in the future let us know about that too thanks for 16:49 watching and we'll see you next time. 16:53