Conf42 Site Reliability Engineering (SRE) 2024 - Online

Code Review: Best Practices, Tips or How To Not Drive Your Colleagues Into Depression With Your Comments

Video size:

Abstract

In this lightning talk I’m going to cover why code review is crucial to successful development of your project. We are going to go over different approaches to code review and see when and which make more sense. I will also touch on how to adjust review style to different levels of colleagues’ seniority. And finally, I’m going to share some tips about how to make code review successful and impactful - all of this without dealing severe emotional damage to your colleagues.

Summary

  • Code review is just review of codes written by the colleagues. More eyes on code means more chances to catch potential bugs. Alternatively, code review can also mean sharing best practices within the project or even the company.
  • If you want to really focus, it may be helpful to check out the authors branch locally on your machine and then examine each individual file. One of my favorite suggestions is really, really adapt your comment style. Beware of emotional damage if your code review doesn't have enough space for all of your comments.

Transcript

This transcript was autogenerated. To make changes, submit a PR.
Hi everyone. Welcome to my talk about code review. Alternatively, best practices tips and how to not drive your colleagues into depression with your comments. And without further ado, let's get started. I'm pretty sure that most of you know what exactly is code review. But just to reiterate on what exactly we're talking about here, code review is just review of codes written by the colleagues. It's as simple as that. Other colleagues write changes to the code, they publish them, they suggest these changes, and then we comment on these changes. Or we just approve without comments, depending on whether we like the code or not. Do we even need code review actually, or why do we need it? Because, well, developers could be doing something else, right? Instead of arguing comments, checking other people's code, they could be already spending time on writing new features. So maybe we don't need it at all. Well, first of all, more eyes on code means more chances to catch potential bugs or things like privacy and security issues, right? It's like additional layer of testing, if you may. If we don't catch those on time and we actually release the code, it may result in potential production downtime. It may result in regulator problems or even ruined customer trust. So from this point of view, we are interested in catching bugs as early as possible, which means maybe during review stage. Alternatively, code review can also mean sharing best practices within the project or even the company. One programmer can comment on something suggests another approach which is used, and this ensures that another developer or a group of developers know the code styles. And this can even actually help developers progress professionally if they get some comments about an approach, for example, which they hadn't known before. Finally, code review can stop the unintended workflows. For example, usually one developer should not be able to push to production and verify changes, and code review may act as a security layer if someone tries to do something bad, intentionally or unintentionally. Hopefully, code review may just add a little bit more confidence that this will be caught early. So now let's talk a little bit about best practices, which I think are appropriate in code review. First of all, regarding a detailed review, if you want to really focus and you want to make sure that you know and you go thoroughly through authors changes, it may be helpful to check out the authors branch locally on your machine and then examine each individual file. I'm saying this because locally you will usually have your favorite code editor, which can help you identify syntax errors, and also you can navigate quickly between files to see the connections. And all of that may really help you maintain the context during code review. Also, sometimes, for example, when I need to really attentively review the code, I book a slot in my calendar. I put on headphones. Basically, I tried to ensure that there are no distractions so that I could really familiarize myself with the context and stay focused. As opposed to that, sometimes it makes sense to review just syntax and obvious syntax bias without getting into the whole business logic. I would say this is appropriate if you get multiple merge requests per day, and then you may just not have the time to actually go through each of the merge requests individually, checking them out as I discussed earlier. Just I would say that make sure that the author knows that you are not reviewing the deep logic, the business logic, but you are mostly reviewing all the syntax. Now one of my favorite suggestions is really, really adapt your comment style. For example, some comment like this may be appropriate for a more junior person, or the person who has been working in your project recently, or just a sensitive person overall, it's a very polite suggestion that hey, can yuri please remove this serializable term? And then I explain the reason why was it needed and then why it is not needed. So you try to be very polite, you give all the context and just basically make sure that person is on board with you and doesn't feel anything any bad things. However, if someone is more senior experienced or you just know, they will understand exactly what you are saying. You can just write something that's not needed and it will be a pointer for them that this is not needed and they will remove it. And maybe they already have the context, or they will ask you for the context if they need to. But sometimes this is enough. So this is up to you to understand which type of person or which type of style best fits in the given situation. Beware of emotional damage. Really, if you put too many comments, I understand we want to comment on everything. Sometimes there are so many things which can be done differently, but it still can be overwhelming even for very experienced developers. Like speaking of my experience, I have more than ten years and still when I receive a review with like, I don't know, ten comments or more, my first reaction is to, I don't know, to try to protect myself, to try to respond back and so on. So I would say try keeping only the most significant ones and maybe move the rest offline. If you really think that you should write more that your code review doesn't have enough space for all of your comments, because the goal is eventually to encourage positive approach and to be productive and the developer may risk in not being productive if they receive too many comments. So yeah, try keeping just the most significant ones. And if you still need to put more, just move the rest of line. When you like something, please write about it as well. We don't only need to comment on whatever needs to be changed. Well, mostly, yes, that's what we do. But also, it's so nice to feel a little bit of gratitude. Right? Like you can write very tiny comments like I like this or thank you, great job. And as you see, I get hearts for these comments. So it may make a developer smile, it may bring up their mood. And happy developer means, after all, more productive developer, right? And also with a happy developer, it's more pleasant to work and we may enjoy that more. So yeah, please try to not always be negative. Try to also emphasize on the good things which you see. Also, I would say, don't argue back and forth in comments. Again, it's kind of the same as when you leave too many comments. People tend to get protective, and especially because it's a public space. Culture view is a public space which can be usually viewed by many developers, they might get the feeling of being publicly attacked. Yes, ideally, if we were all logical machines, we just only consume the logic of your comment. We would never actually, I don't know, emotionally react. But the reality is that we are not. We are all people, we all have our own issues. You never know which issues are there. So maybe you could help preventing any triggers by just following these tips. Like for example, it doesn't make sense to argue back and forth. If he wrote a comment and then developer replies and doesn't agree, and then you write back and then developer writes back, it eventually becomes a battle of egos, like whose comment is going to be the last? Who is going to win? I would say if that's not the goal, if the goal is to actually resolve the issue, just move the conversation offline. If you see that the developer doesn't agree. Okay, what is your goal here? Do you still want to push for that? If yes, sure. Maybe culture view is not the best place anymore to push for it. Maybe you just moved to slack channel or moved to oral conversation or jaspeak any other way of talking. Just one thing to add here. I would say that if you do this and eventually they should get resolved offline in the Koji view, you could make a comment that something like discussed offline or resolved offline so that if other developers come back there and they see that you had kind of disagreement and they don't know what was the result. At least they could see that hey, it was discussed applying, so if they really are interested in knowing whatever happens in the end they can actually find you and ask you what was the outcome. And that is all for me. Thank you very much for attending this lightning talk. I really appreciate this. I wish you all very happy coding, very nice review comments and yeah, just wishing you to be very relaxed and productive during your work days.
...

German Urikh

Senior Software Engineer

German Urikh's LinkedIn account



Awesome tech events for

Priority access to all content

Video hallway track

Community chat

Exclusive promotions and giveaways