 technical, unfortunately, sorry, not that many code examples. So my name is Mikola, I work at Pocket Math, but everybody just calls me Mik because it's shorter and easier to pronounce. So we're going to talk about some code review stuff. So as you can see, the title of the talk is actually, do we do it right? And there's a question mark. So it's not the best way to do code review. It's not the only right way to do the code review or not just let's do it because it's all irrelevant. And we still learning how to do this, right? So it's still learning. Nobody knows how to do perfect code review. Some people don't do code review altogether. Some people do it occasionally. And there is always more than one way to do something, right? And of course, we all have different circumstances and we always have to remember about it. So what is this talk actually about? It's about how do we do code review? What are the common issues that we are facing while doing it? Tools that can either simplify or help us with this process make it better. A lot of communicational aspects which are involved in this process, and this is a very important bit. And how can we become better as the authors who are creating the pull requests and the reviewers who are doing the code review? Okay. So let me share some of the emotional feelings for myself because I'm sure everybody, when you start this career and you're like, okay, why do I even need something like Git? I just push the code to the files and that's it. Like why anyone has to review it? So I was not using Git for first couple of years in my career or any other version control systems and I was happy without it. I didn't felt like I needed it unless I started working without the engineers and then I felt the need. At first, I really, really hated the code review. Like it was always pain. Like you have to deal with comments. You have to convince your team members that whatever you're presenting in your code review actually makes sense and you want to have this in your code base. Pretty much you're asking people to commit for your changes. At some point, I've become kind of okay with the code review as a general thing. I started to understand why are we doing this. I was still not happy when doing it. I was still struggling but at least I saw a value and I finally start to understand it now. I think this is why I'm doing this talk because like I finally feel like I'm at the place where I know why I'm doing this and I see that other people are having the same struggles that I had previously. So maybe by sharing I can help them out. So we'll start with the like the previous talk was very technical and you go so fast. I'm sure people had some issues understanding all of it. So I would like everybody to be involved in this one. Okay. So we'll start with some questions and I also will be answering the same questions so you can just use your hand to show yes or no. I always use version control and again like let's not say about the first day when you just started doing this. Let's only consider, I don't know maybe last year or last two years in your career. So I always use version control for all the work I'm doing. Cool. Cool. That's really good. That's pretty much everybody. That's very good. All my changes go through the code review process. Okay. I would say maybe 15, 20 percent of people. Good. Good. At least we are honest, right? That's good. I said no by the way. I don't do code review all the time. I review and read every pull request made by my team. Okay. I see like five hands. This is terrible. Okay. We definitely have a room to improve. I enjoy doing code review. Oh my God. We're going to be here forever. I have to convince all of you that this is a good thing now. I feel great about getting some feedback and comments. Okay. This is better. People are happy with comments and feedback like maybe 50 percent. This is good. This is good. I review code by checking out the branch and doing the review actually on my machine, having access to the whole system, not just the diff that GitHub will show me. Okay. Like two hands. Nobody does this. We'll talk about this one. Okay. Cool. So my talk also makes some assumptions about your circumstances because what we're going to talk about here cannot apply in all the circumstances, of course. So I assume you are not a solo developer in your project because if you're a solo engineer, sorry, most of these things wouldn't apply. And I also assume that you have less than N engineers in your team because if we are saying let's try to review all the pull requests that are created and our team has 15 engineers, we're going to waste all of our time sitting and doing code review. So of course you have to have a reasonable number of engineers in your team. If you have more than, I don't know, five or six, maybe you have a different problem, not a code review problem. I also assume that you are using Git or other similar source control version systems and that you are using GitHub or again something similar, Bitbucket for example. And you're using pull requests as the main mechanism for doing code review. Okay. So why do we do the code review? This is again a short question. You can just scream your answers. Like the first thing that comes to your mind, like why do you even start doing code review? Don't break production. Okay. So it's partially QA stage of work, right? Okay. Anything else? Learning. Great. Knowledge experience, sharing. Awesome. Anything else? Come on. Don't be shy. Couraging mistakes. Okay. So again, it's partially not breaking, right? So we're kind of trying to do the QA work while doing the code review. Okay. Let me show you what I have in my list. So catching issues and bucks, make sure code follows our style guide, checking for performance issues or any other issues, like who knows what are your circumstances, what kind of requirements do we have to your project. Suggest alternative solutions. Even if we do something that doesn't break production, it doesn't mean that this is the best way to go, right? And knowledge sharing as usual. Okay. So we're going to go through some of those into more details. So first of all, one of the whys is catching issues and bucks. And usually when I have this kind of conversation, this is what everybody starts with. Everybody's like, yeah, we're doing code review to make sure our code is better. It doesn't have issues and bugs. Unfortunately, it's not true. In most cases, the level of QA that you are getting out of doing code review is very low. If you were like most of you said that you are doing code review, how many issues are found by QA? A lot, right? How many issues are still found in production after the code review and QA? Still a lot. So code review, we want to believe that by looking at code, we are smart enough to see the issues. Unfortunately, usually it's not true. When we do code review, we can see just small diffs. We don't see the whole workflow. So it's kind of good to catch something like syntax error, like someone forget to close. Yes? One of the distinctions that you may want to consider making is between code review for production code and code review for the tests against production code. It's been our experience that if we have our automatic tools fail and deploy if code coverage is less than a threshold, which is generally in the high 90s, then we spend our time reviewing the tests to say this is what we're testing for. These are the conditions that we're testing for. Mr. Domain Expert who's sitting on the other side of the virtual table participating in this. Mr. Domain Expert, do you know of any business rules that were violating or that were not catching? And that is far more productive for us than manually reviewing actual code. Yeah, sure. I completely agree. And again, this doesn't mean that you don't have to review and you don't have to try to find issues. This just states that even though we try, we usually are, we don't have the whole picture. We don't have the whole workflow on our in front of our eyes. We just look at the dips. So catching like a syntax error, I forgot to close the parentheses or like maybe I have a possible nail and I forgot to check for it. That's kind of easy and can be done in during the code review. But catching integration kind of issues, like we don't have the system workflow. So it's kind of kind of hard to catch those. Okay. So another thing is we try to follow the style kind, some style guides. And this is a lot of comments in our code review comes from here. And I really, really hate when people start doing this because it's like, especially when someone new joins the team, nobody tells him, nobody on boards him about the style guide. And everybody jumps first in the code review to say like, oh, this is the wrong number of spaces you are using here. Like, oh, we don't use open and close parentheses in this way. So this is something that you have to do. But maybe code review is not the perfect place for that. You can onboard the person, you can have a separate repository with your style guide. And we'll talk about the tools that can be used for this one. Okay, so I'll show you a couple of examples of this. This is the only code that you will see in this presentation. This is just a couple of simple examples where we can just see what kind of issues can be highlighted during the code review. So does anyone see any issue in this one? Yeah, just typo, right? It's supposed to be a box. And we have a backs. So this is simple. We can easily look at this method. We don't need to see anything else in the system. We don't even care. Is it like a bab application or whatever? It's fixable. This one is a bit trickier. Again, anyone wants to guess what's the biggest issue here? This is not about syntax, by the way. Yeah. Well, the tricky part here is the map. As you can see, we're using where. So I assume it's an active record model. And we only need ID so we can use block, for example. Another good issue here is the name of the method says items. But we're actually only returning ID of the item. So maybe the naming issue here, maybe the method should be called item IDs instead. This one is just everything. The name of the variables are wrong. We use different parentheses and multi-line code and it's just crap. It's really easy to poke at this code and says it's crap and leave like bazillion messages. And we can use a lot of tools to automate this. And it makes our life much simpler. We can use RuboCop. HoundCI is a great tool where you can specify your own style guide or use just the default one that Hound specifies, which is pretty good. We can use code climate, etc. And believe me, receiving messages from the bot is way, way better than receiving them from your team members. Because if you forget to put the space in the right place and the bot highlights this, you just go and fix it. You don't have any emotions around it, right? But if the same person comes again and again and says like, come on, Mick, how many times do I have to explain, put the spaces before and after and whatever. So yeah, use tools for that. For tests, hopefully you have continuous integration. So when you look at the PR, you already know if the tests are passing or not. You can also use GitHub templates. You just have to put the file called pull request template into the root of your Git repository. Every time you create a GitHub pull request, it will just automatically pull in a lot of data. I have a screenshot of an example that we're using currently. So every time I create a new pull request, it already has like a bunch of items for me, the place where I put the ticket, the place where I put the logic explaining why did this change. I can do a checkbox to specify which parts of the system I have changed. I can put screenshots, et cetera. So let's try to make our pull request as descriptive as possible. So this is a different type of example. And you can try and find some issues in it. But I would have to say it's completely useless. Why? What can we tell about this code just by looking at this one small bit? Just imagine this is a pull request and this is the diff. This is the only line added in this pull request, for example. Unfortunately, we cannot tell what's going on here. We don't have the context. Why are we skipping it? In which cases do we not want to skip it? Why we only skip it for the delete? What's going on? So some of this context we can get out of the pull request description, maybe from the git commit message. Maybe you actually have to go and look up the ticket assigned to this pull request. But in general, it means that you need to get some context. Whether you get it, it depends on your case. So another good example. I have existing methods and for some reason I've added this line at the top. Same scenario. I don't actually change the business logic. Code looks legit. It will definitely work. I don't think it will crash. But why I'm doing this? What would be the end result of this? Maybe some of the workflows will stop working because it's okay to skip name in some of the cases. Who knows? So it's all about the context. When we are doing code review, in order to do it properly, we need as much context as we can get. So pull request is just a diff. Usually it doesn't have any context. Or you are really lucky if it has a context because, let's say, you're building a huge feature and all the code changes that you need to care about are actually present in the same pull request. And hopefully you don't have to deal with huge pull requests. It's a big pain. So unfortunately diff usually, I'm not saying always, but usually doesn't give you enough context to make the decision. This is why we have to get this context from somewhere to be able to produce a good quality of code review. So let's now switch to the actual process and who's involved in it. So we have the author of the pull request. It may be a couple of people. Usually it's just one person, but can easily be a couple of people. And we have reviewers. And as you can see in parentheses, they put that preferably it's the whole team. Again, we'll talk about this in the later slides, but preferably everybody can take a look and still manage to get some value out of it. Either this is a passive value, like me just understanding better what's going on, just to get to know what other team members are working on, or actually be able to suggest a better solution or find an issue before it actually gets deployed. So for the author, you have two main responsibilities. You have to provide what and why. So what is kind of easy? It's the actual code that you put into this pull request. This is actually what we see in the diff. But the why is a bit tricky. Expressing the reason why we're doing some changes. Usually you don't get a lot of those. And we'll try to go deeper into the details of this one. So improving of the what. So if you're doing a lot of commits, it's really cool if you can group your commits by type of changes you're doing. So especially like if you ever went through the code task before your interview, every time you would see the description and it would say like commit as frequently as possible, because your commits actually show us the history, what you were thinking about when you were doing this, in which order you were implementing features, etc. So the order of your commits and how you group the changes in the commits might be interesting. Move refactorings out of the feature changes. I try to do this as much as possible. Unfortunately, it's not that easy. Usually when I'm working on a feature and I look at the code and I try to realize what exactly what I need to change, it looks like this. Oh, I can just do 10 different things and make the code simpler. And then my actual feature would be just one small line over here, or one additional small tiny class over there. And if you produce this refactoring and implement a new feature in one code review, it's kind of tricky. It might be tricky to understand why you're moving the bits from one place to another, etc. So the perfect case scenario, you split those into two. You do refactoring first. It should be very straightforward. You don't change the logic. Perfect case scenario. You don't even change the tasks. Everything is still passing. You're just making the interface cleaner. Then the next pull request, you do the actual feature change. And hopefully this pull request again will be unbelievably simple because your refactoring prepared us for this change. Moving other fixes or improvements into separate pull requests. So again, unfortunately, we all have to deal with some old code bases and they're not always in the perfect shape. Perfect shape. So as you go through the code, you usually find some low hanging fruits like, Oh, I can fix something here. I can improve performance there. Maybe I can update this jam. So you end up pushing a lot of small tweaks and changes into your feature branch. And sometimes it's okay. If it's still logical and simple to follow up, it can be okay. But again, in the perfect world, hopefully you can highlight like, Oh, I can move this into a separate commit. I can move this into a separate pull request. Again, it helps us to create the smallest pull request possible. Because I'm sure when you have to review something, the first thing that you go and take a look in the get is like, how many files have been changed? And when that number is around like 40 or 50, you're like, Oh, my God, I'm going to get stuck with this pull request. And other people would just say like, Oh, my God, it's 50, 50 files being changed. There's no way I can review it. Like I'm just going to scroll, scroll, looks okay. Done. This is not a good code review, right? So in order to get a good feedback, and we want to get a good feedback and get some sharing, we need to make the size of the pull request to be manageable. It has to be reviewable in a small chunk of time. Like I have a time between my features or between my meetings, I have 15 minutes, I jump in, I do the code review. So now we'll talk about why and like, so why are we actually making the change and how to express this in the best, in the best way. So commit messages, commit messages are important. Maybe they're not that important during the code review itself. But what happens after it's already merged? And like, two months later, we have a bug in production. And we go back and we take a look at the, at the git blame. And we see a commit message. And we are like, Yay, maybe this commit message will tell me something. And the commit message is like, fix spec or fix that fix again, fix broken spec, et cetera. You know all of those commit messages, right? You have a bunch of them. We, we keep, we, we tend to use those short messages because the, the 10th commit that you do to fix back, you don't have, you don't have the nerve to spend proper time putting in a good commit message. You would do something like fix this shit, boom, go. So even if you do have a messy commit messages, because it will happen eventually, at some point you will have a crappy commit history. In pull requests, you can do this in the UI, which is unbelievably easy, or you can do this in using your git, you can actually squash them and change the commit messages or provide completely new one or just combine all of the commit messages into one. Sorry. Again, so with the commit messages, what I usually recommend people doing include the ticket number. If you're using Jira or something similar, usually your tickets would also have a lot of useful information for the reviewer and for the people who would have to support your code, right? So put the number of the ticket into your commit messages. It will make it that simple to find and understand why the particular change was made. You can also add your comments to your own pull request. I do this all the time. I create the pull request and I look at the diff and I'm like, hmm, this bit, I'm sure someone will be confused by it. And I forgot to put any comments into the code itself. Maybe I can just comment on the pull request. And this is more than fine. Use the pull request as the place where you can express your thoughts. Some people would say you that leaving comments in the code itself is bad because as you change the code, you usually forget to change the comments and now your comments are expressing your previous thoughts and the code already moved forward. So yeah, comments in the pull request are more relevant because you're making them to the actual change itself. But still you can of course use comments in your code. I know a lot of people don't like to do this. A lot of people would start screaming like if your code needs comment then it's a bad code. The code should be unbelievably readable and easy to understand. Yes, in ideal world that might be true. Unfortunately, we are not in ideal world. So don't be shy. Put some comments in. And small versus big changes. Again, the smaller the change, the easier it is to understand why it's made, what are the implications, etc, etc. So try to always keep your change as small as possible. It's easier for you. It's easier to whoever is reviewing your code. So when you're doing the review, you're the reviewer, your main thing is your feedback. This is your your sword and your shield. This is what you use in order to go through the code review, your feedback. So feedback is a key here. And feedback doesn't mean critique. A lot of people think that if I'm asking you to review my code, it means I'm asking you to tell me what's wrong with it. That's usually not the case. Code review is not just about finding issues. It's about a lot of things. It's about providing positive feedback. Like, oh, today I've learned something new because I saw you used it in your pull request, right? Oh, this. I don't like this. I put in the comment and the guy responds and I'm like, yeah, he's right. My initial idea was wrong. He's actually right. I learned something, right? And then again, sometimes we use we use comments, right? So we usually write and unfortunately written communication is very hard. Okay, it's unbelievably hard. Like, imagine we are sitting together and we are pairing, right? And I look at the code that you just put in and I'm like, Oh, my God, this is terrible. I have a great idea. I'm excited. You're excited. We are sharing no issues whatsoever, no offense, right? Now imagine I come into your pull request and I'm saying, Oh, my God, I have a much better idea. This is crap. The first reaction would be like, why are you offending me? And like, you don't see my smile. You don't hear the tone of my voice. You just see the bare words. And usually it's really rough, especially if your team member doesn't actually select the proper words to do this. So sometimes written communication is very harsh. And we just have to remember this because it's very, very easy to forget. You're just like, Oh, this is wrong. And you just put like, this is wrong. Do it this way. You're putting the command. You're not giving any alternative solutions. You're not asking. You're just saying what to do. This is not a feedback, right? So we need to balance between the positive and negative feedback because like, whatever crap you found in the pull request, there's definitely some bits that are okay, or they're perfect. And because we usually only leave comments for the bad things, it looks like we hate your pull request altogether. Even if we hate just 1%, if you put that comment, it will look like I hate it all, which is usually not true, right? When you review, there's a lot of good bits. So don't be shy, say something. It's okay. Encourage people, especially if those people are a bit juniorer than you. They look at you and they hope that you would give them some feedback. And of course, you need to have a checklist. Like the checklist, I don't care if you put it down somewhere on a piece of paper, or you use the GitHub pull request templates as we do, or you just have a verbal communication about this checklist. But checklist is very good. And what is checklist? So for checklist are some critical aspects of the change that you can do in the code base, which definitely require a more deep review. So like any DB changes are usually very critical because what if you do migration, which is not reversible and something goes wrong? Like you have a downtime and now you have to somehow manually restore your DB, which is terrible. What if you're doing data migration? This is always very, very sensitive because you can lose data, right? So there's definitely aspects of review where you need to pay more attention. Rake tasks, again, whatever you do, at least don't forget to write them if you have a post deployment rate task or something. Upgrading any libraries. I don't know about you guys. I've been in a hell with updating of some of the libraries, especially libraries which would have some kind of third party integration, like uploading files, for example, like paperclip or something like that. When you upgrade the libraries, usually what I do, I actually look at the change log between the version we were using before and the version we are trying to use now. And sometimes I do find some breaking changes. So it kind of works for us, but there are breaking changes and who knows what will happen when we deploy this. Any API changes? Again, like your API is your contract with your customers or clients or third party integrations. So whenever you're doing any API changes, you always want to check if you are not breaking any compatibility. Any deployment or infrastructure changes? Again, like ops process is different everywhere, but it's always critical. It always means you can have a downtime which is terrible. And like cleaning up the related code, like imagine, again, like I went in and I updated the version of the gem, right? Everything's fine. The review went in and then suddenly someone says, like, oh, we actually had a monkey patch for that library because we wanted this feature, but we were not ready to upgrade the gem. And we still have it, unfortunately, right? So sometimes, again, you do need to have a context because you don't see those changes in your diff. So you need to check out the branch. You need to go in and say, like, oh, we're changing something. Like maybe there's some code that was depending on it that now we no longer need. We can clean it up. And those are usually very, very hard to do. And like, believe me, nobody can do this properly, unfortunately. And yeah, keep updating your checklist. Our checklist has like maybe 15 items. And we kept keep adding in as we are facing some issues. So this is like the hardest part of all of this, like dealing with a negative feedback. When I was starting the talk, you all said that you're using pull request, but only 50% said that they actually enjoy the process. And in most cases, we don't enjoy this process because of a negative feedback. So try not to judge. This happens to me all the time. I read the code. I'm trying to investigate an issue. And I'm like, who's the idiot who put this in? Like, like, what was he thinking? And then I do get blamed. And of course, it's my code. Yes, that happened a lot. And it will keep happening. So yeah, just don't judge because you don't have all the context yet. Try asking questions. Like, if I'm looking at something, and I know for sure that this is a terrible solution, I can easily go in and say like, this is crap. Do it this way. But this is this will be very, very harsh for that person. Maybe that person was in a rush. Maybe that person is very inexperienced yet. And by giving this feedback, I'm pretty much killing the morale, right? But it's so so much easier and so much nicer just to ask a question saying like, Hmm, what do you think about changing it to and you present your solution? Or have you thought about this way of doing this? Or what was your reasoning behind this idea? Try to engage people into conversations. Don't just say no, and give options. In a lot of cases, what people would do if they see that something's wrong, they will just scream about it. Like, this is terrible. This will kill our performance. Or this is terrible. This opens up some security vulnerability. But in a lot of cases, the person who's leaving that comment doesn't actually have a better solution. And if you don't have a better solution, you have to be very, very delicate with this. Because by screaming no, you're saying, Oh, you're stupid. I know it better. When you actually don't know it better, right? So you're not sure. Maybe the person is already aware that this solution has some drawbacks. But because there is no better solution, and nobody can produce a better solution, this is what you end up dealing with. Okay, suggest alternative solution. If you see that something's wrong, and you do have alternative solution, present it, say like, send the links, it doesn't even have to be your solution. You can just say like, Hey, I'm following this cool guy and he posted this cool blog post that actually highlights the better way to do this. Share this, actually post a link, let the person get to your level. Don't try to be a smart ass like I know this, you don't. So you suck. No, share. Start the conversation instead of just commenting on each line. So sometimes when you look at the diff, the number of things that you would like to change is so huge that you actually end up commenting on pretty much every line of code. This is a very bad sign. This is a red flag. That means stop reviewing right now and go talk to that person. Maybe there was a big misunderstanding and the person is actually trying to solve a different issue or the requirements that were set incorrectly in the ticket or something like that. So if you see that the amount of communication will be huge, it doesn't make sense to continue it with the comments. First of all, because it's non productive. Second of all, as I said, written communication is very hard and having a dialogue in a written communication is just terrible. So just go talk to person. If the person is remote, thanks God we have a lot of tools to deal with the remote calls, etc. So usually if something's wrong, it means you don't have the whole idea or the whole list of circumstances around you. So you always have to take circumstances into the consideration. So again, circumstances are part of the context. Imagine we have a terrible deadline. We have to deploy it right now and you start telling me like, maybe we can change because I don't like the naming. This is not the time. I need to deploy right now. We can change the naming at any point. Just come back to me later and we'll talk about this. So circumstances are very important and they build this context around your change. And the context can be very different. What is the source of the original code? Let's say I'm backporting a feature from a newer version of Rails back to my old project. Right? I'm actually introducing this code but I am not the author. I don't want to change it. I want to keep it as it was in the Rails even if I don't like the naming there or the space and not in the right way. Who cares? This is not my code. I'm just moving it so I'm not going to change it. Time and complexity constraints. Sometimes we just don't have time to deal with something better. Sometimes we have to rush and produce the value right now. Sometimes the complexity of the change is too big and we don't want to spend too much time on this because every change has its value. If I'm going to go in and say like, oh, we have tabs in our code, I'm going to remove it and put spaces in, maybe it's a good change but does it actually produce any value for us? Does it improve our quality, stability, performance? No. So if you made a mistake and put a tab instead of two spaces, it's okay to fix this one line. But asking someone to go back and do a bunch of crazy changes just because we would like to see it this way might not make sense. So always think about the return of investment. What we will actually get out of this change? Is it going to be more readable, more maintainable, more testable? Those might be the values that we might spend some extra time in order to get those. What if I'm building just the proof of concept? I'm sure that the level of review and your comments would be completely different if I would just say this is proof of concept. I might just remove this completely tomorrow. Or if it turns out to be a nice idea, I will keep working on it, I will keep improving it. Same with minimum valuable products. The level of your review would be, again, different. It's no longer just the proof of concept, so it has to work. It has to be somewhat readable maybe, maybe not very readable but still. It has to actually perform because if it's going to kill our servers, it's not a good value. So there's a lot of danger of giving this negative feedback if we don't take into the consideration all the possible aspects. So we are demoralizing people. If you keep submitting your PRs and getting the same stupid comments, you're just killing people by this. People start being afraid to do this. It's counterproductive because instead of actually getting the value out of it, you're demoralizing people. It doesn't encourage an open environment. So by open environment, I mean environment where you can easily express yourself, where you can submit a pull request and if it's crap, people will give you a proper review. They would say like, I like your intention. I like what you're trying to do here, but this is not the right way or this is not the right time. It also limits the knowledge sharing because if you're just saying no or just blindly suggest another solution without explaining why you're doing this, then technically that person will do the right thing, the right thing, but it might not actually share the knowledge. So the person might do the code change without actually understanding why this is a better way. Another problem that I see a lot is the senior versus junior. So by senior and junior, I mean it can be the level that you are on or it can be number of years that you have spent working on the project so you know the domain or you know the product. So for seniors, seniors usually feel like they have to provide a lot of feedback. They kind of feel like that if a junior engineer submits a PR, it's their responsibility to come in and just crush them, to show them that they know nothing, they have to learn, learn and learn. And it has to be negative because positive like he's learning, so he needs to produce some positive results, but I'm going to highlight the negative stuff. And they feel like they have to find those issues. So even if the PR is kind of okay, maybe it's not perfect, it's kind of okay, we still get this feeling like this can be improved, this is not perfect, this is not perfect, and we keep pushing those comments. And it can be okay, but in most cases it's just too much. For juniors, they're just kind of afraid to provide the feedback. So imagine I'm a new comer in the company and someone submits the pull request and I see something that I don't understand or I don't like, but I don't have a strong feeling about it. It's just like, I'm not sure. In an open environment, I would just freely just stand up and say like, sorry guys, maybe I don't understand something, can you explain this? In the closed environment where everybody only communicate via these negative messages, it would be pretty much impossible and people just close down, they're too afraid to ask those questions. And they also are afraid to look stupid if they ask something that they don't understand and other people just get this for granted because they worked on this product for ages and they know all of this. And they are also afraid to look smarter than the senior engineers. In many cases, I've seen the circumstances where the junior engineer would see the issue, but because the pull request was created by his team lead or architect or whoever, he's too afraid to comment on this because he's afraid that there's going to be some personal issues like, oh, you're trying to make me look stupid or something. So we have to remember that code review is not a competition because a lot of people try to compete in code review and this is a terrible idea. So the speed of code review is irrelevant. If your code review is getting merged after five minutes, it's cool. If my code review stays in code review for two days, it doesn't mean that it's terrible. It doesn't mean that I'm stupid, slow, anything. So just keep remembering, you don't care about the code review speed and you don't care about number of comments. If someone posts a lot of comments in your pull request, hopefully you learn and you don't just submit negative comments, it's a good thing. It might be a great knowledge sharing. It might be a great discussion. You might actually produce a better value. So the fact that nobody comments on your pull request doesn't actually mean that you're super cool and nobody complains so your code is perfect or something. And yes, some people use code review just for showing off and I hope you don't have to deal with people like this. But unfortunately, sometimes you do have people like this, people who don't care about most of the values and only care about showing that they are better. They will produce a very complicated solution. They will send you articles to some really, really fancy algorithms, et cetera, et cetera. They don't actually care if you listen to them. If you implement those changes, they just care about showing that they know better, that they know more, et cetera. So this is a red flag and we'll talk about this a little bit later. And code review cannot be personal. So if I'm putting a lot of comments into your code review, it doesn't mean that the next time I'm opening the code review, you have to go and say, like, hey, it's revenge time. Now I'm going to leave comment for every line of your code. I'm going to just destroy you. It doesn't have to be personal. But it becomes personal because we only do negative feedback. So we are hurting other people and eventually they want to hurt us back. And this is very, very terrible. This is killing the culture. This is killing the spirit. So it cannot be personal. If you see that something's personal, it has to be managed. It's not a code review issue. It's your team issue. You have to sit down and talk about it. In some cases, sorry, I have to drink too much talking. In some cases, people have issue with domain expertise. So imagine you're working on the product and only one guy knows how the payment system works. Come on, I'm sure you all have examples of this, right? So what happens when you need to touch the payment system? You either ask that guy, and when he does the change, nobody wants to review it because nobody knows how it works, or you're doing the change. And the only guy who's doing the code review is your domain expert, which is a bad position to be in because first of all, come on, buzz factor, right? The guy can move, can get sick, can go on vacation. Second of all, the code review is supposed to be open and everybody in the team is supposed to participate. So if you only have one guy knowing something, it's terrible and it already kills this idea. And again, code review is supposed to help you fight this problem, right? If I'm doing the change to the payment system and that guy comes in and crushes me, I'm actually learning. And everybody who's looking at this pull request, even if they don't have any comments, they can see my change, see the feedback from the guy who actually knows what the payment system is about, and they can learn. They can figure out how it works. So during the discussions, again, like there's no silver bullets, there's usually more than one way to do something. So we have to agree to disagree. There's more than one way to do stuff. And it's all about trade-offs. Sometimes you convince someone to change something, sometimes you don't, and that's okay. We learn as we go. So hopefully if you, let's say you have a junior engineer and he does something not very smart and you highlight this and he fixes this, you should be fine, right? Because hopefully the next time he does the same kind of change, he will already apply the knowledge that he got from the previous code review. If he doesn't, it's a different problem. It means maybe this person doesn't care, doesn't want to learn or can't learn. Again, it's not a code review issue anymore. And yeah, we learn by asking questions. If you see something and the code is perfect, it doesn't mean that that's it. You can still ask questions to have a better understanding, to ask anything. Seriously, I try to keep asking questions. It doesn't actually have to be a comment in the pull request. You can just walk to that person and say, like, hey, I actually enjoy your pull request, but I'm not sure I fully understood this part. Maybe we can walk through it, just talk about it. And yeah, if you ask questions you learn, but if someone asks you question and you answer, you share your knowledge, you're building a better team around yourself. So it's always a benefit. So of course, there are some complex cases and the things that I shared in this talk cannot cover everything. Of course, I'm sure there's a lot of cases in which this doesn't apply. But there are some complex cases which you can still deal with. So if you have a very, very complicated change and unfortunately you fail to split it to multiple pull requests or you fail to make the code change small and it's, as we said, like it's 50 files, 40 files of change, code review won't be effective. Even if people go through it and say it's a proof, usually people didn't actually pay much attention. It looks okay. So what you can do instead, you can do a code walkthrough. So you sit down in front of a computer and you actually go line by line explaining the workflow, not just looking at the code, explaining the whole workflow, people asking questions. And it's really cool and it's a really powerful thing. In-person communication versus flood of comments. Sometimes I receive an email every time there's a comment in pull request. So if I see there's a pull request and there's like more than, I don't know, 10 emails already, it means maybe we need to discuss this outside of pull request. Because again, written communication is terrible. A lot of people asking the same questions. It's not real time. So I can ask you a question. You respond in an hour. I already forgot why I was asking this, etc. So just taking everybody involved in this, spending 10 or 15 minutes talking to them is usually way more efficient than continue this huge discussion. And again, maybe this PR is too big and this is where your issues are coming from. So try to keep it as small as possible. In other cases, you might have suggestions or questions which are not actually in the scope of this ticket. Right? We cannot fix the whole system. We always try to produce the best possible code, but we cannot fix the whole system in one pull request. So sometimes if your pull request is touching too many parts of your system, people start suggesting some changes which are not actually in the scope of this ticket. Maybe they're suggesting the refactoring that can be introduced later on. You can just fill in additional tickets for this refactoring. Thanks. So red flags are just things that would highlight that code review doesn't work in your team for some reason. So people committing straight to master. I don't think I need to explain that this is a bad idea. Ignoring comments on pull request, unfortunately this happens. People might just ignore your comment or your question and just merge right away. A poor communication in pull request, meaning that it's mostly just negative feedback and I'm suggesting something and the person just responds, no. This is a very poor communication, right? The person might have a great idea and great reasons why he doesn't want to continue this or why he doesn't want to take a look at my approach, but by not expressing it explicitly you are just killing all the communication efforts. So code review is a team effort, right? We want everybody to be involved. It's not a one main game. If it's a one main game then we are losing pretty much at least half of the benefits. We're not getting the sharing of the knowledge, etc. So it's a team thing. Everybody should be engaged and this is why I said that if you're a solo developer or your team is too big, those things wouldn't apply because you cannot expect the team of 25 engineers to all participate in code review. It would be just crazy. Code review is a team responsibility and by this I mean that if you do the code review and it actually passes, it means that it's a team decision. If I'm reviewing and I'm emerging, this is my decision. If the whole team is reviewing, it's a team decision and the difference is very, very important. If something goes bad, if something turns out to not work or breaking production, there is no fingering anymore because we all did this decision. We all participated. We all had our chance to express what do we feel about this code. So as a result, the team owns this code, not the pull request author. This one is very hard because most people would never say yes to this. Most people would say like, I don't like his code. I don't want to be responsible for his code. But again, this is not the issue of code review. This is the issue of your team members and communication. This is a team sport, so the best case scenario, everybody's involved, everybody commit, everybody's responsible. If something goes wrong, everybody just jump in and fix it. There is no fingering and saying like, this is your issue. I don't care about this. So as a summary, sorry, I didn't expect this to take so long. We have to try and learn to understand each other. We have to respect each other. This is again about negative and positive feedback. We have to respect each other and we have to help our team. By helping our team, the team is helping you. Code review is a team sport. Thank you. Do we have some time for questions? Okay, one question. Two questions. Sorry, that guy was first. Go on. Okay, cool. So I think we need to cover the topic of having PR and speech. So yeah, obviously my team has the same issues and what happens is one person will get it and the other person will get someone else to say, okay, we will come in. So I think we need to cover one of the ways to look at such PR is to review the work together to walk through. I'm just wondering if we have other and I'm sure this is something unavoidable. We are bound to be some teachers who are so huge and we have to think about it. Okay. Let me repeat the question because I'm not sure everybody would hear it. So the question is what do you do when unfortunately your PR is too big and people are struggling to review it? So if you already are in this situation, I would say it's too late to change anything. So this is something that you have to do during the planning. When you are planning this feature, when you even start thinking about it, you have to think is it possible to split it into the chunks? Is it possible to implement this in phases? What would be the minimum possible and valuable piece of software that I can deliver that would produce some value without building the whole thing in one go? If you have to build the whole thing in one go, then again, sometimes what we would do, we would do PRs for other PRs. So you create a feature branch and let's say you have to change, I don't know, you have to do database changes and your API changes and some business logic changes. So you create a feature branch and then you only do the DB changes. You create the PR against the feature branch, not your master or development. Then on top of that, another person can do a change for API, another pull request, another pull request for something else. So you kind of artificially splitting the work, even though it's not deliverable as a small piece, but you split the PR into three phases. It's really tricky because usually it means that when the first PR is merged, you have to rebase all the other branches to take those changes, etc. So it's not a very nice and convenient way, but at least it's something. So sometimes we would do this. We would actually create multiple PRs against the feature branch and build it step by step. But yeah, try to fix this issue during the planning. If it's possible, try to plan ahead. You can actually see, oh my god, it's going to be huge and whenever we get to the testing of this and code review, it's going to be just unmanageable. Thank you, Mike. So we're running short of time. Yeah, sure. If you have any questions, just approach me after this.