Amazing Code Reviews: Creating a Superhero Collective • Alejandro Lujan • GOTO 2019

[Music] so to frame the conversation of what we're going to talk about today let me paint you a little bit of a picture and you'll let me know whether you find yourself identified in here but before we do that actually who uses code reviews as sort of like day-to-day tool or process of their job awesome you're in the right talk so let's paint a picture here let me know if they sound familiar so imagine you join a team and this may be because you could hire any company or maybe change teams within the company and you know they want you get some work to do you get a task and you go ahead and work on it for a while now here's the thing you know you're really good but you want to show others what you're made of so you're gonna work for about three weeks on this thing create a PR you've written some code and you know it's about that 5,000 lines of code a bit of work but again you polished it really really well because you want to show what you're made of you put that PR out there get a couple people to review it and then you get a couple comments so I'm about indentation and variable naming you fix those in one commit because they were really easy and then you get your approval and they're about hear something settles in and you go why wasn't that easy and then you grind your teeth close your eyes and hit the merge button and protist not broken yet and then you go and live in fear for the next few months because you know something was wrong you know you've broken something you don't know what it is yet so hopefully we've seen a few red flags in here of a bunch of things that could have been better in that process and that's what we're gonna talk about today now I'll tell you why this topic is important for me why did I pick it as a talk here at this conference and what happened was so I've been in the in the industry for about twenty years now and have been a developer an architect a team leader trainer so I've been around I've done my when I joined Shopify two years ago I figured I know what I'm doing I'm the new team but I know my stuff and what happened was on the first few months of working at Shopify I've got the most gruesome code review Sam ever had first PR to be like a week of round back-and-forth three people going back and forth so naturally what happened was I took it personally maybe I'm not as good I'm not as good as I thought it was maybe I don't know enough about this particular area of work like maybe maybe this team doesn't like the way I work right so there's a whole bunch of interpretations we can take out of sort of a long code review process but what I noticed was once once I started sort of looking into the actual comments was though not about me there were not about my skill if there were about the work that was being done so one way I like to think about it is that great code reviews are both a symptom symptom and a contributing cause to highly effective teams if your team does not have a strong code review process then there's probably a lot more you could be doing you could be moving faster and then do it working better and that's what I want to focus on of course building software includes a whole bunch of other stuff right so some of those steps are automated to a certain point so what we're gonna focus on is the human aspect of as an author I have something for others to review and as a reviewer I have work that I wanna offer feedback on so that's what we're gonna focus on so the way I've broken down this is usually there are four things we care about when we're building software building the right thing so actually it being correct building the thing right so keeping quality high building it fast you know obviously time is money and being first to production is always an advantage and building it together so let's go through each one of those and talk about what about the code review process could we be doing and to move faster and build better things okay and a lot of these are observations that I've that I've made of how we do things at Shopify but looking at other teams and other companies I've seen sort of common patterns that I think around okay so what about building the right thing so a lot of you will be very familiar with this metaphor this is a wonderful car it's beautiful as strong as durable it is also very expensive to build so if I'm giving a task to build a car and I create this and then I show it to my customer then they might ask questions like what do I put my dog and how do I Drive this thing to the Scandinavian winter those are things we weren't thinking about when building this car right so instead of building this really expensive thing then we could be building a miniature version of it right this much cheaper much faster to build but even cheaper than that would be a drawing like this so a good illustrator might do this and I don't know an hour or two but if this is the first time you're communicating that idea even I can do something like this in a few minutes okay so obviously what's the metaphor here building something quick and fast to show intent is much more valuable in the early process of creating something than going for the whole polished thing so showing direction early PRS or sharing early work for feedback is very very valuable and why is it well because we want to minimize if we work I want to avoid working on something for three weeks so that then show it to someone to someone so that they go oh actually that's not going to work at all because the tool you're using is not available on CI because the algorithm you pick this trim is low in this particular case it's at whatever it is right we only have so much information when we build things when we get feedback we might we the idea is to learn a lot about the context so one of the things of course in here is when you're shooting early work as a person as a human being you're putting something out there that's not great that's not polished that percent all that you can do so we have to be open-minded to getting criticism here right to learning from another the feedback that we're getting learning about the context learning about how do things more effectively or how other people see something that for us might look perfectly fine so this is one of the things I had trouble with because again I felt I knew what I was doing this is my best work and then I put it out there and go oh actually this person is much more strong or hostile reviews and I don't know API design for example is one of the things I had to struggle with so being open-minded here is really really important another thing that's important that these products are like the team dynamics is to make sure that you're labeling this work properly what do I mean by that if you said early feedback sort of piece of work make sure that the person who's reviewing knows that so this might be a github we use labels or seven one seven one used draft PRS and github you raise your hand handful of you I don't know if other sort of like big bucket or the other have a similar sort of tool but you gotta find a way to let people know what's the level of sophistication of this piece is this prototype very very early on where you show indirection or sit halfway there or is it polished this is ready this is I think ready to ship what what do the viewers think it gives them a chance you know how deep to go and on which aspects of the review right so I'm a big fan of draft DRS and github and I'll very quickly show you here what that looks like when you're creating a pull request and github instead of doing clicking on a create pull request this is a relatively new feature by the way a few months I think you now have a drop-down that says create a draft pull request and he has two interesting Wow save three interesting things one is you cannot merge a draft the aren't meant to be early word second it doesn't ping code owners so who here uses code owners on the repos so for us it's a big problem noise with code owners is an issue so a draft PR doesn't actually pincode owner so it reduces the amount of noise and of course all the other important things is that it communicates the fact that this is an early work pull request so and this is a label that you would see when you create a draft will request and then you see a little option in there called ready for review which converts this draft we are into a regular appear all right so that's sort of the transition and again the merge pull request option is grayed out not available when you're in a draft beer so this is just one tool it might be that you use labels in front of the pr name or whatever it is that your team chooses to do what you gotta find a way to communicate this is an early work yeah so what about building the thing right whatever quality so I would say this is probably the most important thing I learned in that sort of first few months a Java file was this idea of really breaking down and keeping my work small so I had the bad habit of creating large pieces of work just that sort of looked like a like a unit to me and it turned out that my frisbee I wear five hundred and a thousand lines and I can't like I've got a lot of push back can you break this down in three or four pieces can you make this smaller right now there's obviously no good guideline of what small days it depends a lot on the type of work you're doing the the language there's a whole bunch of variables there for me and my team and the data engineering work that I do 200 lines is what I've set for myself anything beyond that threshold I'm going to look really hard and try to break it down and why is that so important so in our team there are lots of reviews flying back and forth I have a lot of PRS open that I want people to review and other people want me to look at their work the first thing that I think is important is just the barrier to start look code lowers if the thing is smaller so if using me up the are and it's a hundred lines of code I'm like I could probably take a quick skim at it now cuz I have five ten minutes but if it's a thousand or two thousand or five thousand lines of code then it's gonna take me a lot of effort just to even take it first to look at it right so by keeping my work small keeping my units to a small line count for character cap then you're lowering the barrier so actually people starting looking at your thing so that's one now what about sort of general quality of the whole process well people will be able to go deeper if it's a small piece of work right and that's something that I noticed immediately when I see you know as a reviewer the longer pieces you have to read the more elements you have to keep in memory as you're trying to figure out the different pieces right so you end up doing quick skims instead of going deeper into each one of those elements right and the other thing that's interesting is also you know reducing blind spots it's a one way that I don't like to think about it again if there's a really big piece then you'll have people looking at it that might not understand very well let's say the algorithmic aspect of it here versus the data modeling part of it here versus sort of the language aspect of it okay and we'll talk a little bit about picking people to reviewer work as well so um what do we do about that so a lot of times you'll start exploring an idea and exploring ideas and you'll end up with a big piece of work that's fine that would be sort of your draft your early work but then you'll want to break that down into several PRS so another question who here uses stacked PRS and buy stock VRS I mean several PRS are depending on each other so anyone uses that on their team so that's a really really powerful technique because and this is something that I also had to learn how to better be more comfortable with so the workflow is very simple you create a thing that is too big your PR end up to being a thousand line you're like okay now because I want deeper reviews and actually want to polish this thing I'm gonna break it down in three or four because I can separate maybe the data model here and then the algorithm here and then the API calls over here for example break it down in pieces of our logical of course and then you'll get better more deep deeper reviews in which one of this now of course the danger is to go to the other extreme which is to create too many very small things I've never actually seen that be in a real team but what happens if you go to that extreme then the challenge is going to be on the reviewer to actually understand the whole picture right if I can only see this little slice of what you've done then that might not make sense I might need to look at five or ten other PRS over there okay so again I don't think this is very common but you know it's an interesting sort of thought experiment so what are some of the actual practical tools have me to I need to get better or I've had to get better at in order to make this work with my team for me well-organized commits are important so actually the individual comments make sense to me when I read them so I can figure out where each one of the elements were being introduced using you know base ranch's BR and like I mentioned that be ours it's also a really powerful technique because you might have again three or four pairs that are related you create you know PR one and then PR two depends on the code that you've created here so those are stock be ours I've had to go I've had to get a lot more comfortable with those and I think those are a really powerful team technique so obviously you know being getting comfortable with get or whatever system using for for this cherry-picking proper branches rebasing merging whatever it is that you team does incredibly proud powerful to get very very comfortable with this Gators of course a very powerful tool but a hard one to I think execute on at an advanced level but I think it's worth it okay so we've talked about sort of breaking down things you know in a we've used the idea of size but sizes is a very limited way of thinking about work right so I'd like I'd prefer to think about it in terms of concerns can I break down a large br into its different concerns at different pieces individual pieces that make sense right now why is that I think important again the qualitative review it might be that you know this one person is very very good at this particular algorithm and I want them to look at this or or they know the system that I mean interacting with so they'll have intelligent feedback to give me about that okay so that's one thing but another thing that's really really important is the atomicity of when things get merged and go into production and what do I mean by that and I suffered from a couple of these if you merge one large piece that includes you know five different components if one of those fails in master then you have to revert the whole thing right and now you're having to debug and find into root cause analysis and the terror on a very large piece right whereas if you broke those down and you know things broke when you invert piece number three then you don't have to look at the other pieces right so breaking things down into their individual concern this helps me actually figure out when things go wrong where's the culprit where's the root cause so I think that's also really powerful now one thing that that has a lot to do with not so much about the dynamics of how the peers have created bumper about the communication that happens in that process this is one realization that I had I sort of quickly mentioned that at the beginning after getting all those reviews that I thought at the beginning we're all about me I realized well they're actually about the work and we had a conversation about this on my team and we all have sort of boil it down to these idiots about the code it's about the product and not the personal thing so what I think is interesting here is to think about as a reviewer how do you offer feedback that embraces this and that helps people realize and understand it's not about them it's about how do we build a better product so a couple ways that I like to think about it and that I communicated with my team as well first of all as a reviewer think about how can we improve this guy not how can you improve your things so that I'm satisfied how can we improve a thing right so you start to create that that sense of common ownership right the ownership starts to translate from the one person that's the author to the team throughout this process so how can we improve this thing another thing that's really important and you'll probably seen this as well is how do I offer actionable feedback feed but that I can do something with and if I get feedback that says this is not great what I do with that right so this is really important we don't always have the solution of how to make it better as a reviewer sometimes you might be mentoring a more junior person you don't want to give them the solution that's fine but at least if you have a direction of how things can get better try to offer that okay both sides the reviewer and the author if we all assume best intention in the conversation that things go a lot more smoothly that's not always easy for people we have stories there's these people we work with we might have sort of offline connections that are different quality right so this is this is a hard thing and that goes outside of the code review process and into the team dynamics and if you are lead then encouraging this type of connection behavior conversation obviously has a huge impact on your team because once you assume the person is giving you feedback in order to make this thing better as opposed to talking about me then things can watch more suppose and then this might seem really simple and I think someone mentioned this on another talk here at the conference offer positive feedback as well if your PR only contains feedback that that sounds negative that is about things that need to be improved then it's really easy to again take a person but if that is mixed with oh this is a great choice oh that's a really cool call I didn't I didn't know you could do this in Python right if offering feedback you also include positive feedback then the person receiving that goes oh I guess I'm there in the right direction these are the things that I need to improve okay so this is really really powerful okay and here's where we're gonna try to make this work with a big room we'll see how that works I'd like making my talents as interactive as possible so this is what we're gonna do now so I'm gonna show you a few comments that have happened on a PR and what I want you to think about is what's good or not so good about each one of those and then you just shout back your answers and I'll repeat them on the mic okay so again what I'll show you the code here doesn't matter much so don't don't think about whether the code is good enough look at the comments and think about it as your firm what is good or not about receiving that more as a reviewer what's what's great or not so great about those cool let's give it a try so first one I'll give you a second to look at that and you let me know what's great or not great about the comment I see some nods and some heads shaking what's not great about that feedback listen to short yeah what else non-actionable very important what do I do with that what is the person you cannot ask him anything else confusing not context right maybe if you the author of this you have a bit more context as to what this is this is a matrix that contains integer numbers are they asking whether it contains into intervals rather actually suggesting something okay so yeah obviously not a great piece of feedback you would want to offer a bit more context or a suggestion or if it's really a question then explain the question in more details why have you chosen to use ins instead of pools wouldn't audience be more efficient whatever I what about this one you like it passive-aggressive maybe what else not to the point right so not to the point because the person is not saying exactly what's wrong about it or what could be improved it's just directing to some documentation what else yes aha he's this person is assuming that the other that the author didn't read the guidelines right so let's imagine that this has to do with using the print function maybe the guidelines say that you should use the parentheses for example right so probably a better way to offer that feedback is first of all why don't you include a link to the guidelines to the specific section that indicates what what the problem is for example or include in the comment what this is about right so this probably comes across as Kurt to breathe passive-aggressive right it is sort of actionable but the action is in fact if you ever work with this if you look at those guidelines this is a humongous document they're asking me to RTFM right okay let's do a couple more and this and then we'll go on okay not sure if that's a little small so I'll just read it out loud so the comment is how about breaking out the problem description to a markdown file that that way we have formatting and we can more easily share the full file with the candidates what is good or not so good about that it's constructive yeah what else it's written in sentences in full sentences yeah what else it uses we write so there's a connection between like I'm also taking ownership that's really important yeah it offers a solution right so as a suggestion why don't we put it in a markdown file the author might reply and go actually is make it just a plain text file for example right but but there is a solution offered as a guideline what else price it's a suggestion and not a command yes yes that's really important it's offering what would happen if we do it this way right it's it's justifying why this is a good thing to do because I could do all that and say how about breaking out the problem into a markdown file period and you know that's that's nice infrastructure than an actual but it still doesn't tell me why whereas here it's actually laying down well this is the benefit if we do it this way so we can actually have a conversation about why to do it this way okay anything else about this anything that's not great about cool okay how about too fast yeah let's go to this one I'm worried about this method being too slow can write a performance test to run it a few thousand times to gauge its speed right so why is the suggestion including more work to review right there's no justification you mean okay yeah why should the reviewers sort of help with with that work as well anything else yep so it's it's planning or only what the issue is but a suggestion for how to improve it and giving some some guidelines as well right there's a few things missing probably here and and I think there's a bit of a hint in here is like why does why does it matter so much and why do we want to do is this wedding particular yes and I think that's really important the the phrasing event is opening the discussion it's also not a command but a suggestions that is opening the discussion that's important to say they're good right it also assumes that a person hasn't already done it yeah you can imagine that the test should be in the PR and it isn't so this person is saying well I I don't see a test and we should be testing this okay a couple more what about that what's great or not so great yeah that's a good one it's either too long or too short depending on who the author is and that's also something we haven't really commented much on it depends a lot on what the who the author is with the reviewer is about the connection right if if this is an experienced developer then I shouldn't have to explain all of this to say hey you missed a single variable character we have we've had this conversation before if it's a newbie a person that you're mentoring that is maybe not familiar with the language or the guidelines then again you might want to link to to where you document this or the justification for why we want to avoid they stopped a variable name okay anything else about this one this is also the type of thing you should probably try to automate so catching something like this should be the job of a linter rather than it would be your thing so we should try to do as little work as we can and use our tools at the best weekend right how about this I like that you appreciate it this is my best riding ever this feels slow we needed to be super fast make it the lightning fast so all the things that are wrong or summarize here cool okay so in the topic of speed so talking about building the right thing and build this thing right what about building fast isn't a thorough detail review process doesn't that makes it slower well obviously it it makes the process more more thorough a more more detail but what I'm focusing about here is well the team dynamics of this thing I created work I put it out for review there's you know five ten people in my team how does that dynamic affect speed okay so obviously if I am a reviewer anything that comes my way is probably blocking others so your team has to figure out a way to prioritize reviews there might be labeling in the PR or indirect communication saying this is an urgent thing I need this to be reviewed you know between today and tomorrow or whatever it's fine or your team might have a policy to say you know a review should shouldn't be you know unattended for more than two or three days or whatever again as a team we have to think about how do we avoid this problem of tell PRS people waiting for amused are not there obviously the the extremal or what may happen this might degrade in having too many PR so pending because you're waiting for someone here so you do more work in here and then as you're waiting for those in you create three four ten peers open and then you start suffering from having to context switch all the time right so this is a vacation again one more reason to try prioritize one of the things that I thought I notice as well and and and this is that this is an interesting for exercise about communication and what tools we used to communicate code reviews are great for communication about individual pieces of the code and maybe the sign of that small component but what happens when more and more people start and you have a ten people conversation within the PR within github or bitbucket those kinds of formats are not great for having a crowd discussion so you as the author have the responsibility to go actually maybe this is a conversation we have to do we need to have somewhere else bring people into a meeting with people into a hangout appear with someone some kind of communication that is outside of this asynchronous communication is great for some things but for these type of discussions it can be really hard so keep an eye for that keep an eye for it is this a conversation a discussion that needs to happen somewhere else and if it is if you do end up bringing people together make sure that the results of that conversation are reflected back on the PR and there's another thing that I really didn't think about much before Shopify and this is about sort of the post-mortem value of a PR people might go back to that thing go why was this created why was it done like this now that it maybe it breaks six months afterward after you murdered we want want the ability to go back and say well this is this is why it was chosen - this is why we chose it to do it this way okay so the value of reflecting offline conversations in the PR are so that others can get that context but also ourselves when going back in the future if we ever do understand what happened there and of course different teams have different sort of dynamics in terms of pair programming I find I wish we did we do more often in my case I do it when when I feel the conversation is not flowing well or when I'm not comfortable or sure about what I'm doing but I know other teams actually do pair programming a sort of like their their go-to daily routine so think about if you're not doing it as much is this something that could speed you up because always in-person communication is almost always you know higher bandwidth and higher plan that is an asynchronous okay so what about this artist of together building fast building the right thing build anything right building fast but how you know a couple there are a couple more things about how the team dynamics work with us I've mentioned the idea of labeling your PR properly so the person knows is this draft or already or more polished we have to think about depending on who jumps into the reel depending on who you've invited to the review we want to give them all the context they need to get comfortable reviewing them so your your description of your PR needs to be attuned to who's actually jumping in so for example if if this is my teammate we've been working on this feature for a couple weeks they know they have about the same context as I do maybe my description doesn't have to be all that detail but if I'm inviting other people from other teams or who don't have the same content as me I want to be sensitive to those I want to make sure that when they read my description they have they know where to start they have the picture that they need in order to offer quality feedback now who do you pick for your views and that's another question depending on how your team dynamics are you might have the one review body the one person you go to always for a review right and that's that's good or bad it has the disadvantage that you know you're you're not getting diversity of opinions in the work that you do okay so what are some of the things I think of when I'm when I try to decide who I'm inviting for this review well first of all obviously who has context on the thing that I'm building so you might already have spoken to you lead about this thing that you're working on them you know they're probably a good person to be in there but what about who has skills that are related to this so very personally and I had never worked with Python before I started Shopify so at the beginning for the first few months I was complete beginner so I knew there are a couple people I could tag that could help me really speed up my learning of the language right I knew how to use the basic tools but there are a whole bunch of idiomatic things in Vice that I hadn't learned yet so it was useful for me to think about I know this two or three people are really good at Python that can help me get better at quickly there's also who cares about the thing I'm building and some of these may overlap but they they won't necessarily right so if I'm building a system that is or a module or an API that is used externally well there's probably people on the other side of that that you'll probably want their opinion on and and lastly you know I went too fast there and lastly who should be learning about this again there might be in junior people on your team and you'll want them to be picking up context from that so who in your team should be learning about the thing you're building so this is so this is a quote I got from when when having conversation about code reviews with the person who used to beYOU might lead and out really just because I think it's really really well worded he said great companies great companies investing people for the long term great unbrushed code reviews are part of that we could prioritize landing in the short term and we will write the same forever what we can prioritize making you stronger contributor and all of your future contributions will be better and your career brighter and enlighten author should be delighted to have this attention there are lots of times where I was not delighted to have that attention but if I think about this in the long term I'm thinking well I'm getting better and the team is getting better as part of this so if we do all these things right we're encouraging a common understanding of the things that we're building right you're starting to sort of share that responsibility and I think this is actually a really really powerful thing and and some of you might relate to this so a very very very quick story so I'm originally from Venezuela a place that is not very safe to live in we're always looking over at your shoulder were you always being extremely careful about what you do where are you going to be taunted then I moved to Canada 13 years ago a place that's a lot safer a lot sort of nicer to live in I realized that after a couple of years of living in Canada I have lost that paranoia of the daily day-to-day life right well I actually noticed a Chava fide that after about a year of working there I didn't have the paranoia of breaking Masur anymore and I think in and you know for the most part it comes from this it comes from this idea that I'm actually sharing the responsibility of the thing that I'm building really early on it's not a thing that I put out there and now you know everything everyone has to own it after a third of code review I really felt the thing that I'm landing is no longer mine it belongs to at least one or two other people and I think that's really really powerful I think it was powerful for me so just to wrap up what can you do with some of these ideas if you've liked them well some of these things are actual really I think relatively easy to implement yourself and to start suggesting them in your team talk to your team especially to your lead or your manager you know I saw this at a conference I think these some of these are good ideas what do you think proposed one crazy idea if your team is open to innovation you say you know what if you start breaking things down you know it in smaller pieces and with that is a word just say Alejandro made me do it and we'll see how that goes okay so to summarize you know about five things that I think are the takeaways for you keep your work small your pieces easier to review share your work early drafts for feedback when you're a reviewer focus on the work not the person and this is about the language they use and the intent that you have or for actionable feedback that the other person can do something with and try to pick think about how to pick the right people when you're an author that's all I have that you're all code reviews superheroes I think we have a few a few minutes for question with you [Music] you

