Goldilocks and The Three Code Reviews

Video in TIB AV-Portal: Goldilocks and The Three Code Reviews

Formal Metadata

Goldilocks and The Three Code Reviews
Title of Series
Part Number
Number of Parts
CC Attribution - ShareAlike 3.0 Unported:
You are free to use, adapt and copy, distribute and transmit the work or content in adapted or unchanged form for any legal and non-commercial purpose as long as the work is attributed to the author in the manner specified by the author or licensor and the work or content is shared also in adapted form only under the conditions of this license.
Release Date

Content Metadata

Subject Area
Once upon a time, Goldilocks had a couple extra minutes to spare before morning standup. She logged into Github and saw that there were three pull requests waiting for her to review. We've probably all heard that peer code reviews can do wonders to a codebase. But not all type of code reviews are effective. Some of them seem to go on and on forever, while others pick at syntax and formatting but miss bugs. This talk explores what makes a strong code review and what makes a painful one. Join Goldilocks as she seeks to find a code review process that's neither too long nor too short, but just right!
Computer animation
Programmer (hardware) Word Group action Process (computing) Computer animation Inheritance (object-oriented programming) Control flow Water vapor Bit Staff (military) Machine code Error message
Software developer Multiplication sign Decision theory Feedback Machine code Mereology Rule of inference Programmer (hardware) Process (computing) Computer animation Different (Kate Ryan album) Formal grammar Thumbnail
Computer animation String (computer science) Software developer Machine code Relief
Computer animation Computer file
Computer animation Bit rate Software developer Right angle Machine code Position operator
Observational study Multiplication sign 1 (number) Checklist Mereology Rule of inference Graph coloring Theory Software bug Product (business) Frequency Bit rate Internet forum Core dump Authorization Software testing Logic gate Focus (optics) Software developer Constructor (object-oriented programming) Projective plane Machine code Lattice (order) Line (geometry) Demoscene Subject indexing Type theory Word Process (computing) Computer animation Software Order (biology) Formal grammar Cycle (graph theory) Reading (process)
Type theory Process (computing) Computer animation Bit rate Single-precision floating-point format Machine code Line (geometry) Error message Computer programming
Peer-to-peer Computer animation Software Different (Kate Ryan album) Software developer Planning Bit Lattice (order) Machine code Computer programming Scalability Theory
Axiom of choice Programmer (hardware) Process (computing) Computer animation Software developer Archaeological field survey Machine code Twitter
Computer animation Java applet Archaeological field survey Iteration Machine code Extension (kinesiology) Number
Scaling (geometry) Computer animation Software developer Statement (computer science) Machine code
Peer-to-peer Dependent and independent variables Goodness of fit Process (computing) Computer animation Software developer Archaeological field survey Software framework Data structure Machine code Formal language
Computer animation Personal digital assistant Average Machine code
Group action Dependent and independent variables Computer animation Software developer Archaeological field survey Machine code Food energy
Computer animation Weight Multiplication sign Feedback Machine code Social class
Context awareness Data management Process (computing) Computer animation Software developer Machine code Inequality (mathematics) Social class
Point (geometry) Dependent and independent variables Focus (optics) Computer file Software developer Multiplication sign Content (media) Machine code Semantics (computer science) Theory Mathematics Process (computing) Computer animation Data conversion Thermal conductivity
Point (geometry) Cellular automaton Real number Machine code Checklist Bookmark (World Wide Web) Software bug Mathematics Process (computing) Computer animation Order (biology) Video game Right angle Data conversion Physical system
Group action Context awareness Archaeological field survey Branch (computer science) Checklist Theory Information technology consulting Software bug Template (C++) Product (business) Formal language Software framework Error message Logic gate Metropolitan area network Collaborationism Projective plane Constructor (object-oriented programming) Shared memory Machine code Peer-to-peer Process (computing) Computer animation Flux Spacetime
Dependent and independent variables Computer animation State of matter Data conversion Machine code
this is the end and so on
and so forth and hello and my name and by the and end just to get a sense of the room and how many of you grew up hearing fairy tales when your kids nice OK called me too and I want to make sure that this topic that can be fun if you had this to him that that been um so when I favor fairy tales growing up I was the story of Goldilocks and the Three Bears and how many of you are familiar with that fairy tale cool I see a couple people who didn't raise their hands and so very very quickly Goldilocks and the Three Bears is it
set it's the very tell where a girl being Goldilocks those breaks into a house and it happens to be the house of 3 there is an the entire story is her basically like rummaging through all their stuff and using it as an but she lies down in their beds in the origin of the public bears words to hide among which reports to cold and it turns out the baby there's words like just write the whole story is her like trying to find a just right thing I which I had a little bit of research on Goldilocks and the Three Bears and it turns out the story that were the most familiar with is a was written in 1837 which is like a 150 year old story quite old and and looking back on it as an adult I think it's probably strange to teach children the break into the house of errors like I don't know why it was like a bunny rabbits the water or something more friendly there's not friendly but that's OK and but anyways in researching this of I got to thinking and it made me wonder what this fairy tale would be like if we did of modern-day retelling so that's what we're gonna do today and I'm but modern-day Goldilocks is over the whole breaking into people's houses there's houses of and using all the staff modern-day Goldilocks and she does something the different of modern-day Goldilocks is 0 rails develop where
because of course you should from so Goldilocks has started her 1st step jobs and it's super super exciting because she didn't used to be a programmer and she still amazed that people pay her to write code of this kind of unfathomable concept to her and so her 1st of job she loves what she does because she gets right amazing code she thinks it's amazing and and she gets to work with a really great group of people and and she's learning a lot what the as we all know things change very quickly and let the programmer and after about 6 months
and she's writing code but she doesn't really know if she's getting better at it she doesn't know building any technical chops so she starts talking to some of the other programmer friends and she finds out that a lot of other junior developers and are working in companies with formal code review processes the let's weird we don't really have that in my company and my company is pretty small and the usually if I want feedback I have to go with this for and have to talk to another as senior engineer and get him to look at my code and the more that she talks of the people she realizes that she doesn't have any written documentation of whether she's getting better not she doesn't have a code of process a company this is probably not a great thing and and after while she just to get really really frustrated by this so she decides maybe it's time for her to move on and Goldilocks goes in the job and she finds her 2nd job working in
rail and also angular about the difference story that she's having an amazing time at this company of part of the reason for that is because everybody has code reviews everybody reviews coding everybody is reviewed In fact on her so 1st week on the job she refused the to use code use like what we really it that's really cool like people think that my opinion matters and I can actually have opinions about technical decisions this is really excellent I this is also a very small company at the start up and so they basically have 1 rule of thumb as long as 1 person OK the code it can be merged the this works OK and code gets merged in the team
is really productive and should however Goldilocks doesn't know that there are some the drawbacks of this sometimes sometimes the code reuse kind of devolved into like long strings of comments and sometimes they're having discussions about half rockets and whether or not we should use them which she feels that maybe we shouldn't do encode reviews and sometimes the discussing but syntax of she goes ahead and she's like OK were enable everybody up because we shouldn't talk about these things this they're not perfect at the start so that's good and nothing terrible has happened
really until 1 day so Goldilocks is working on an amazing feature has been working on for 2 weeks and she finally creates this amazing has pull request and it gets OK by 2 other developers on the team and it gets merged and and she breathes a sigh of relief and she's like OK I'm gonna go get coffee other back she comes back 20 minutes later to find that there it is like problem the the In makes
a slight problem I mean like a fire but because all of staging has gone down from and reviews of what happened Goldilocks what did you do I don't know if you that that the review new has reviewed by a pull request I don't know what I did that goldilocks not to be deterred she's fight aspire she takes a look at her pull requests method for request and it turned out that she did some bad cop passed up and she has to files that name the same to control of files and 1 of them is blank and was was to be deleted I if have anything get deleted and now the entire actors has come crashing down his 2 conflicting files the the thing is nobody coppiced during review
self Oh no we thought we really had this coding thing down as a team not just getting we didn't were clearly doing something wrong and this gets Goldilocks thinking
OK there must be a better way of doing code is right rate so Goldilocks is
pretty audacious and she does with any developer would do in this position she does some research OK she does a lot of research and she starts with 1 of her favorite books code complete
the the this ancient tax dates back to the year 1993 and was written by Steve McConnell and she's actually reading the 2nd edition which came out in 2004 that's fine and but what she finds from the receiver McConnell's work and is that code reviews or something that Art new and there's actually a lot of theory behind them and so she decides to dive into the theory in order to try to understand what they're supposed to be in the 1st place so while reading his book she comes across 1 passage Steve McConnell rates 1 part of managing software
engineering is catching problems at the lowest value stage that is at the time at which the least investment has been made and problems cost the least to correct the OK that's cool she thinks to herself and I by this inferior this makes sense I'm on board she keeps reading and she finds that a lot of times code reviews are used as quality gates that is to say Gates to make sure that after periodic testing and periodic review only code that improves our product improves our projects and code base is what's getting allowed through the gates and nothing else OK but our quality gates the my teens quality did not they're not really functioning as gates that kind of feeling formalities maybe bag of get through color she keeps reading and she finds that there's something called the collective ownership of construction and that's kind of like the cost a work order is code reviews came from the studies have found that the collective ownership of construction has a lot of benefits 3 main ones the 1st is that are that's that's a crazy couple it up up and the 1st is that multiple sets of eyes on a code base always lead to better quality code another benefit is that if somebody leaves the team you have your buyers fight less impact because more people have seen the code so even if someone leaves someone else can pick it up and the 3rd is that those defect correction cycles the cycles of debugging and fixing what's wrong with the code the cycles happen a lot faster because it's not just that 1 person knows what the bug is it's not just that 1 person knows where to start many people have worked this code that's a great added benefits and it turns out that there are also 3 types of code reviews Stephen corals book talks about this index but the 3 types of code still arts scene in software development practices today in fact some of you might use them on your the 1st is something called inspections inspection that to date back to the a team of IBM which is led by a guy named Michael Fagan and he started doing the inspections on his team and when he saw them working so well internally he started sharing them with the rest of the community and when inspection is is kind of like a really intense code review process the focus is on the defect detection not correction this is really important because if you're correcting code as you go along you're not actually scanning for any possible defects a core part of what inspections part are having a checklist of what you're looking for and having rules for all the people involved so what this team at IIT and used to do was it would have at least 3 people at every code review a moderator the author of the code and the reviewer and all 3 of them knew what their roles and they all came into this meeting with the checklist knowing what they were going to look for in the Code review process the the most important part about this was that they would take any data anything that was working or wasn't going right and it would feed that back into the next code review process In other words they were perpetually iterating on their code review process in using inspections 60 per cent of defects can be caught and once in that use inspections regularly found that 20 to 30 % you were defects you're bugs were found per 1 thousand lines of code so they can work yeah but not everybody has our
necessarily that they wanna spend on on I 10 lines of code which leads to the 2nd type of code review shorter reviews so the idea here is
that even 1 line of code should be reviewed every single line of code should be reviewed and 1 team that was using that introduced sure reviews before they introduced this process they had a 55 per cent error rate in their programs after implementing short reviews that dropped 2 per cent that's just a 2 per cent error rate when you start looking at less than 5 months ago another team had 86 per cent correctness rate after they started implementing code short-code reviews regularly that went up to 99 . 6 per cent the the 3rd type of
code review that has been and used throughout different software development teams is something called a walk through and this is generally like a working meeting 30 to 60 minutes long and usually there's senior developer and you that supposed to be an exchange of ideas the senior developer will walk through and talk about what might happen if this program runs and try to look for any defects for any by this the junior developer it's meant to ask questions and push back on any methodologies a senior developer might be um suggesting and half what these are a little bit less helpful and as Goldilocks is reading about the she realized how this is exactly what I used to do my 1st company but this wasn't very formalized and we didn't really have a plan for me just kind of did it whenever but it is a plane the when she looks at
the pier sophistication the peer review sophistication scalable like sex appeal really disappointed because she realizes all the software teams that worked on as often as they then how you really think we ever really gone beyond the to honest am I the only 1 who feels this way well theory is
great but it's very different in practice and being a programmer Goldilocks has no choice but to try to debug her teams could review process in our modern tale of Goldilocks and as the developer she does with any well-meaning developer who doesn't really know where to start and she does what they would do just
our obviously some good things come from Twitter and she creates a survey and she's spread that come you out all of her colleagues and ask them to share it I'm hoping that she can get some sense of whether everybody else is having the same feeling of disappointment with their code reviews as well the the a little disclaimer here Goldilocks
is by no means a data scientist she probably could have done this survey a lot better but and 1 major ball with all of the data is that it's limited to whoever actually self-selected into the survey and and probably in future iterations she liked improve this but I it's a good start that serve somewhere what she finds is that of the around
500 or so people who answered and responded to the survey the majority of them worked in Java Ruby on Rails JavaScript OK so what do they think about code reviews this seems like a pretty diverse said and not the best but a decent number of people what do they think about how these she asked them to what extent do you
agree with this statement code reviews have made me personally a better developer everybody seemed to agree with that it the averaged about 8 . 6 on a scale of 10 interestingly she found that swift
developers found code reviews the most beneficial she's not sure what that means but Ruby developers we're a close 2nd they came in at about 9 . 2 on a scale of 1 to 10 and she also wondered if she was the only 1 who had to ask for code reviews in her in her previous experiences where everybody else also just had that same experience it turned out the
majority of developers who responded to a survey did have most of their pull request reviewed but you'll notice that around 10 per cent of respondents said that they always had to ask for review from their peers it wasn't something that was really land for formalized or structure they had to reach out and get the review from someone else and this was also the case across
languages this led Goldilocks to believe that a good code review process has nothing to do with what language you work in a war with what framework to use and it has everything to do with your team and how you carry out your she also
found that most people only needed to have 1 person review the code before it could be merged into the code base and that people spend
on average between 5 to 20 minutes reviewing a single-pole request the but they sometimes have to wait an hour or a day and maybe even a week in rare cases for their code to be reviewed and merged into the code base and it seemed like a lot of people were doing
reviews but based on their responses not happen many people were super thrilled with how they were doing 1 person wrote in to her survey and said we have 1 developer who blindly thumbs up every pull request and rarely leaves comments they're attempting to gain the rule of at least 2 approvals it's easy to tell because inside of 1 minute they've approved 2 to 3 Paul requests someone else wrote in and said the 2nd and 3rd reviewers are way more likely to rubber-stamp after seeing 1 other person's approval yeah and this got her thinking are just going through the actions of reviewing is it actually having an effect and for those people who did feel happy with their code review processes what was working what can we learn from their experiences and pull into our own team she thought well after reading 500 + responses she found that almost always what makes or breaks a code review comes down to 2 things the energy and substance
so energy what that really means is
who is doing the review and how much time are they spending on it she found that it's not just the act of doing a code review that matters it's equally about who is doing them and how much weight they carry on teams someone wrote in and said everyone on the team should receive equal review I feel like it's so common for senior people to get feedback because people assume they can do no wrong the they totally can and they might want feedback Junior people get nitpick to death and we should remember that people self-esteem is likely to be affected remember that they're being vulnerable this really sat with Goldilocks and she thought about it she found somebody else who wrote in that code reviews need to be acknowledged as a 1st class citizen in adapting and they need to be thought of as a legitimate activity that need time and effort overall the overarching
themes that she found were that big commits logical requests with no context need for a really poor code review experience the same went for unequal reviews if junior developers were getting reviewed and never doing the reviewing or if they were never getting to see with senior developers were working on they generally walked away from those teams feeling pretty disheartening an overall developers who felt that the code review process was lacking generally pointed to the fact that you there management for the team lead and or somebody at the at upper echelons could not justify code reviews get it being a 1st class citizen they were always treated less than important and not worth a short time OK but what
about substance well when we talk about substance what we mean is what
exactly is somebody doing while reviewing what are they saying and how are they doing even if everybody on the team is reviewing code what they're doing is a review really matters and Goldilocks found some interesting data which is she knows a lot of people were using the phrase in text she did a quick like minded file and found a that or 5 per cent of the people who responded actually referred to work associated the WordNet take with their code review process which probably still a lot someone wrote in
and said that you should review the code or not a person let tools handle styling changes for you so that you don't have to spend time with that another person wrote in and said if I ever find myself going back and forth on something with by comments Aldus came the other person and ask them to pair sometimes in my team it's just really hard to talk to someone and another person wrote in and said sometimes on my team there's trolling of code and we haven't implemented any kind of code of conduct there were some interesting things that came out of all of these responses when it came to substance which is that code review that devolved into tons and tons of comments that were not picky and focused on tiny details really should have probably been pulled out a conversation it probably means that we just need to talk to someone on your team you have a conversation or discussion with everyone else a huge huge great for developers is that style and syntax and up being major pain points encode reviews someone wrote in and said most of our code review is all turn into huge discussions about what to name a very it turns into 2 correction rather than defect detection as what could've should have been it's what they originally the theory behind them was to be rather than style and syntax being the focus its Content and Semantics were what people focused on developers ended up feeling like they're review process was actually helpful and making them better engineers that the biggest 1 no was code reviews that were very egotistical and not empathetic this goes for both the person doing the reviewing and the person being repeated as Goldilocks look through all of this data
she realized that she wasn't the only person to have been disappointed by could be processes in the past everybody else seemed to have some issue with it or another and she started thinking back to what she ready and and what she had founded her research in the days of yore i.e. like in the eighties and 25 issues ago and code reviews were yes they were a check list but they were iterative process these you were perpetually feeding data back into a code review system in order to figure out of what you were doing as a team was working or not and if it wasn't working we talked about it and you changed it the and this kind of became the crux of what the problem was she found the bug in her teens coffee process which was that they had stopped discussing it they had stopped sharing whether this was working on on stopped iterating on and they were reflecting on whether their code review workflow was efficient and impactful cell stories are great
and very the wonderful by at some point you have to like find the moral of the story bring it back to reality because story the stories the good news is that I told you this is a fairy tale but not actually a fairy tale because it's very much my story I am Goldilocks surprised laughter but anyway so how these days I work at either of those 2 companies and I work at a company called till and we build skylight favorite rails profile and and because this isn't the fairy tale it's real life the truth is AT Archon of we have our review process right but way I do know is that we're still talking about we have conversations about & every engineer on our team feels like bacon bringing up in conversation actually made little changes to try to make the process better for them and for us as a team so if I've learned
anything from this whole project of trying to understand what code reviews are and trying to learn from my peers about it it's these 5 things 1st 2 error is human and we are all human so we should review everyone and everyone should be reviewed 2nd it really really helps your teammates if you can be concise and give context a couple of my colleagues that talked to have actually started using screenshots in their pull requests and detailing out what the what the behavior was before the pull request and what it should be after and if it's above showing your teammates how to check it out the check out a branch and actually like to see the bug in action and see what you're changed it to change to a compact code the 3rd is being kind I think there's really no better way of doing that then being able to leave your ego behind the door and realize that this is the collective collaboration to construct something that's bigger than all of us for as a team the this is really hard to do and I think it really depends on each team individually make it easier on yourself use Lintas do not talk about tabs versus spaces because man that is going to get under somebody's implement winters and use and templates of possible and get have has a great feature where you can have a template when you open a full review pull request and it will actually like go through and make a checklist for you and you know exactly what should be an apple question what's missing there really awesome I encourage all the to check it out and then if there is nothing else you remember from this talk I hope that this iterate it's so important for say again paper territory and what I mean by this is talk about your code review process and perpetually keep that in flux because it's not a fairy tale this story still going you can still make it better if it's not where you want to be and if it's good you can improve upon and share with other people and other teams and in my favorite quote from all the people who
wrote in that's my survey was this I love code reviews in theory in practice they are only as good as the group that's responsible for conducting them in the right manner it doesn't really matter what language you use what framework for how big your team is or whether you're a consultancy or product team it's on you as a team because all of you are adding to this yeah amazing creation together that's on you to make sure that your quality gates are actually a good quality
it so I hope that you read today if you haven't started this conversation wanting to started with 13 end if you feel I could've used are working for you talk about it and share your ideas and this data with them in fact if you want
to check out the state data you can visit better code . reviews all of the data and I did a scientist but all the data they could manage to collect is up there and and I did read a couple of people's responses there are tons and tons and tons of them they're really good and I encourage you to check them out of the way more than I could fit into the development and and if you'd like to continue the conversation and you can add to the survey and if your data scientist maybe you can help me the data and and you can contribute by bring us back to your team and starting conversation because as much as fairy tales are great this 1 is just beginning of all have different thank that so if
if you are