9:59:59.000,9:59:59.000 Good afternoon RailsConf. 9:59:59.000,9:59:59.000 I hope you're all doing great after lunch. 9:59:59.000,9:59:59.000 Thanks so much for coming. [br]I've very excited to be here. 9:59:59.000,9:59:59.000 My name is Derek Prior. [br]I'm a developer with thoughtbot... 9:59:59.000,9:59:59.000 ...and I'm here today to talk to you [br]about code reviews, doing them well... 9:59:59.000,9:59:59.000 ...and what it means for the culture [br]of your team when you are in the 9:59:59.000,9:59:59.000 type of place that does them well. 9:59:59.000,9:59:59.000 So, let's start with a show of hands. 9:59:59.000,9:59:59.000 Just so I get an idea where everyone is at. 9:59:59.000,9:59:59.000 How many of you are doing code reviews 9:59:59.000,9:59:59.000 as a regular part of your day, [br]every day already? 9:59:59.000,9:59:59.000 Ok. And how many of you really [br]enjoy doing them? 9:59:59.000,9:59:59.000 Okay, a few less. 9:59:59.000,9:59:59.000 And how many of you do them because[br]you feel like you have to? 9:59:59.000,9:59:59.000 It's about equal. Okay. 9:59:59.000,9:59:59.000 All the people who said they really[br]do enjoyed them also said 9:59:59.000,9:59:59.000 they do them because they have to.[br]Okay. 9:59:59.000,9:59:59.000 So, why is it that we do reviews in the first place? 9:59:59.000,9:59:59.000 This is pretty easy, right? 9:59:59.000,9:59:59.000 To catch bugs. 9:59:59.000,9:59:59.000 We're going to have somebody look[br]at every individual line of code 9:59:59.000,9:59:59.000 and we're going to find what's wrong. 9:59:59.000,9:59:59.000 Not really. 9:59:59.000,9:59:59.000 That's not why it is interesting, right?[br]I've been doing code reviews for 9:59:59.000,9:59:59.000 over ten years now.[br]I used to hate them. 9:59:59.000,9:59:59.000 And, they were the worst[br]part of my day. 9:59:59.000,9:59:59.000 We did it just for compliance[br]documentation at one of my former jobs 9:59:59.000,9:59:59.000 But now I think that code reviews are[br]one of the primary ways 9:59:59.000,9:59:59.000 that I get better at my job [br]every single day. 9:59:59.000,9:59:59.000 So 9:59:59.000,9:59:59.000 Yes, we're gonna have fewer bugs[br]in code that's been peer reviewed. 9:59:59.000,9:59:59.000 than in code that [br]has not been peer reviewed. 9:59:59.000,9:59:59.000 But studies on this show that[br]the level of QA that we get 9:59:59.000,9:59:59.000 out of code review, doesn't meet[br]the level of expectation that we have 9:59:59.000,9:59:59.000 as developers and managers. 9:59:59.000,9:59:59.000 So, that is, we think that by doing[br]code reviews we're getting this much QA 9:59:59.000,9:59:59.000 when in reality we're getting [br]somewhere down here. 9:59:59.000,9:59:59.000 So, why is that? 9:59:59.000,9:59:59.000 Well, the reason is [br]when we do code reviews, 9:59:59.000,9:59:59.000 we're looking at a slice of a change. 9:59:59.000,9:59:59.000 We're looking at the git diff essentially. 9:59:59.000,9:59:59.000 And we can catch syntactical issues,[br]or problems where you might be 9:59:59.000,9:59:59.000 calling a method on nil. 9:59:59.000,9:59:59.000 But we can't catch the really [br]heinous stuff which happens when 9:59:59.000,9:59:59.000 your whole system interacts [br]and corrupts your data. 9:59:59.000,9:59:59.000 So, code reviews are good for [br]some level of bug catching 9:59:59.000,9:59:59.000 but it's not the end-all be-all, right? 9:59:59.000,9:59:59.000 So, what are they good for?[br]I already told you that I think 9:59:59.000,9:59:59.000 that code reviews make me better every day, [br]and I want you all to feel the same way 9:59:59.000,9:59:59.000 Well, in 2013, Microsoft and the [br]University of Lugano in Switzerland 9:59:59.000,9:59:59.000 came out with this study: "Expectations,[br]Outcomes, and Challenges of Modern 9:59:59.000,9:59:59.000 Code Review." So, in it, what they did was[br]look at various teams across Microsoft 9:59:59.000,9:59:59.000 which is a huge organization - they have [br]several teams with different 9:59:59.000,9:59:59.000 uh, they have Sr. developers, [br]Jr. developers, managers, everybody 9:59:59.000,9:59:59.000 all working on a different product. 9:59:59.000,9:59:59.000 They surveyed all these people to ask them[br]"What is it you get out of code review? 9:59:59.000,9:59:59.000 "What do you like about it, what do you [br]don't like about it?" 9:59:59.000,9:59:59.000 When they were done surveying them,[br]they watched them do code review 9:59:59.000,9:59:59.000 and asked them questions afterwards. 9:59:59.000,9:59:59.000 And, finally, they looked at all of the [br]feedback that was given. 9:59:59.000,9:59:59.000 Every comment that was logged in their[br]code review system. 9:59:59.000,9:59:59.000 And they manually classified it 9:59:59.000,9:59:59.000 They said "This one has to do with a bug[br]that they found. This one has to do with 9:59:59.000,9:59:59.000 a style comment. This one has to do with [br]a possible alternative solution." 9:59:59.000,9:59:59.000 So after doing all this work, [br]what they found was that people 9:59:59.000,9:59:59.000 consistently ranked finding bugs very high[br]as a reason for doing code reviews. 9:59:59.000,9:59:59.000 But in the end, it was actually a lot less[br]about finding bugs than anyone thought. 9:59:59.000,9:59:59.000 The chief benefit they saw from code [br]review were Knowledge Transfer, 9:59:59.000,9:59:59.000 Increased Team Awareness, and[br]Finding Alternative Solutions to problems 9:59:59.000,9:59:59.000 That sounds a lot more interesting to me[br]than hunting through code looking for bugs. 9:59:59.000,9:59:59.000 Through this process we can [br]improve our team. 9:59:59.000,9:59:59.000 One person involved in the study [br]said this: 9:59:59.000,9:59:59.000 "Code review is the discipline of[br]explaining your code to your peers 9:59:59.000,9:59:59.000 that drive a higher standard of coding. 9:59:59.000,9:59:59.000 I think the process is even more important[br]than the result." 9:59:59.000,9:59:59.000 I really do like that last part[br]even though it's not on the slide, right? 9:59:59.000,9:59:59.000 The process is more important[br]than the result. And we're gonna talk 9:59:59.000,9:59:59.000 about that today. Just by going through[br]the process of code review the right way, 9:59:59.000,9:59:59.000 we're going to be improving our team.[br]Regardless of the actual results 9:59:59.000,9:59:59.000 that we're seeing on any individual change. 9:59:59.000,9:59:59.000 But I also like the definition here[br]"the discipline of explaining your code 9:59:59.000,9:59:59.000 to your peers." I tweak[br]it just a little bit to say 9:59:59.000,9:59:59.000 rather than trying to explain it to them 9:59:59.000,9:59:59.000 Code review is one of the few chances we[br]get to have a black-and-white conversation 9:59:59.000,9:59:59.000 about a particular change. We often talk[br]at abstractions 9:59:59.000,9:59:59.000 like when we come to conferences[br]like this, we talk in large abstractions 9:59:59.000,9:59:59.000 about things. And those are really[br]comfortable conversations. 9:59:59.000,9:59:59.000 In code review, we have to get down [br]to the implementation details 9:59:59.000,9:59:59.000 and talk about how we're [br]going to apply these things. 9:59:59.000,9:59:59.000 So, like so much else that we do,[br]it's a communication issue. 9:59:59.000,9:59:59.000 If we get better improving code reviews,[br]then what we're really doing is improving 9:59:59.000,9:59:59.000 the technical communication [br]on our team 9:59:59.000,9:59:59.000 So, the study also found that[br]those benefits I cited earlier 9:59:59.000,9:59:59.000 were particularly true for teams that[br]had a strong code review culture. 9:59:59.000,9:59:59.000 So, what does it mean to have a[br]strong code review culture? 9:59:59.000,9:59:59.000 To me, it means that your entire team[br]has to embrace the process. 9:59:59.000,9:59:59.000 Everybody has to be involved. It can't be[br]something that the senior developers 9:59:59.000,9:59:59.000 do for the junior developers. [br]It's a discussion. 9:59:59.000,9:59:59.000 That's what we're after here. 9:59:59.000,9:59:59.000 So, as I mentioned earlier, [br]I've been doing code reviews 9:59:59.000,9:59:59.000 for over ten years now. But only in the[br]last 2-3 have I started to see a real 9:59:59.000,9:59:59.000 improvement in what I'm getting out of[br]them, and in myself because of them 9:59:59.000,9:59:59.000 And so why is that? 9:59:59.000,9:59:59.000 I think it's because I'm part of a strong,[br]code review culture at thoughtbot. 9:59:59.000,9:59:59.000 And, at thoughtbot we often go to clients[br]of various sizes to help them with issues 9:59:59.000,9:59:59.000 that they're having. And nobody comes to[br]us and says, "I really need help improving 9:59:59.000,9:59:59.000 the code review in my team."[br]That never happens. 9:59:59.000,9:59:59.000 But when we get on the ground[br]with those teams, we often find 9:59:59.000,9:59:59.000 that there isn't a strong[br]code review culture. 9:59:59.000,9:59:59.000 One of the challenges that we have[br]is how do we get people to have 9:59:59.000,9:59:59.000 this culture around code reviews. 9:59:59.000,9:59:59.000 And there are a lot of little rules that[br]we suggest following in our guides. 9:59:59.000,9:59:59.000 But if you look at them, you can [br]really boil them down 9:59:59.000,9:59:59.000 to two key rules of engagement, [br]two things that you can do today 9:59:59.000,9:59:59.000 to start getting better at code [br]reviews with your team. 9:59:59.000,9:59:59.000 The first of them is something [br]that you can do as an author. 9:59:59.000,9:59:59.000 The second is something[br]that you can do as a reviewer 9:59:59.000,9:59:59.000 when you're providing feedback. 9:59:59.000,9:59:59.000 So first, as an author, [br]what are we going to do 9:59:59.000,9:59:59.000 to make sure that our code reviews[br]get off on the right foot? 9:59:59.000,9:59:59.000 So, this quote here is from Gary [br]Vaynerchuck and he was talking 9:59:59.000,9:59:59.000 about social media marketing,[br]or something, not interesting. 9:59:59.000,9:59:59.000 But, it's also applicable to code reviews. 9:59:59.000,9:59:59.000 Believe it or not. 9:59:59.000,9:59:59.000 So, in a code review, your content[br]is the code, it's the diff 9:59:59.000,9:59:59.000 it's what you've changed. 9:59:59.000,9:59:59.000 The context is why that's changed. 9:59:59.000,9:59:59.000 Your code doesn't exist for its [br]own benefit. It solves a problem. 9:59:59.000,9:59:59.000 So the context is [br]what problem is it solving. 9:59:59.000,9:59:59.000 That study I cited earlier found that[br]insufficient context is one of the 9:59:59.000,9:59:59.000 chief impediments to a quality review. 9:59:59.000,9:59:59.000 So, as an author, we need to get, we need [br]to know this and get out in front of it. 9:59:59.000,9:59:59.000 We're gonna provide excellent context[br]around our changes. 9:59:59.000,9:59:59.000 So let's have a look[br]at a real world example. 9:59:59.000,9:59:59.000 This is a commit, or a pull request title,[br]that you might see come across your email. 9:59:59.000,9:59:59.000 "Use type column first in[br]multi-column indexes" 9:59:59.000,9:59:59.000 The specifics here aren't [br]particularly important 9:59:59.000,9:59:59.000 if you don't understand what this means. 9:59:59.000,9:59:59.000 The problem here is that [br]there is no why. 9:59:59.000,9:59:59.000 So, I can look at this change[br]and I can say "Well, yeah, this changes 9:59:59.000,9:59:59.000 the order of the indexes, or the order [br]of columns on multi-column indexes 9:59:59.000,9:59:59.000 But I can't tell you if that was the best[br]solution to the problem you're solving. 9:59:59.000,9:59:59.000 I can't really learn anything from it. 9:59:59.000,9:59:59.000 So, this is a loss. It's not interesting[br]for me to review, and it's just going to 9:59:59.000,9:59:59.000 get a thumbs up and move on. 9:59:59.000,9:59:59.000 Actually, what I would do with a change [br]like this is comment and say, 9:59:59.000,9:59:59.000 "Can you provide some more context here?" 9:59:59.000,9:59:59.000 And a lot of times, what we find happens,[br]is somebody updates the pull request 9:59:59.000,9:59:59.000 or adds a comment that says [br]something like this. 9:59:59.000,9:59:59.000 Right? 9:59:59.000,9:59:59.000 So, okay, so I guess you guys [br]think that's not better, which is true. 9:59:59.000,9:59:59.000 That's not better. 9:59:59.000,9:59:59.000 So, this is probably a link to Github [br]or maybe it's a link to Geer or whatever 9:59:59.000,9:59:59.000 your issue tracker is, right?[br]And a lot of people do this. 9:59:59.000,9:59:59.000 They'll provide a short explanation and [br]say, "For more details, see this ticket." 9:59:59.000,9:59:59.000 But what you're doing there is you're[br]making the reviewer hunt for this context. 9:59:59.000,9:59:59.000 If they click this issue, are they gonna [br]see a good description of what you're doing? 9:59:59.000,9:59:59.000 Probably not, right? 9:59:59.000,9:59:59.000 They're gonna see a report of a problem,[br]maybe some discussion back and forth 9:59:59.000,9:59:59.000 about how to solve it, and then maybe[br]a link to this pull request or something. 9:59:59.000,9:59:59.000 But they've got to go through all this[br]and they've got to hunt for that context 9:59:59.000,9:59:59.000 and put it together until they are in the [br]right frame of mind to review the changes, 9:59:59.000,9:59:59.000 to tell you whether or not [br]it is the right solution. 9:59:59.000,9:59:59.000 So, here's an improvement. 9:59:59.000,9:59:59.000 It's not important to read this, again. 9:59:59.000,9:59:59.000 So, what we're doing is identifying [br]the problem with index ordering. 9:59:59.000,9:59:59.000 That's what we do first. 9:59:59.000,9:59:59.000 Why is it a problem? 9:59:59.000,9:59:59.000 We're gonna back it up with some [br]postgres docs, and those link off to more 9:59:59.000,9:59:59.000 information if you need it.[br]And because this particular change 9:59:59.000,9:59:59.000 was a change to Rails, and we need[br]to be concerned with multiple adapters, 9:59:59.000,9:59:59.000 we're also going to back it up with some [br]MySQL documentation. 9:59:59.000,9:59:59.000 And finally, we're gonna talk about [br]why this is the best solution. 9:59:59.000,9:59:59.000 So that's a lot of context. 9:59:59.000,9:59:59.000 But it's important to note that now, as[br]somebody is coming along to review this, 9:59:59.000,9:59:59.000 I know why this change is being made,[br]and maybe I'd even learn something about 9:59:59.000,9:59:59.000 how multi-column indexes work, by reading[br]through some of this documentation. 9:59:59.000,9:59:59.000 So there's value for me to review this. 9:59:59.000,9:59:59.000 So, as an author, we're going to [br]provide sufficient context. 9:59:59.000,9:59:59.000 What we're trying to do here, like 9:59:59.000,9:59:59.000 You've been working on this change for[br]four hours, two days, however long 9:59:59.000,9:59:59.000 it took you to fix this, right? 9:59:59.000,9:59:59.000 What you need to do is [br]bring the reviewer up to speed. 9:59:59.000,9:59:59.000 Let them know what you learned[br]in this process, 9:59:59.000,9:59:59.000 and put them on equal footing with you. 9:59:59.000,9:59:59.000 So I challenge you to provide[br]two paragraphs of context 9:59:59.000,9:59:59.000 with every change you made. 9:59:59.000,9:59:59.000 At first it's going to be really painful,[br]and there are some changes that it's 9:59:59.000,9:59:59.000 torturous to describe in two paragraphs. 9:59:59.000,9:59:59.000 And yes, it's going to take you more time. 9:59:59.000,9:59:59.000 But how much more time? 9:59:59.000,9:59:59.000 I don't know, like, five minutes, maybe? 9:59:59.000,9:59:59.000 And it avoids a whole round of,[br]that round of questioning I described 9:59:59.000,9:59:59.000 before, like "Why are you making this[br]change?" So we've headed that off. 9:59:59.000,9:59:59.000 And the extra bonus is that all of that[br]context we saw earlier 9:59:59.000,9:59:59.000 gets to live on in the commit. 9:59:59.000,9:59:59.000 We're gonna squash that [br]and we're gonna save that. 9:59:59.000,9:59:59.000 So rather than that one we saw before[br]that had an issue - a link to G or a link 9:59:59.000,9:59:59.000 to Github, that's going to go away[br]as soon as we stop paying that bill. 9:59:59.000,9:59:59.000 But our git history is going to stay with [br]us, and we're going to see that there. 9:59:59.000,9:59:59.000 Ok, so that's what we can do[br]as an author. 9:59:59.000,9:59:59.000 What about as a reviewer? 9:59:59.000,9:59:59.000 What can we do to 9:59:59.000,9:59:59.000 make it so that our [br]feedback is well received? 9:59:59.000,9:59:59.000 We like to call this[br]ask, don't tell. 9:59:59.000,9:59:59.000 To start off with, it's [br]important to note that 9:59:59.000,9:59:59.000 research has shown there's a negativity[br]bias in written communication. 9:59:59.000,9:59:59.000 That is to say if I have a conversation[br]with you face-to-face and I give you 9:59:59.000,9:59:59.000 some feedback, or maybe [br]even over the phone, 9:59:59.000,9:59:59.000 just in a place where you can hear [br]the tone of my voice, hear the inflection. 9:59:59.000,9:59:59.000 You're gonna perceive that one way. 9:59:59.000,9:59:59.000 If I give you that same feedback written,[br]like in the form of a pull request review, 9:59:59.000,9:59:59.000 it's going to be perceived [br]much more negatively 9:59:59.000,9:59:59.000 So, it's important that [br]we're cognizant of this 9:59:59.000,9:59:59.000 negativity bias in our[br]written communication. 9:59:59.000,9:59:59.000 We have to overcome this if we want[br]our feedback to be taken in the right way. 9:59:59.000,9:59:59.000 One way to do that[br]is to offer compliments. 9:59:59.000,9:59:59.000 I would suggest that you do that. 9:59:59.000,9:59:59.000 So, if you find something in a [br]pull request that is something you learned 9:59:59.000,9:59:59.000 or is something that you think [br]is done particularly well, 9:59:59.000,9:59:59.000 those are great to call out. 9:59:59.000,9:59:59.000 It lets everybody know that... 9:59:59.000,9:59:59.000 "Yes, you taught me something. [br]That's great. Thank you very much." 9:59:59.000,9:59:59.000 Rather than just always nitpicking[br]at the change. 9:59:59.000,9:59:59.000 But, there's gonna come a time when[br]you need to provide critical feedback