Channel: GOTO Conferences

Views: 20,001

Keywords: GOTO, GOTOcon, GOTO Conference, GOTO (Software Conference), Videos for Developers, Computer Science, Programming, GOTOber, GOTO Berlin, Alejandro Lujan, Shopify, Code Reviews, Developer Productivity

Id: ly86Wq_E18o

Length: 40min 15sec (2415 seconds)

Published: Wed Jan 22 2020

Reddit Comments

[u/mto96]

Check out this 40 minute talk from GOTO Berlin 2019 by Alejandro Lujan, Senior Data Platform Engineer. Check out the full talk abstract below:

Are you a bit terrified when merging to master? Do you loathe having to read hundreds of LOC to understand that PR you got tagged in? Do you polish and re-polish before asking for a review? Then you should come to my talk. We’ll talk about how your team can be awesome at reviewing code.

There is general consensus on the fact that code reviews are an important step for a successful development workflow. Most organizations undergo code reviews of some form.

However, it’s all too common to see reviewers that barely scratch the surface of the work being put forth, or that offer feedback that is unclear or hard to act upon. This robs the team from the opportunity to speed up learning, share knowledge and context, and from generally raising the quality bar on the software being built.

In this talk, I will share my story of encountering a new code review process when joining Shopify, and the surprising learnings I found - even as an experienced developer. I will discuss the tremendous gains individuals and teams can reap from a thoughtful, well structured review process. I will also share some practical techniques we use to help both the reviewers and the reviewees do a fantastic job at crafting solid deliverables.

What will the audience learn from this talk?
Attendees will take home a set of principles and pragmatic techniques for improving their Code Reviews and raising the quality bar on their organization.

Does it feature code examples and/or live coding?
There will be plenty of code samples, but no live coding.