Building a code review culture – Emil Hornung

okay hello everyone and my name is and I am very pleased that she decided to join this present this presentation especially as this is quite late today my talk is titled building ecology of the culture and during which I would like share review my experience and knowledge about the process of code review at the very beginning let me quickly introduce myself and give you a short context of this presentation so I've been a Java developer since 2009 and for all these 10 years I was creating developing and maintaining web applications first as a full-stack developer and lately as a back-end developer I've also been a technical or team leader in different companies and different teams and currently I'm working at J labs which is 100% Polish company we've had Quattro's here in Krakow but also offices in Warsaw when I am working and also in Germany in Minich my experience is also in enterprise project so I won't be talking how it looks in open source projects and also it might be slightly back and rated but even if you are full stack developer front-end developer or mobile developer I think you will gain a lot from this presentation okay maybe let's start with a quick few questions to you to make sure you are still not asleep or sober enough so could you please raise your hand who have you'd ask cold reviews okay I think it's at least half and who have you liked doing code reviews actually could you please raise your hand okay that's like a 1/3 I think that's nice and who of you doesn't like it really could you please raise your hand yeah there are still some people there ok so so my observation was that followed a lot of us code review it's unpleasant thing to do definitely not the best part of our day I think we rather prefer to write our own code that review or read someone else's code and also I think that many of us do code reviews kind of spontaneously without any structure without a deeper thought and so my reason for this talk would be to share with you what we can do to make code reviews more pleasant for us and that's more effective so quick agenda I will start with what code review is and what it is not then why we shall even do code review followed by how to prepare good pull requests then how to review code in particular how often thinks about checklists and psychological aspects then few things we shouldn't do so don't and disadvantages and costs and how to address them and we'll end up with that with a summary ok so let's get started I think all of us here know what code review is actually but let me clarify on one thing because when we go to Wikipedia we can see the definition that says code review is a systematic examination of computer source code and actually I don't really like this definition mainly because of this word examination so I would like to I prefer to use a definition that says that code review is a systematic discussion between two or more developers about changes to the code okay now what code review is not what I would like to emphasize here so number one would be that that code review is not a way to measure code quality or pointing mistakes what I mean here is that we should have fought this attitude that when we receive more comments it means that our code is worse it doesn't arrive like that like that actually the more comments we have only means that the pros of code review is effective so we should rather be happy because of that another thing I would like to mention here is that code review is not an explicit Quality Assurance method this should be done done by tests and this is something like Uncle Bob mentions in one of his book that quality assurance should find nothing I think it should be the same with with a code review we should deliver production early code that we know that we did our best and we know that it works and not challenge our colleagues to find bugs there okay eventually they will find some issues but there's not their role and it's not the role of that of the code review itself okay so before we will start talking about how to optimize the process let's think about why we should even do this okay so for that reason let me present to you results of our research conducted in 2013 and so the research had two parts and the first part was a survey where authors asked developers about what are their motivation for doing code review and so they can answer give three answers so what's my top motivation my second motivation and my third motivation and so we can see the results based on more than thousand answers and you can see that a number one top motivation for doing code review is finding defects then called improvement then alternative solutions knowledge transfer and so on and so forth and so there was the second part of this research and it was about analyzing actual code review comments so the autos took the code review comments and try to assign to one of this category to see what are the real categories of this of these comments and so you can see the results on the right right hand side and you can notice that number one is the cold improvements number two is understanding number three is social communication and number four is defects all right so this concludes us to the summary of this of this research that says that our study reveals that while finding defects remains the main motivation for review reviews are less about defects than expected and instead provide additional benefits such as knowledge transfer increased team awareness and creation of alternative solutions to problems there was another research on code reviews conducted in 2014 which has a very similar conclusion they say that MCR which is modern code reviews process seems to be generally more valuable for improving mayn't ability than for fixing defects for example model code reviews might be recommended for software systems that require high mint ability such as long-lived software systems and I don't know what is your experience what in what kind of projects you are working or worked on but I have never been given a project that was about to end in a free months it always was planned for for much much longer and so this is the place where you should really invest in the process of code review so let's sum up all the motivations for doing code review as I already mentioned we shall do code review to initiate discussion about the code to promote communication in general in our team to share knowledge about the project itself so to increase so called buzz factors or the number of people who possess a knowledge that is needed for project to be continued and also for sharing knowledge about the program in the programming language techniques frameworks principles we follow and so on another very interesting motivation and attribute of code review is that it spread code ownership so instead of having my and yours code we more getting ours code what I mean here is that we trying to avoid situation when there is a back on production and somebody says okay I have no idea how to fix it it was John's code I haven't seen it ever so only John can can I can fix the back for example yeah so we're trying to avoid the situation code review another thing that we do called you before is to learn from others and also to teach others very effective way to develop our skills actually of course this all leads to improve general quality of the code and it leads to it decreases number of defects in our code but this is some kind of a side a side effect of a whole of the whole process okay so and now let's talk about how we can make this process more effective and actually code review doesn't start when we are starting a review of the pull requests it actually starts when we are creating the pull requests so what we can do to prepare good pull requests so the first thing I would like to mention here which is the most important actually is to make sure that that your peer is small enough and we can have a different measurement of of the size of the peer but one of them and most obvious is numbers of lines of code and so here you can see the graph from the book called best kept secrets of peer code review published by smart bear in 2013 and you can see the relationship between the lines of code under review and the defect density and say okay you can see that the defect density decreases with the number of lines of code and after more or less 400 it's almost zero so I don't want you to treat this for hundred lines of code like a very precise number because it will differ depending on what languages you are using and what kind of application you are working on but I would like to encourage you to find this number in your in your project and try to not not to exceed these lines of not these numbers of lines of codes in in a single pole request also there was a great tweet about it some time ago that says ask a programmer to review ten lines of code and he will find ten issues ask him to do five hundred lines of code and he will say oh it looks good and I think you you might be familiar with that and this is this is how it really works and one thing to mention here is maybe we shouldn't be – shouldn't be happy – too fast too soon if we are getting no comments on our review because maybe it was so big that somebody didn't even look into it okay so another measurement of the size of the PR can be how many issue addresses and I would like to encourage you to to be sure that the peer you are creating address is not not more than one issue okay so it's a little bit like single responsibility principle on level of pull request so as an outer I should ask myself a question does this PR really address punish only or maybe I can divide it somehow in particular refactoring should be done in a separate pull request I really think that we should do refactoring as as we encounter some problems not as a separate task but it would be very beneficial if we put it on the separate pier and then go back to our main work another example would be that main logic and the glue code might be in separate piers what I mean here is that we and we are creating a new feature so for example we are implementing a new component we can create a pair for that and then track plug in all the new code into the existing one with the separate peer another idea to make peer small would be to create a pull request to a feature branch instead of a master because for some reasons we don't want to merge into master only part of our work so every other piece would go to this feature branch and then afterwards we can have a one big peer to the master but we shouldn't it is not we don't need to review it actually because those reviews previously we can just make some sanity check okay and if we for some users don't want to use this for those feature branches and this kind of division maybe we can at least divide at our our peer into separate comments so the reviewer can is not forced to review peer all at once but coming by commit it might be also very helpful okay so another important think when creating a pull request would be to give a context a short description a reason for the change okay sometimes JIRA link will be enough sometimes not it bends how good is our your tasking your in whatever issue tracker you use generally put yourself in reviewers shoes and ask yourself a question what somebody else has to know to do a proper review okay and and very quite interesting thing here is that only preparing a description will make you rethink your job and can lead to some changes and this is actually the fastest feedback loop you can get from yourself so it's also very beneficial another thing to remember as a as an as a peer outer would be to put all team members as reviewers to promote knowledge sharing in particular muting members should always be assigned to the pool requests and also especially junior developers okay they have to learn somehow about our project and about our techniques so they should be able to see all their peers that are in the project I'm not claiming that they have to have right to approve or decline our pool request but we can also tag this option off and just let them review and ask the question and actually talk about the code let's think in this part the last device from here would be reviewer code before creating a pull request and there are also studies that shows that 10 to 22 percent of changes are self-motivated so are done by ourselves so again a very beneficial very short feedback loop we can have from ourselves all right now let's get to the second part so how to review code actually okay so the most important thing here would be that the context is the king we should know the reason for the change because same change can be good or bad depending on the context it's like with domain-driven design at the same term in different context has actually a different meaning in java word for example if somebody writes synchronized code is it bad or good it depends because if we will be using this code in a single favorite environment then it's an overhead yeah if we will be using it in the motivated environment then it's actually really needed so we need to know the context next thing as a reviewer would be make sure that the PR is small enough and if we see that we are overwhelmed by the by the PR is big is too big for us we should immediately go back to to the outer and politely ask him to divide it probably also with some suggestion how we can divide it because sometimes it's not an easy task but we shouldn't let ourself dragged in by the big pull request it very kills productivity very quickly and so another thing as a reviewer would be to find the best place to start because our tools like github crucible and whatever you use shows changes in alphabetical order yeah and so I compared it to reading a book but not in the order of chapters that are that are appearing there but sorting them alphabetically and then reading in that order it doesn't make sense so my advice here I am very fond of test-driven design and and tests in general so I'm seeking for a test that describes this change and starting from the test and then go to the to the code production code that fulfills the test and and go this way another topic would be about the pace so first of all we shouldn't rush too much what I mean here is to avoid a situation and someone is telling us ok hurry up just I prove my PR everything what has to be done is done you're actually blocking my my work yeah we are waiting for the review which is useless we shouldn't take this edited I would say that code review is more like sightseeing than rushing through a highway because when you are going on a highway our goal is to go from point A to point B as quick as possible and that's not our goal in the during the code review it's not about giving a proof without winning a code so it's more like sightseeing so we want to get acquainted with this code see see some details ask some questions and so the more attention you will pay the more details you will see and this is what we care about actually during the code review natural thing would be to go through all changes more than once ok and also if you you have a one or two or three liner pull request I really encourage you to try to take your stopwatch and spend at least three minutes for this kind of pull request and see how many actually questions you can ask and how many details you can see in there it is a very interesting experience actually and also if we will keep in mind that reviewing someone else's code is more like an opportunity to learn something then just not pass on duty we will not be having this eager to rush so let's keep it in mind on the other hand we shouldn't spend too much time and again this is the graph taken from the best-kept secrets of peer code review book where you can see the relationship between the average number of defects found the a number of time spent in on the on the code review and so we can see the more time spent in code review the more defects or comments you will find but after around 60 minutes there is no actual raise of this number okay and this is because how our brains work so we cannot actually focus more on this kind of activity which is kind of a passive activity if you can say that so we just lose our focus after the 60 minutes and so there is no really reason spending more time if you stick to the rule to have small peer 60 minutes will be really really a lot but sometimes you will encounter a big PR that you want to deal with and if you see that you are spending portent went sorry 60 minutes just give it a break for like a twenty minutes and then go back to this to the spear to be more productive okay so about reviewing code it is better to really code by yourself than having a walkthrough by the outer and why is that this is because if you ask somebody to present his or her code because you don't want to read all the things you will be listening to to the outer and nodding your head in agreement but actually after some time like a few days or weeks or months when you are you will have to work with this code like fix some back or use it or whatever you will see that okay I don't know what's going on here and wise why is it written this way and you will be having a lot of questions okay so it is better to go for your food the code by yourself then if a code needs some explanation during the code review and you are not sure if it's you missing some knowledge about the project or or the technology the most natural thing or maybe it you're missing some some some other other knowledge so the most natural thing here might be to go to the outer and again ask about clarification but it would be a similar situation like mentioned before it is better to ask another colleague about this code and so if he or she knows and understands the code then it means okay I'm missing something here but if another person also doesn't really understand it then it means okay we can write this code better okay so let's let's do some changes here and very important a very important thing to mention is that when we are asking the outer for some explanation let's change it into code actually because this is our source of true we will not be going back to our pull request to see clarifications we want everything to be in code and so we should do some do it through through refactoring or if we if it can be done by refactoring let's explain all this tricky things through our tests okay well another thing a little bit from my autopsy is that once nothing is written in another way you would write it it doesn't mean it's bad it's just different okay in computer science and software development I think there for every problem there are like tens of ways to to solve it and sometimes there is no strictly better or worse solution so it's always good to tell the outdoor that there is some alternative solution but when we don't have any strict arguments for for our code I would stick to the rule that's always outer choice which solution he would stick to okay and we should also remember to as a review as to pay attention to mistakes and errors that appear again and again from the same outer because it shouldn't happen this is why we have this process to learn from it so this shouldn't take place okay so now let's have a few words about how often we should do it because I think you encountered this situation when you were waiting for a review for for a really really long time and you feel that is totally uneffective so what we can do here so I would say the most important thing is to do it on regular basis at least once a day and I like stick to the rule that if the PR is created today it should be review tomorrow at the latest okay we can of course do it quicker but this is the slowest place we can have if you have problems with it you you really don't like to do it you can try to make it a habit okay so for example pick one time of a day like the first time you do when you start working oh the last time before lunch or first time before that or whatever you choose and do the review at this constant time and so it will make this protests like more automatic and more unconscious and it will be easier for you to to get used to it also sometimes code reviews are getting longer because we are encountering some something called review ping-pong so I create a PR someone is giving me comments then I am dressing the comments then someone is reviewing again giving another comments and so on and so forth and so to avoid this situation let's just quickly per app and move things forward but remember do the initial review on your own also to speed up things you can try so-called draught beers if you are using github draft beers is the mechanism that you can actually show the code because is it's a PR which you cannot merge there is no merge button actually so to show the code to the others – to make chunk of the codes as small as possible to review to talk about it alright now few things about checklists and also a question for you could you please raise your hand who of you uses checklists during code reviews is there anyone like one to several person okay that means that it will be beneficial for four other guys so of course we we know checklists we use checklists on every day also there are other professions there using checklists like the sergeant's pilots cleaning service and so on and I I'm sure we are using checklist as developers – but yet we somehow forget that we can make use of them during code reviews so I would like to encourage you to to prepare authors and reviewers checklist yes so two checklists and simply think of what is important during code review in your project in your team and and put it on the on the checklist you can have separate check with like per person it doesn't matter they will differ across projects and they won't be written in stone they will evolve as the project evolved or as our team evolves actually important thing here is that we should put things that might be missing because again for our brains it is quite easy to spot that something is wrong it's more difficult to spot that something is missing so let's put those things on on our checklists and please remember that when you have a review a checklist your own code should always also pass it okay so also use it for for your own code now let me quickly share with you my to sample checklist so first forward for the author and this is the very short one so number one check the size of the PR a number of five change number of lines change number of wishes addressed and can I divide it somehow isn't it too big number to give a short description of a task if there is not sufficient number free advice starting point if needed it might be helpful and number four go through all changes last time especially is good to look at it on the github for example if you're using it not in the IDE because it's basically it looks different so you can have different perspective same as the reviewer okay so my second sample checklist is for a reviewer okay and it's a little bit longer but I hope you find some inspiration here so number one what was the task is the change described with proper context was the reason for the change yeah number two is the pure small enough and doesn't address one issue only and again if not I'm going back to the author and politely ask him to change it number three how would I implement it on a very general level and this is quite interesting because when I try to figure it out and find it okay I actually I don't know where to start and how to handle this it will mean that okay this is the the review I will learn quite a lot so I'm more eager to do it number four which tests describe the change so what's the first fight to review now five is the cold easy to read including tests of course and this is because we will be reading this code much more frequently than than writing it you have to write it only once but we'll read it a few number six our classes in proper packages so this is about the modularity number seven our classes variables methods named properly because this is one of the toughest tasks in software development naming things also and if you are following domain-driven design here it is also important to see if you are following ubiquitous language or not making up some terms that are not used by others or we are using not proper terms for for some things number eight does every new class have a single and clear responsibility number nine is every change covered with a test number ten are there any tests changed and if so it should be checked whether the change was necessary because maybe somebody changed the test that was failing now it's passing by actually he or she changed the behavior of our application which wasn't what this task was about number eleven other any test that's removed and also if so are all of the case is justified and number twelve is the code is the code of good enough quality and this is the very broad topic but for example i following crews like principles like solids dry kiss yakhni etc but of course you can use different different measurements for that if you don't know what measurements you use you can also use so-called WTFs / permanent of course as well alright so now let's get to next topic called psychological aspects so we need to remember that software developers are humans do right no matter how introvert ik we are no matter if you'd prefer to work you know in a team or rather we would like to lock ourselves up in a basement for eight hours and just just quote we are humans and we have feelings okay and so also we have to remember that the code review is only a written communication so we cannot hear a voice facial expression gestures and think things like that you might be familiar of the sentence that words are only 7% of our whole communication and actually that's not true the 7% is it's not not not real rap number but it shows a tendency definitely words are only a part of our communication so we can also experience it I don't know writing joke in email it might not be funny because somebody didn't might not hear that the sarcasm irony in our in our voice alright so the most important thing here would be to be kind to each other of simply okay and we can do it by simple things like using a word please like reformat this lines please or use what form we instead of you like we are missing a test case for X equal to zero and we can use also a form of a question so instead of saying extract the method we can say what do you think about extracting a method instead of writing add a test we have an external exclamation mark at the end we can say how about adding a test or have you considered adding a test here or instead you forgot about we can ask I would rather what is your opinion okay but also we should remember that not every question is a is a good one because if we ask how in the world it was you could miss that you know it's an quite aggressive offensive question and we it's not what we are talking about here also I would like you to think for for a second how do you feel when you know that the code review is only about pointing mistakes so AB all I can get after creating a peer is a bunch of mistakes I made and requests for change it doesn't feel good it doesn't make me eager to create small and frequent peers okay so we should be aware that there are different type of comments during code reviews so there are requests for change there are suggestions there are questions and there are also complements appreciations and saying thank you so I would really like you like encourage you to give compliments like big plus for using composition over inheritance or thanks for clearing up this unused code okay it might seem as a very small thing but in fact it is small thing but it makes a big difference also good idea would be to point places where you learn something like I didn't know this library looks clean simple Thanks it really made people feel appreciated and it's really good to read such a thing in your own poor requests and so we can see the exaggerated example here at this picture saying that okay at least we don't need to obfuscate this code before shipping yeah and so the rule number one try to find at least something positive yes it is exaggerated but I would like really like to exaggerate in this direction then in in the opposite one not giving any compliments in classes in our culture views and also one not here that there are also a research that shows that being kind to other people makes us makes our brains actually feel better so if you don't want to do it for somebody else you can do it for yourself actually another aspect he would be presented on this picture when one guy is asking the other for some kind of review and to highlight things that are stupid yeah and the guys taking the hired highlighter and hi the other guy and this is what we definitely should not do okay so please comment code not people so for example instead of saying that writing you forgot to write a test we could simply state that this missing right also in that area please keep in mind that code review is quite similar to key is about giving a feedback so we can learn from this area as well and so for example we can use iMessage instead of view message for example you didn't initialize those variables versus I don't see where those variables are initialized much more offensive and much much less personal it's better received by the other person also there is the theory it's really beneficial to give 3 positive comments on every negative comment if we won't would like forget giving a feedback to somebody and like him to actually receive this feedback truly and apply some changes it would be good to hear some positive things before and I'm not saying that you should be making up some things but be honest and appreciate even small positives it really pays back okay and also please remember that comments and findings are more are other reasons to celebrate them to be ashamed of so when I receive many comments in my pull request I shouldn't say to myself that oh my god my god is I received like 40 comments this PRI must be really bad at at programming no it's not like that it it only means that this process is is a fact event we should be happy also thanks to the code review process we can notice that the nobody actually writes a perfect code nobody is perfect we can all always talk about code and change toughen clarify make it something something different okay few things about what we should avoid so section called don'ts first of all do not do anything that can be automated so things like tests find bugs check style sonar even size of the pier might be automated yet and so let's use all the tools we can because there still will be a space for things that can be automated it has to be done by by us by developers so let's make this as small part as possible another thing I would like to mention is do not correct someone else's code in that case Altru will learn nothing and also will be left with this terrible feeling that he is he or she is so bad that he or she cannot even fix his own code so please don't do it and last thing we should avoid to do not per se same authors and reviewers all the time because it will decrease the knowledge sharing so the knowledge will be shared between those pairs instead of the whole whole team it's not an easy task from from from my experience it's it's just something sometimes it's often like that that we like somebody better with I don't know we have a desk next to some original so we pair up with the same person but let's try to avoid it of course unless you are you work in a team of two people the end that's you that's all you will have okay and one of the actually last topic I would like to share with you is nor are disadvantages and costs and how to address them ok so first thing would be that another developer has to spend some time reviewing ok and yet that is true but actually this will happen sooner or later because somebody will eventually have to use your coat or fix them back or I don't know change something in this coat yeah so thanks to cloud review actually it makes this time explicit that we need the time and another developer reads someone else's code and it also moves it in time it is it is much sooner also when we can actually influence the code we can change the code okay so it's actually actually not so bad another nother thing would be that there is a time time gap between creating a peer reviewing it applying changes and final merge and yes that is true and we should do everything to to make this gap as small as possible yes so do small peers that address one issue only we should do it on regular basis and all the things I already mentioned and I think I would like to say here is that the results might not be visible immediately they are postponed so if you are only starting your journey with with code review you might not seen the results quite so like after I know one or two sprints it might take a longer time but it doesn't mean it doesn't work it works but the results will be visible later so this is the investment we will we are doing that will repay us slightly slightly later ok and in this case we should only educate others about how it works and actually I think that's all we can we can do all right so let's just quickly sum all these things up in seven points so a code review is a way to collaborate to share knowledge and improve code quality but not as explicit explicitly the back finding tool alright this is definitely not a way to point mistakes and evaluate developers performance number two as there as an author's we should prepare our peers and the smaller we do the better number free know the context because st. change can be good or bad depending on the context number four review on regular basis once or twice a day number five do not hurry up but don't spend too much time go find a proper pace number six use checklists both for outer and as reviewer and number seven remember about psychological aspects be kind and supportive give compliments and praises alright and I think that's all from me so thank you very much for your attention if you have any questions we still have some time here so just raise your hand I'll just ask it if not thank you very much

Leave a Reply

Your email address will not be published. Required fields are marked *