We're sorry but this page doesn't work properly without JavaScript enabled. Please enable it to continue.
Feedback

The art of giving and receiving code reviews

00:00

Formal Metadata

Title
The art of giving and receiving code reviews
Title of Series
Number of Parts
60
Author
Contributors
License
CC Attribution 3.0 Germany:
You are free to use, adapt and copy, distribute and transmit the work or content in adapted or unchanged form for any legal purpose as long as the work is attributed to the author in the manner specified by the author or licensor.
Identifiers
Publisher
Release Date
Language

Content Metadata

Subject Area
Genre
Abstract
Code reviews are an invaluable tool for improving code quality - not just by catching bugs, but also by improving readability, scalability and maintainability of code. But code reviews are challenging to do effectively, for reasons of personal psychology as well as organisational structure and culture. This talk will present tactics for constructive code review drawing on conflict resolution theory and lessons from the software industry, and discuss the particular challenges of applying these in a research setting.
6
Thumbnail
15:56
53
Thumbnail
22:03
Mathematical analysisMachine codeMathematical analysisSoftwareQuicksortPrisoner's dilemmaFeedbackMachine codeXMLComputer animationLecture/Conference
Self-organizationComa BerenicesComputer programLocal GroupObservational studyAverageSoftware maintenanceMachine codeError messageLine (geometry)Software testingAutomationComplete metric spaceDivisorScalabilityStandard deviationSet (mathematics)Population densityComputer programmingPlanningVector potentialTelecommunicationPoint (geometry)Integrated development environmentLibrary (computing)Peer-to-peerVapor barrierTelecommunicationComputer architectureScalabilityMachine codePlanningComplete metric spaceError messageLine (geometry)CollaborationismObservational studyOrder of magnitudeMathematicsVector potentialQuicksortLevel (video gaming)Pattern languageFormal languageGraph (mathematics)Bus (computing)Software developerSelf-organizationSoftwareMultiplication signWordUniversal product codeMilitary baseProcess (computing)Single-precision floating-point formatFunctional (mathematics)Computer programmingBitFlow separationNumberSoftware testingIdeal (ethics)Rule of inferenceExecution unitOnline helpShared memoryQuantificationGroup actionType theorySound effectDivisorSoftware maintenanceStandard deviationConvolutional codeTerm (mathematics)Set (mathematics)Bit rateProxy serverPopulation densityDegree (graph theory)PlastikkarteUtility softwareReduction of orderMathematical analysisPatch (Unix)View (database)MereologyCASE <Informatik>Limit of a sequenceBoss CorporationProjective planeDiagram
Machine codeLibrary (computing)TelecommunicationIntegrated development environmentType theoryLecture/Conference
Inclusion mapLink (knot theory)Rule of inferenceMachine codeLogicInformation securityAxiom of choiceRevision controlSoftware design patternCollaborationismData modelDuality (mathematics)Conflict (process)GradientFunction (mathematics)Library (computing)Code refactoringFeedbackLatent heatCartesian coordinate systemProof theoryFormal languageVector potentialChecklistFitness functionPoint (geometry)AreaType theoryImage resolutionMachine codeGraph (mathematics)AuthorizationProcess (computing)Axiom of choiceRevision controlProjective planeExtension (kinesiology)Wave packetOnline helpCuboidMultiplication signGradientCollaborationismFormal grammarSpectrum (functional analysis)Software bugMilitary baseContinuous integrationDegree (graph theory)Descriptive statisticsBlogScalabilityGame controllerLatent heatFeedbackTwitter1 (number)Software developerBitLevel (video gaming)Computer programmingFlagArchaeological field surveyFunctional (mathematics)Software design patternMereologyPosition operatorSoftwareRight angleImplementationPlastikkartePerfect groupCore dumpConfidence intervalMathematicsTube (container)Order (biology)Dependent and independent variablesCommunications protocolStaff (military)Asynchronous Transfer ModeSound effectGroup actionSpacetimeDiagramProgram flowchart
Machine codeMereologyPlotterSocial classSoftwareMultiplication signObservational studyAuthorizationMathematicsRight angleCode refactoringCASE <Informatik>Limit (category theory)Point (geometry)Context awarenessQuicksortChannel capacityProduct (business)Line (geometry)Set (mathematics)DampingPairwise comparisonPlastikkarteAreaForm (programming)Video gameDivisorLecture/Conference
Transcript: English(auto-generated)
Hi, yeah, my name is Alex. I'm an Imperial in the global infectious disease analysis center And I work in a small team of RSEs called Reside. That's research software for infectious disease epidemiology Just before I get started could I see by a show of hands who here is regularly reviewing code and having their code reviewed Okay, awesome lots of people so I'm hoping for lots of feedback from you guys
Whether my experiences map onto yours any sort of questions or thoughts that you have about the ideas that I've got to present So first of all some motivation for doing code review Although it looks like I don't need to sell it too hard to this audience. So code review is obviously great for defect finding These are just a few illustrative examples taken from code complete from by Steve McConnell
Just to give an idea of the magnitude of the effects of code reviewing on defect finding So one example saw a change from 50 55% of one line changes being an error down to a 2% rate of changes being an error after code reviews were introduced
Another study saw errors reduced from 4.5 errors per hundred lines of code to 0.82 errors per hundred lines of code and Final example another study showed a 90% decrease in defects so all of these all of these were served to illustrate the idea that Code reviews are very good at finding defects And of course automated tests are also good for finding defects and code reviews are in no way a replacement for that
But they are a complement to it and especially for the RSE community I know that there were several talks about tech the challenges of testing scientific software in the previous session So I just just just to make an additional note that these that code review is especially useful when automated tests are perhaps
Perhaps of limited value or you're still struggling to implement meaningful automated tests across all of your code because all code can be reviewed by a human regardless of how amenable it is to automated testing But moving on to some less quantifiable benefits of code review one thing of course is learning opportunities, so This can happen at any stage of your career
whether you're having your code reviewed and you're getting useful comments back on it or you're reviewing someone else's code and picking up on patterns that you didn't know about or perhaps you're Reviewing code in a language that you haven't been using for a long time and you discover new functions and new libraries the learning opportunities of code reviews are really important and Then this third point sustainability, which we've heard a bit about today already
And I think it's really important to the RSE community Code reviews I would say absolutely essential for making sure that code bases are sustainable for several reasons Obviously, they increase the bus factor in other words. How many people are responsible for this project? Is it is it a sort of a code base that's owned by a single person?
Well having someone else reviewing that code ensures that at least two people hopefully many more Have eyes on that code and have some familiarity with that code base Code reviews ensure that code is readable. Someone else has to be able to read and understand that code So again, this speaks to the transparency point that was talked about in Alice's keynote this morning and performance and scalability
Thinking about the future of the codes. Well Humans are very good at picking up on these things in perhaps a way that automated tests are not so thinking about what is The future of this code base is this code that could be extended is this code that? will perform well under different conditions and for just general maintenance of code standards of
enforcement of best practice and code architecture patterns within the team So some practical advice One golden rule I think of pull requests is to keep change sets small small enough that they can be meaningfully reviewed This is a graph taken from a study done by IBM and this one this this is looking specifically at defect density
So the number of defects that were found per lines of code under review and found that they drop off quite significantly after Well, certainly after 400 lines of code, but there's sort of declining Returns earlier than that. So the rule that they drew from this is keep change that's under 400 lines of code I'd say this is true not only for defects, but also those less quantifiable things. We were talking about
sustainability readability things like that and The lines of code things is perhaps just a proxy for change set size. So as well as having a small Number of changes really the poor request should be conceptually distinct So you want reviewers to be looking at something that is a small unit of work that can easily be understood and digested and meaningfully
reviewed Relatedly this is a graph about pace. So don't review too much too quickly. So IBM also found that 500 lines of code per hour was a sort of upper bound for how fast you should be reviewing things and
And relatedly, I mean this really comes back to the previous point about change set size. So keeping poor requests small, but also Also review fatigue, so you can't spend too long reviewing code in a single session So maybe an hour is probably a reasonable upper bound for that as well We do in my group spend a significant portion of our time. We allocate a significant portion of our time to review and code
I'd say maybe as much as 15% of my time goes on reviewing code a day But certainly not for more than an hour at a time Okay, so a little bit about a culture which enables productive code review I think in an ideal environment and we're talking, you know, this is the kind of environment
You'd maybe see if you're working for a big software organization You would have no siloed code bases in other words No code base that's isolated with one person who's primarily responsible for that code base. You'd have developers moving between code bases Without too much specialization and too much ownership of a single code base You would have a culture review in which
Review is enforced for every line of code every line of code is under review and every person has to engage in the code review process as well and Maybe you'd have much Many other collaborative practices within the team as well Perhaps doing a lot of pair programming or at the very least like collaborative planning of issues that are going to be implemented
All of these things make reviewing much easier So this is an environment in which reviewers can conduct really meaningfully meaningful reviews of their peers work But I know that in the RSC community there might be some practical Barriers to achieving this kind of collaborative workflow Not least if you are working within embedded within a research team rather than embedded within an RSC team
So I have a few ideas for sort of practical moves towards this type of culture So one thing is just making sure that channels of communication are open So even if you're working by yourself Perhaps you have access to a wider RSC community or other people working at least in the same language as you within your department
Or within your group. So keeping open those lines of communication at least establishes opportunities for people to Review each other's code and maybe some kind of quid pro quo arrangement where you'll Be prepared to review someone else's code if they'll be prepared to review yours Even if you have quite high levels of ownership and some degree of siloing of those code bases
So this is my second point as well that everyone is a potential reviewer and everyone should be prepared to review unfamiliar code So ideally a reviewer is well versed in the code base that they're reviewing or at the very least Well versed in the language of the code
But minimally I think any review is better than no review and certainly within our group in the absence of someone who? Meets those standards to review code actually having a review from Someone else in the group who has some level of familiarity with the language you're coding in has still been really really useful in terms of finding defects in terms of improving architecture and actually improving knowledge as well and
Sort of knowledge sharing within the group so that people are skilled up to then Be able to really contribute really meaningfully to other people's code and offer helpful suggestions Another thing we've actually done recently has started having inviting anyone in the department anyone who's writing code To come to a weekly stand-up with us so that again is just another channel of communication where someone can say oh
I'm writing this library in R But no one else has looked at it except me and perhaps someone can volunteer to at least do some kind of cursory review of that code this I think Comes in particularly when you're talking about these types of environments where reviewers might not be
Extremely familiar with the code bases that they're reviewing you can help reviewers know what to look for so at the very least an author should always annotate their pull request write a Description of exactly what the code is trying to achieve and also review your own code first so go through your pull request You can annotate where you would particularly like attention. This is going to be really helpful if the reviewer
Yeah, maybe isn't a specialist in this in this particular area You can direct the reviewers attention to things that you think maybe you're not sure of maybe you'd like a second opinion Of and we've also had some success with checklists So in github you can actually create pull request templates, which makes this easier
so predefined checklists for certain projects just to make sure that reviewers are ticking the boxes quite literally and Looking at some of the key things that you want to be picked up during code review And these can obviously be tailored to the individual projects Okay, so those were some practicalities and
I'd like to now move on to the subject of the psychology of code reviews I mean to some extent everything I've been talking about is the psychology of code reviews, but what I mean is a code review is Essentially a critique of someone's work and if someone is invested in their work Which I assume most of us are that's necessarily going to feel personal to them
We had a quote actually the previous session someone said software is easy But people are hard and this really speaks to the people side of code reviews So the actual dialogue that takes place when reviewing code In a highly scientific and representative survey of my Twitter followers I was just curious to ask people how often they feel defensive when they're having their own code reviewed and
Yeah, I mean, I suspect that everyone is familiar with basically some feelings of defensiveness when their work is under critique I think that's just normal. That's just human. So bearing that in mind I would say that not all code review comments are created equal
This is a two axes a low reward to high reward axis and a low conflict to high conflict potential axis And I think it can be helpful to think of when you're making a comment on a review Where does the comment fit in this broad?
This brought this broad graph about this top left quadrant the high conflict low reward These are things that I think could be characterized as pedantry or at least perceived as pedantic by the by the author so typos whitespace
Arbitrary preferences where there are two equally good ways of doing something and I don't want to spend too much time on this quadrant But other than to flag up the fact that there are tools that can automate away this type of conflict, right? So you have style guidelines and you have linters You can run all of your code through a linter through a spell checker as part of your continuous integration process
so that by the time the review lands on the reviewers desk all of these types of things that can be perceived as pedantic by the author they will have caught themselves and they and they will have Have corrected in the bottom quadrant this the bottom two quadrants the ones I've characterized as low conflict
These are things that I think could be broadly described as factual. So somewhat comes back to the idea of defects or bugs in the code and These are obviously on a spectrum of low reward to high reward Some things are very minor and some things are very critical But I would say broadly these are going to be fairly low conflict Precisely because the author doesn't feel a high degree of ownership over these things, right? These were accidents
These were oversights. These weren't things that the author identifies with strongly and that leaves us with this top quadrant This is the quadrant that I think is where code reviews really come into their own right? This is things like Is this code over engineered or is it just the perfect level of abstraction?
What design pattern choices have been made here is this code really readable or what trade-offs have been made readability versus performance say and These I think speak back to that point that I made at the beginning about code reviews being great for code sustainability
Because these are the types of things that really play into the transparency the performance the scalability the future proofing of the code and I think these are things about which To qualified developers can reasonably disagree and Could be really productive to collaborate and and talk about these things But how do we induce people to collaborate? I?
Find it useful to know a little bit about conflict resolution Archetypes because I think these map quite neatly onto ways in which I've seen code reviews be dysfunctional so one way in which code reviews can fail to be highly useful and highly functional is if they turn into hotbeds of competition and Aggressive conflict and I think if you're seeing if you're seeing something like that
it could be that one or some of the participants involved have a competing style of conflict resolution On the other end of the spectrum There are poor quest processes code review processes that really seem just like a formality Where code is technically being reviewed, but no comments are coming back on that code or you have people who just opt out of the process
They don't really want to put their code up for review or perhaps they don't want to be a reviewer and I think this is where we're seeing people who are perhaps avoidant or Yielding when faced with a conflict and what we really want to do is encourage people to collaborate Common wisdom on this is there are two modes for for moving people towards that collaboration quadrant one is to lower defensiveness and
One is to raise confidence So with those two things in mind Some tips for language mainly mainly language and approach that you can take if you are the reviewer On someone's code one idea is that you can raise the code
Raise the grade of the code if you think of code like a school assignment and you think of it Graded A to F you can raise code by a grade or two, but don't go overboard So you can make a C into a B plus or maybe a B into an A But if you're trying to drag code, that's like a D or an E up to an A
That's going to be a very painful process for everyone involved, especially the author There might be other Ways to address low-quality code that are not code reviews, right? So extra training or mentoring or pair programming or what have you but just a deluge of code review comments is not going to be helpful Language choices so using we instead of you it's an incredibly simple trick, but it really works. So, you know, it's much less
Yeah, it fosters the idea that this is a collaborative effort, which it is right these code bases are collaborative efforts Or they should be and it shouldn't be accusative So a couple of examples, you know, you should have reused this function can easily become we could reuse this function
Which also feeds into the third point which is there's no need to phrase things Too directly either you could also ask things as questions. There's a couple of reasons for this I mean one is just that people respond better often to questions and demands But the other thing is you actually might be wrong as the reviewer right like you might have overlooked something So it's good to be circumspect
So ask, you know, could we reuse this other function here or you know, how does this change? How is this changed from the previous implementation? What are the advantages of this approach and Of course give some positive feedback It's so simple But everyone responds well to positive feedback and it's really easy to find something good to say about code at the very least
You can just say thank you to someone for their hard work But often there's something much more constructive and substantive to point out that's positive about the code Onto being an author. So obviously you don't have control over a reviewer's comments But you still have control over how you respond as an author one technique that I sometimes practice
That's a little bit challenging. It requires some self-control, but it's quite useful is the as-if technique So you try to ask yourself if you find yourself responding negatively or defensively to a comment you ask yourself Well, how would I respond if? Something were different about this situation perhaps. How would I respond if this was I was in a better mood
I was having a better day, or how would I respond if actually this comment was made on someone else's code? And I wasn't the author here How would I respond if that if this had been phrased more sensitively if perhaps it hasn't? Another really simple thing say thank you to the reviewer. This builds a good relationship to an author and reviewer It's actually an implementation of the first technique because by practicing gratitude or just behaving in a way that shows gratitude
you actually start to feel more gratitude and Make sure you annotate your review first It's a really simple practical points, but if you go through and explain your choices You're much like more likely to have a constructive dialogue with the reviewer And you can also solicit feedback from a fairly unresponsive review if you if you're having
Trouble getting out of the reviews what you want you can solicit feedback with specific questions And actually directing the reviewers attention to what you want Okay, I'm gonna wrap up and ask for questions and feedback from the audience You can reach me on Twitter And I have a blog post which goes into some of these ideas at more depth which is linked from my Twitter
we also have a blog at reside in which we've Blogged about our pull request protocol, so if anyone's interested in that I'll be sure to tweet it out later and with that I'll open to the floor. Thanks
Are there any questions at the back?
I think it so the question for those listening was what do you do if you actually get code that could? Reasonably be graded a D. Is that acceptable and what do you do? Do you just throw that back and say sorry? We can't merge this. I think it depends on the context right if this is something that's going into production
And it's just subpar then yeah, you're not going to be able to to merge that code if it's I Guess it depends what you mean by D as well right so I presume you mean what what do you do if you? Have really subpar code that can't be merged into production. Yeah, I think unfortunately I really think the code review is not the place to address that I think
There may be parts of the code that could be factored out and could be acceptable So that's a great thing to suggest if you think that some part of it was Was acceptable you could suggest that that be factored out and that could be reviewed and go in and that's a small win for the Author that will make them feel better about the rest of it aside from that I think you'll just have to establish whether it was because it because of a
misunderstanding of the requirements in which case maybe the author could just go away and have another go or if it there's really a deficit in Capacity there. I think yeah, you have to start again and probably pair with that person is going to be the right thing at that point Thanks. There's another question here
So in the beginning you showed those plots with the effect on on the defects in the code Do you know how those defects are measured? Actually, I haven't dug really deep into that IBM study, sorry, I can't tell you yeah Although I would be interested to and there's a lot more studies like that and I I know different institutions
Mean something slightly different by defect. So like the sort of comparison between those studies I think would be really interesting to do and I haven't had time to really to really dig into that too much. Yeah chance for another question
So the question was when software is undergoing a really substantial refactor how to keep to this limit of 400 lines of code per change I Mean, that's a huge subject I think that's like there's room for a whole other talk just on how to factor out small change sets
But I think it's I think it's almost always possible. It's my short really short answer There's many ways to slice up a code base like, you know Horizontally vertically can use code switches so that you're you're making a small change, but it's not getting released into production You're starting from scratch with a yeah with a new class while keeping the old one side by side
And using some kind of switch to switch them out So there are a lot of techniques out there for refactoring and I'll admit that there's some more upfront Time and work into making those change that small but I'm a big proponent of that being Absolutely the way to do it and it being a false economy to think that making a large change at once is the way
to go