 Right welcome back everyone let's get started and so this afternoon. We're going to have a Discussion panel rather than a talk to start with with these five gentlemen. We've lost one of them the guy from one fun state but We'll do without him So how does it work? we it's a discussion, so we need you to take part of it and No one has prepared anything So it's going to be very Ro and they all come from different companies, so they might have very different point of views about code reviews and Yeah, so let's start with the presentation we first have radic from stacks next it works in Poland and you're a senior developer, right? Michael Ford from Canon Nicole and maintainer of the mock library and Daniel Pope Bank of America should I In it and you've done a review board project Yeah, okay Let's do that Vladimir Risenov from Google where you are very passionate about code reviews and Jonathan Lang where I'm okay Being too late to present them then Can you say that again Cluster HQ for this dreaming and Right, so my first question was What tools do you use do you make them in-house do you use open source projects or do you buy tools? It is allowed. It is allowed if you want is there JavaScript So Because our project we it's all on on github So just want to see like who he actually uses does code reviews and uses github for code reviews in the audience Cool. Okay. Just wanted to see that That's what I asked for github for code reviews Yeah, just One last point if you're from the audience and you want to ask a question just Rise your end and wait for the microphone as a wise you're not going to be aired So we use github. Good. So, um, what tools do you use? so I'm working on a project called juju which is a big github repository and we use review board for code reviews and we have we use the the github webhooks API so as soon as you create a pull request a corresponding review is created on review board And is updated when the pull request changes. I also worked for four years for a company called resolver systems where we did strict pair programming and we didn't do code review on top of pair programming and my experience and opinion is that Code reviews are essential if you don't do pair programming, but they're a relatively poor substitute for pair programming. I Can explain why later if you'd like So a Bank of America there is we've written quite a lot of our own tooling and There is a system for code reviews, which is more like a kind of it's it there is It will give you a diff and you can comment on it, but there isn't like very good support for Like commenting on very fine-grained sections of the of the review So I wrote a tool that's very much inspired by review board that we used in In the team that I've moved within the bank And that was very successful in improving code quality Well, I'm afraid I cannot really say anything interesting because we use our own internal tool, which is very good but It has some things that would not really make sense to like some other companies because it's very tightly integrated to our testing infrastructure and continuous integration and stuff like that I Think it inspired some of the open source tools. I don't remember the name of the tool, but there is Possibly yes, so so there's a open source tool which looks awfully similar to what we have in Google and And Of course, it's pure coincidence. It's probably written by ex-gugler But I know other other teams Who do more open source stuff than I do they use github And there is something else for Chrome project as well and Rodek, what do you what do you use at a stack net? In my current project, we are using github pull requests and comments to the to the diff so that's nothing not a separate sophisticated system just just just github pull requests right and I was actually interested in knowing why pair programming is Better than code reviews, but is there a more pressing question from the audience? Maybe Yes, there is one there Yeah Well, my company were using it right now, and I want to know if you can go a bit on on the difference between Garrett and other tools and what do you think? It should be What are the best option that those two have that you use compared to others also? What kind of other hooks or? automate System you use to check the code like not only for code review from another developer, but just automation Checkers and also what is the important part? What is the policy that you use and what has been the most important policy that you have help you to? Do better core reviews and faster because in my opinion the most important issue is the to share a common policy On how to do the code reviews and what to expect? That was a lot of questions So the first part was what tools do you use and what are the benefits of them the specific features? Is that right and then integration with the? infrastructure and ecosystem around testing and then About the shared knowledge of code reviews, so that's so The shared knowledge is a particular aspect where pair programming Wins a great deal over code reviews one of the big things that code reviews provide is it's not just one person who's looked at this code It's two people But if you're doing a code review of an aspect of the code You're not familiar with having enough context to be able to really understand the code in detail I mean, this is a common difficulty with code reviews is reviewing code that you're not Intimately familiar with can be very difficult and which is why you tend to find if you're working on tricky bits of the code Well, there's only a few people who are really qualified to review this and then the knowledge doesn't actually get shared around a great deal Or you have someone who doesn't know the code at all who comes and does a review and they can only do quite a superficial review catch Your typos catch Duplicated code catch sort of which things that are worth catching but but which don't really address things like architectural and design decisions That are important or maybe notice things like race conditions with other parts of the code because they don't fully understand the semantics of the code You're touching and certainly The pair programming I've done we used to rotate that you'd have somebody who owned a feature And then we'd rotate the person that they were pairing with every day So you would it's not so pair programming is a great tool for mentoring for bringing people on board It's great for collective code ownership and it's really much better than code review for that purpose There's another difficulty about code review is if you have this big amount of code that's landed and you kind of dislike The way it's been implemented, but it's already been done. So to go back and say well actually look It would be slightly better if you did the whole thing a different way Nobody's gonna go and rewrite their code for that But whereas pair programming where you've got more than one person Involved in the design and the implementation of a code right from the start you're much more likely to have those kind of discussions Going on as the code is being done rather than waiting till the end So that's what I that those are some of the benefits that I'd say pair programming offers over code review Yeah, so just it's all I agree with Michael and that those are definitely benefits that pair programming can provide over code review I want to maybe highlight a couple of the things where it where it can sometimes not work so well one of which is that It you can have a pair that ends up this happens I just happened a lot on the twisted project where I contribute we'd be at a sprint We pair with people at a sprint we'd work on stuff. We'd make great code and then a month later We realized it was completely wrong because we were caught in this great group think kind of cycle But having a code review having from someone with a bit of distance from the implementation can often provide really valuable insight So which you can't get in pair programming for precisely the reasons that make pay program quite good The second thing is that sometimes pair programming can create It can make it harder to kind of look for patterns in code reviews, so You you doesn't generate artifacts about what we think works So sometimes it's quite it works because it's like it's organic, you know people are on a site They're talking to each other and that's good, but it can mean that you don't It can mean basically if one person's being an asshole that then You might not detect that and or if yeah anyway, I read along. I had a really good piece. I wrote about this It's much better on writing Did we switch to pair programming versus code review question with The original question was how do you share the knowledge? I mean, I have something to add to it But I just don't want to like kind of switch to different question before we answer the previous one If it's about sharing knowledge by doing code reviews, then you can add this So I think it's important to understand that Pair programming and code review is just like two Kind of different tools and they achieve different slightly different goals so I think for pair programming we have two person who know this particular piece of code very well and they've written it they could probably like Figure out within seconds where the problem is and how it works and so on and so forth But there is also something that we call credibility Which means your code should be readable which means Basically any engineer who is somewhat familiar with the language should be able to figure out what this code does and You know when you have two persons they may end up They may end up with a brilliant code But no nobody else could understand this and then you know one year later These two persons like one of them left company another one moved to different office and now in completely different time zone and Somebody else some poor soul sitting there and trying to figure out what this code does and they cannot understand because they didn't really thought about it so I think this is one thing that Cold reviews, especially if you send Cold review to somebody far enough like outside of your team. I think this one tool that works great For making sure you produce maintainable code It's maybe not code reviews probably not so great at detecting like design problems Because yes, and when you send code to review is probably already too late to to find the design mistakes So so I kind of disagree slightly with so I think code review is more valuable than pair programming I found that I could mentor a much larger team by working in that way, and I could just like working one at a time with With with take members of the team But I think then there are other advantages like having so having somebody outside a pair Get their eyeballs on the code It's not just about sort of the readability of their the product in a way, you know to new to new eyeballs, but to so You know you get to Comment on the readability But you also get the knowledge building up around the team of people who are doing their reviews as to what's going on What are the new features going in? How's this? How is the project evolving and new things that they can they can use? I also think that it's not impossible to catch like architectural architectural problems at code review time So when I'm when I'm doing a review I sort of go through it in several passes and my first pass will usually be like sort of readability things Because they just like stick out like I don't really get understand what's going on here but then you knock it back a couple of times and And cycle it and then you're getting into sort of clear a code it's clear of what the intention is and then it's clear what the the architectural problems might be so I had successfully spotted sort of some significant security problems and architectural problems in code review and and Kick them back But if you've been working with them you'd have been able to discover those Before they've written what huge amounts of care, but the thing is Daniel. Yes, you you can mentor more people by Doing code reviews than the one person you could pair with at a time But if you get your team pairing with each other you get them You're able to mentor much more effectively than just you doing all the code reviews No, I think Wadek wants to add something. I think the most important thing about code reviews is to Make them part of the part of the job because I often saw a situation when people was treating code review like something additional not like Like it was the part of the of the development process and in the results the code reviews were very shallow So yeah, I think the most I think the most important thing so that code reviews Work is that make make developers aware that they have to do code review and that it is It is there their their job to do them well because it won't have Because otherwise it will have some very very bad Results also My my project showed that good notification system for code review is is essential also So there is this should be something that read that the developers will will really read not I think adding the certifications to systems that developers already ignore Yeah, so Okay, so that's it So I think this is an essential point and this goes a bit back to the tooling So we're getting back to the actual question that was asked but code review Along with anything that you want to add as part of your Processes really only works if it's if it's part of the the process and understood as this is the standard workflow If it's in any way thought of as an optional extra, then it's just not going to happen so so the rule we have is that Code has to be signed off by at least one other core developer before it can be committed and what what we actually have is With our github project only the the juju bot is the only Is the only one who can? merge through github is the only one with commit rights through through github and We have the review board has quite a nice ui the dashboard will show you all of the current reviews So and it'll show you the the status So it's only when it goes green that it's got to ship it from another developer that you're allowed to trigger the bot To to do the review, but yeah, it has to be seen as part of the the standard process and and alongside that to avoid sort of shallow reviews When code goes in essentially the the two people responsible for the code we see it the person who wrote the code but also the person who approved it so if there's a Problem if there's a substantial problem with the code to whoever reviewed it whoever approved it Is just as responsible for those problems as the as the person who wrote it because they didn't spot the problem that they okayed it Yeah, so we get more questions from the audience and I'm not sure which This man is closer from you. Sorry Yeah, my question is more about the human side of it How do you motivate encourage and teach the members of the team who are doing the reviewing to make sure they give you reviews in a Timely way, how do you teach them to improve the quality of the review? Like how do you how do you get the human side of the So I'll make a very quick comment before I pass it on that we People who are new to the team we start them off as They are mentored reviewers there. They're okay Alone is not sufficient to give a ship it and we have a process of people Graduating as code reviewers and they will be mentored by another core developer through the review through their reviews will be checked and discussed and So that's how we handle that particular Part of how we handle that city code reviews reviews Yes, we review the code reviews indeed. I'm not sure how interesting is this I mean it kind of happens naturally so what usually happens to People who join Google they join some team They spend some time learning our technologies and then some time later they produce their first change list and What usually happens their first called review takes a lot of time There is a lot of comments both about style and some design issues and you know Just sometimes Like sometimes people even new to the language because they use something else in outside of Google And then when you join Google they just it's happened that they use different language so usually what happens first core review is very kind of long process it usually takes several days and Then the next core reviews are easier and easier and what usually happens that The person understands the value of code reviews The person sees that as a result of code reviews their code is better and it's much easier even for them to work with their own code So I think after a while people just understand what's the value of code reviews That it's actually helpful them So I don't think it's really a problem if you like if you do it right Yeah, so I think one of the things we have is just a list of We've had I've had in past positions a list of good practices and bad practices like just saying Reminding reviewers to be thankful for code is actually like a really nice thing or avoiding the anti pattern of oh And while you're there could you just you know letting things drag off in terms of timeliness There are some tricks you like some organizational tricks you can use like Kanban, which kind of can help But the the at least for me the trick there is to Emphasize empathy and the golden rule due to others what you would have them do to you. No one likes waiting for code reviews Yeah, that and I think if you encourage that attitude in your team more generally you'll end up with a better team I'd also say that this is different in corporate environments than an open source Projects that I work on in my spare time. I would love to review patches more quickly But sometimes I don't have that time and that's a difficult question and I don't know how to answer that one Yeah, actually, it's good points. I think all of you five are mostly working in commercial environments and Mostly proprietary stuff and a bit of open source And open source. Okay, so Almost so it's it's it's different in open source than what you all say And I also have small edition. I think it's very important to Kind of look at what type of like what language do you like especially if you are not like We usually do code reviews in English and if your English is not native just try to be careful try to be kind of Attack a code not a person don't do not say like this is stupid. This is wrong and you are bad engineer Like if you say, you know, like if you could dislike if you could use this library your life will be easier This is much better way of saying this than saying like you should remove all of this So just try to be very polite. It's extremely important especially if you do core review for somebody Who you don't interact face-to-face? Right that is that is a big thing and so I think and How do you talk? I already write and can't review so Either we can go into this so we can try to take another question something to add to that which is that I've you shouldn't when you're reviewing somebody's code be just saying do it like this because that's my preference I Like people should have the flexibility to develop in the way that they want to develop You know according to like house style and that kind of thing But when you're reviewing you should be saying Have you considered doing it like this because of all of these benefits and maybe they have and maybe they will reply and say Yeah, well, that's not right because you haven't thought about this and Engendering a discussion about why the code is is good is a useful product code review process Something further to add One of the ways we ensure that code reviews get done in a timely way is we have a timetable of on-call reviewers just because You know some people don't like doing it and so you're you're scheduled once every fortnight I think it works out. There's a couple of people in different time zones who are the on-call reviewers and Doing code reviews on when you're the on-call reviewer takes priority over over whatever else you're doing and that's You know can be a good organizational trick So make to ensure they happen Right does it answer the question? Great. So we had another question from here and Quickly I I can't sell you any good idea for For teaching people how to code review but what I saw in my in my project is that when a new person comes to team and He does his first code reviews he always sees things that other developers are used to and He always can't point out some problems that we already have some Workgrounds and don't want to fix it and he points it out and we we can we can Do we can fix it and Then this way improve improve the project How do you end all? Emergences like production emergencies You don't seem to be web developers, but It has happened to that a Bug has gone through the whole process comes in production and the client sees it But isn't it different times on so for him it's like four in the afternoon To us it's maybe two in the morning So there is Generally one person staying Available, but if he pushes his correction That probably won't be reviewed when it goes live How do you end all that kind of case do you keep two people available? We have people working in all the major time zones, so I Think you that's that's possibly a different kind of aspect of the operational life cycle So if you release and there is a problem, do you have the capability to roll back? That's one of the things that Bank of America, and you know absolutely requires is that there is some back-out strategy if everything goes wrong Roll back as questions later Yeah, also this can be solved partly with tooling so Google's cut review system has a way of forcing Saying oh, this is really important. I'm gonna force it through but then that gets flagged Later and so like that's something that you you have to get reviewed later and the ways of bubbling that up so that you know that Yeah So yes for for very important stuff There is a way to submit it without called review, but it's kind of not encouraged to do this so if you can like You know, it's it's not uncommon that Your fix for a bug introduces some other buck which is even more horrible than the original bug because you just very hard it So I think it's actually very important to review this kind of fixes in In my project we have We are Reviewing the stories after they are much to master once again. So if then something is broken the same person who worked on the story is Is working to fix it and after after the master is pushed on production and it's it's broken there There The people who do code review, I think I don't are not cold just the just the team that Handles the integration is is is deciding either to hotfix or revert it Right. I know there are more questions, but there is a point. I think it's really really important. I would like to Focus on which is how do you? Talk to the other person when you're reviewing how do you how do you say something is wrong? How do you? How do you disagree? How do you say it exactly? Do you say I think that piece here is wrong and should be done this way or So when I when I do code review and wait when when when we do a code review in our team we try to suggest Better solution and if it's a if it's a small piece of code small function. We Suggest a new implementation or a skeleton of new implementation Or just if if if it's clear if it's if it's a matter of using like another library or function that already exists, we just Suggest then the name of the of the model of function. So that's that's how it that's how it goes and Sometimes of course there's there's situation where something is wrong and I don't want I don't know how to fix it or Maybe not that maybe I just don't have an instant idea for a better for a better solution and in in that case I just I Just write that it's it's that this this part is bad or it should be Rewritten and and often go to that person and talk about what was the idea behind the behind this implementation Yeah, I think it was Jonathan who said that when reviewing One of the most vital skills is empathy and I really with code review that's it's gotta be that it's you know We've got to be as adults. We've got to be able to to disagree and discuss and You know see things from other people's point of view that there's no way around that but just really to say that Canonical where I work it's all remote work We're distributed team distributed across countries and the thing that makes Such a difference when working with people and discussing things and having code reviews is having met in person we do sprints at least a couple of times a year and Once you've met a person and you you've you know had a meal with them And you you understand how they talk and you can almost hear their tone of voice when they type something on as a suddenly Having discussions become so much dramatically easier. So It's very important to even with remote work really if you can to meet up with people I don't think this is that difficult. I think you just be professional It's sometimes difficult. I had a review a couple of weeks ago That just like had me almost tearing my hair out. It's like you just don't understand the first thing about this application I wrote like a kind of techie review and I looked at I thought this is just unprofessional and I Tried to roll it back so that it was guiding that person as to how that they they could get up to speed on how this this whole system works and And like I hopefully removed most of the tetanus might show you through a little bit Yeah, I agree that it you should be very careful when doing code reviews. I think like if you should be in the Correct mindset when you do code reviews if you are not Like if you're angry, it's better to not to do it right now. It's better. I know go have lunch Relax a bit and then do a code review because otherwise you like, you know, you work with people He's like we all work with people despite what we are programmers and we are technical people interacting with humans is hard and This might be actually the hardest part of our jobs So just be careful if you're not native speaker be especially careful Yes talking to people in person helps a lot if you cannot fly have a video chat or Just, you know in the phone call is much better than just email Because you know, some people just you look at what they write and you would think like he's such a terrible person But it's up. He's actually not or he he or she sorry and On the other side if you receive code review, it's important to assume best intentions like you Whatever they say no matter what the tone is The intention is to improve your code. It's not to annoy you So you should never you should try to not Receive criticism as it applies to yourself. This is only criticism for your code Which is not the same as you Yeah, I'd say that there are also just a couple of tricks like you can Air on the side of asking questions like why did you do it this way? or You know what what led you to do this? Someone mentioned earlier talking about the code rather than the person So rather than saying you've missed this use case you could say this function doesn't handle this use case Which does you know, which does make it less personal and also to just make Reports subjective facts as in I found this hard to understand at first, you know, perhaps doing this That those are all all good things. I think that you can do that can help Yeah, and also be fast like having a fast feedback loop actually covers a multitude of sins We don't have a lot of them for a lot more questions. So we're going to take that question over there Thanks So I was wondering about adding the incentive for making reviews, you know Have has anyone tried the gamification or something like this, you know coders giving hearts or stars or whatever for the review or something like this? I don't know So Gido in a talk years ago said that You always do code reviews. Sometimes you get to do them before the patch lands Other times you do it when you're reading someone else's code when you're trying to change it So in some ways, I haven't tried gamifying the incentive is being able to maintain the code base that I work on Yeah, so I didn't really comment on how how timely our reviews are in in my current team but They haven't always been it's always been sort of a case of like nagging one of the things that we have inside the project is we have Like a chat system and there is a chat bot that means like this maintains this sort of internal Currency balance thing for every every person and so by saying happy holiday to the bot on the day that Like it's a holiday in some country in the world You get like 10 quartz coins and then you can like do whatever you want with it You can bribe you can use the to bribe people however you want like maybe sort of say some QQs up to for grabs if you Review my thing These things have no intrinsic value But people understand that like, you know, they have a number there and the number bigger numbers are better So they want to acquire these things It's amazing what I'm afraid about using Gamifications for code reviews that it will escalate quickly quickly so people will just click I it's okay. It's okay. It's okay and Yeah, and after that you will you will say, oh, okay, so these people are just doing shallow code reviews so let's maybe Have some harsh consequences if code reviewed Broke breaks production and then people will be talking. Oh, it was not back introduced in this code review It was introduced by someone else and That's will that will just get worse or and worse worse and worse so I Don't say no, but you need to be careful and know know the people in team to be sure it won't be it won't escalate like this Right. We had a question at the front Hi, a few of you touched on the point that a curvy was is a bit late for sort of architectural And and and design our reviews. Do you have any other process in place that happens earlier in the flow? Well, I always think it's worth having a conversation as soon as you pick up a story pick up a card off the canvan or whatever Just say like what is the expectation for this? It's you know, sometimes you like really get to get in and that conversation gets missed And that's that's most likely the cause of the problems but then for for sort of larger piece of work, I found it very useful to do CRC cards, which is class responsibility collaboration. It's like a Java technique and In Java all of those can't you say this is a technique where you write down you do like a sort of small piece of upfront designs. It's like This works in an agile process. You just sort of gets in thoughts on the table for how an architecture will be laid out just using the layout of cards and the Information that you put on them and you iterate over that you can tear up cards and move them around until the sort of the pattern is clear and Then That can give you that might not be per story. That might be sort of per epic or whatever so it gives you sort of a map of how that epic is going to to To work and it doesn't take very long takes maybe sort of an hour or a couple of hours for a few months work maybe or a month's work and it should then be very clear how you can you should be able to pick up any of those cars off the table and Sort of go straight to an implementation because you know what the responsibility is of that Component that could be like a function or a module or a class in python Or you know any of the ways of defining structure in python But then you can just sort of you should be able to say well, this is going to collaborate all these things I need to create mock versions of all of those things And just you'd be able to write the test suite almost sort of straight off Ask me more about that if you if you're interested Strong opinion a quick opinion from what I can and we'll have to Okay, so in the beginning of our project we were encouraged to post pull requests with code even if it was working if it was still in progress so that people can review the early versions of solutions and this really worked because for example when you pushed your code after one day it it wasn't so hard to rewrite the architecture because because it wasn't too much work and The other The other idea is that new new people in team over often are working in pairs with with someone experienced or if you are going to do something in a part of code that someone else is Specialized and did did much there you can just ask him if this the solution is You are going to implement this okay, and it's it's working. It's If it's alpha sentence only at two sentences too much So we do something called design reviews, which means you write a document which called design document And then you send it to somebody to comment on it and to verify that your ideas is correct And this document usually does not contain any code. This is just kind of a system diagram, which kind of Try hole helps to catch like bigger design problems Right. Um, we really have to go say thank you all this because and Yeah, let's go for the next old then