1 00:00:00,120 --> 00:00:03,560 Hey, a quick message from me. Thank you for being part of the 2 00:00:03,560 --> 00:00:06,800 Techledjunal community. This show wouldn't be the same 3 00:00:06,800 --> 00:00:10,280 without your ears and you are the reason this show exists. 4 00:00:10,960 --> 00:00:14,440 Creating this podcast is a labor of love, but the truth is it 5 00:00:14,440 --> 00:00:18,160 also takes time, resources, a whole lot of passion, and an 6 00:00:18,160 --> 00:00:22,520 extra bit of caffeine. So if you're loving TLJ and want 7 00:00:22,520 --> 00:00:25,880 to see it keep on growing, consider becoming a patron at 8 00:00:25,880 --> 00:00:30,200 techledjunal dot dev patron or buying me a coffee at techly 9 00:00:30,200 --> 00:00:34,640 journal dot DAV coffee. Every little bit helps field the 10 00:00:34,640 --> 00:00:38,240 research, editing, and sleepless nights that go into making this 11 00:00:38,240 --> 00:00:41,440 show the best it can be. Thanks for being the best 12 00:00:41,440 --> 00:00:43,640 listeners any podcast could ask for. 13 00:00:44,080 --> 00:00:47,720 A lot of developers tend to tie their self worth to their code. 14 00:00:47,760 --> 00:00:51,560 Sometimes we feel like our code is an extension of ourselves. 15 00:00:51,760 --> 00:00:56,800 So being able to let go of your ego and understanding that if 16 00:00:56,800 --> 00:01:00,920 we're asking the reviewers to objectively review our code, the 17 00:01:00,960 --> 00:01:05,040 feedback is based on the code as I have presented it has nothing 18 00:01:05,040 --> 00:01:08,440 to do with me, has nothing to do with anything about me. 19 00:01:08,440 --> 00:01:16,480 It's just the code. Hey everyone, my name is Henry 20 00:01:16,480 --> 00:01:20,600 Surya Virawan and you're listening to the Technically 21 00:01:20,600 --> 00:01:23,840 Journal Podcast, the show where I'll be bringing you the 22 00:01:23,840 --> 00:01:26,960 greatest technical leaders, practitioners and thought 23 00:01:26,960 --> 00:01:30,360 leaders in the industry to discuss about their journey, 24 00:01:30,640 --> 00:01:35,120 ideas and practices that we all can learn and apply to build a 25 00:01:35,120 --> 00:01:38,680 highly performing technical team and to make an impact in your 26 00:01:38,680 --> 00:01:41,880 personal work. So let's dive into our journal. 27 00:01:46,840 --> 00:01:49,160 Hello to all of you, my friends, and my listeners. 28 00:01:49,440 --> 00:01:53,080 Happy New Year 2024. This is our first episode of the 29 00:01:53,080 --> 00:01:56,320 year and welcome back to the Tech Lead Journal Podcast, a 30 00:01:56,320 --> 00:01:58,920 podcast on technical leadership and excellence. 31 00:01:59,480 --> 00:02:02,560 If you haven't, please subscribe on your favorite podcast app to 32 00:02:02,560 --> 00:02:06,200 get notified for new episodes. Tech Lead Journal also provides 33 00:02:06,200 --> 00:02:10,400 bite sized contents on LinkedIn, X, Instagram, YouTube, and 34 00:02:10,400 --> 00:02:14,040 Tiktok. My guest for today's episode is 35 00:02:14,120 --> 00:02:17,600 Adrienne Tag. Adrienne is a software engineer, 36 00:02:17,800 --> 00:02:21,440 keynote speaker and the author of the upcoming book looks good 37 00:02:21,440 --> 00:02:24,200 to me. In this episode, we discussed 38 00:02:24,200 --> 00:02:27,280 code reviews and why it is an essential part of the software 39 00:02:27,280 --> 00:02:30,560 development process. Adrian discusses the importance 40 00:02:30,560 --> 00:02:34,600 and benefits of code review, the common code review workflow, and 41 00:02:34,600 --> 00:02:37,920 the different roles involved, how to provide effective code 42 00:02:37,920 --> 00:02:41,160 review commands, and why we should leverage on code review 43 00:02:41,160 --> 00:02:44,800 tools and automation. She also provides tips on how to 44 00:02:44,800 --> 00:02:47,080 speed up our code review turn around time. 45 00:02:47,880 --> 00:02:50,520 I hope you enjoy listening to this episode and learning best 46 00:02:50,520 --> 00:02:54,120 practices on doing code reviews. Remember to share it with your 47 00:02:54,120 --> 00:02:56,840 colleagues, friends and communities and leave a faster 48 00:02:56,840 --> 00:03:00,000 rating and review on Apple Podcasts and Spotify. 49 00:03:00,680 --> 00:03:03,360 Let's now go to my conversation with Adrienne. 50 00:03:06,240 --> 00:03:07,840 Hey everyone, welcome back to the New. 51 00:03:07,840 --> 00:03:09,600 Episode of the technicianal podcast. 52 00:03:09,600 --> 00:03:11,320 Today I have with me Adrienne Tech. 53 00:03:11,520 --> 00:03:14,160 She is actually writing a book, which is something that I look 54 00:03:14,160 --> 00:03:17,360 forward to read, which is titled Looks good to me. 55 00:03:17,800 --> 00:03:20,560 I mean, if you know this term right, I'm sure you can 56 00:03:20,560 --> 00:03:23,680 associate that with Code Review. Maybe we'll ask her what does 57 00:03:23,680 --> 00:03:26,800 look good to me means. So, Adrian, welcome to the show. 58 00:03:27,520 --> 00:03:29,880 Thank you, Henry. Thank you so much for having me. 59 00:03:29,880 --> 00:03:31,800 I'm excited to be here. Right. 60 00:03:32,160 --> 00:03:34,920 Adrian, I'd like to ask you to share a bit about yourself, 61 00:03:34,920 --> 00:03:37,680 right if you have any career highlights or turning points 62 00:03:37,680 --> 00:03:39,880 that we can all learn from. Sure. 63 00:03:39,880 --> 00:03:44,080 I think the top three highlights I'll share was in the beginning. 64 00:03:44,080 --> 00:03:49,240 I was never really introduced to STEM as most people would, so I 65 00:03:49,240 --> 00:03:53,320 didn't really have any family members who coached me or 66 00:03:53,480 --> 00:03:56,080 exposed me to that. My only exposure was through 67 00:03:56,080 --> 00:04:00,040 playing my own computer games, rather like RCT Two or Age of 68 00:04:00,040 --> 00:04:03,000 Empires 2. And that's a very important 69 00:04:03,000 --> 00:04:05,960 thing I like to distinguish because a lot of people think 70 00:04:05,960 --> 00:04:10,160 that getting into a career, you must have had this love for STEM 71 00:04:10,400 --> 00:04:14,080 or coding or doing these types of things from a very early age. 72 00:04:14,360 --> 00:04:19,240 And while that's helpful for me, I didn't know that I wanted to 73 00:04:19,279 --> 00:04:23,440 be in this type of career until college when I was a student 74 00:04:23,440 --> 00:04:25,560 help desk technician. So that's probably the first 75 00:04:25,560 --> 00:04:29,000 turning point was I didn't even get that job because I wanted 76 00:04:29,000 --> 00:04:31,000 to. It was the highest paying job at 77 00:04:31,000 --> 00:04:34,960 the time for university and it was only there where you would 78 00:04:34,960 --> 00:04:38,640 problem solve kind of help people break down their problems 79 00:04:38,640 --> 00:04:42,240 and try to help them was where I said huh, this might be an Ave. 80 00:04:42,240 --> 00:04:45,920 that I want to go into. Fast forward to probably middle 81 00:04:45,920 --> 00:04:50,080 of my career. I was a.net developer for small 82 00:04:50,080 --> 00:04:53,680 companies here around Las Vegas where I was based and this is 83 00:04:53,680 --> 00:04:57,280 where I started to learn that not a lot of people dressed like 84 00:04:57,280 --> 00:04:58,920 me. So, you know, you have this 85 00:04:58,920 --> 00:05:03,600 stereotype where it's like, you know, you dress casually or like 86 00:05:03,600 --> 00:05:06,520 tech people don't like to dress up. 87 00:05:06,520 --> 00:05:09,600 I don't know. That's probably false now, but 88 00:05:09,600 --> 00:05:12,800 at least at the time during my career, nobody dressed up in 89 00:05:12,800 --> 00:05:15,480 heels, Nobody wore dresses, No one wore skirts. 90 00:05:15,480 --> 00:05:17,640 No one did their hair like I did. 91 00:05:17,640 --> 00:05:21,480 And so that was a big turning point for me to kind of share. 92 00:05:21,480 --> 00:05:24,800 And so I started to share that on an Instagram account where I 93 00:05:24,960 --> 00:05:28,840 actually grew fairly large without intending to, but I 94 00:05:28,840 --> 00:05:32,160 wanted to show other women specifically and other Filipino 95 00:05:32,160 --> 00:05:36,040 women because there's kind of this notion and push to go into 96 00:05:36,040 --> 00:05:38,840 the medical field of, hey, there's actually a different 97 00:05:38,840 --> 00:05:42,160 Ave. for you to go into. So that was a big part, was to 98 00:05:42,160 --> 00:05:46,040 share that you can be feminine and be technical and you can go 99 00:05:46,040 --> 00:05:49,640 into the technology field if you don't want to go into the 100 00:05:49,640 --> 00:05:52,480 medical field like I didn't. And then the last turning point 101 00:05:52,480 --> 00:05:55,000 I'll share was later in my career. 102 00:05:55,200 --> 00:05:58,480 I went to a technical conference and I was listening to a talk, 103 00:05:58,520 --> 00:06:02,120 and the person who is sharing or giving the talk was really, 104 00:06:02,120 --> 00:06:04,680 really bad. I mean, I wasn't really learning 105 00:06:04,680 --> 00:06:06,840 anything. The slides were just bullet 106 00:06:06,840 --> 00:06:09,160 points. I I started to wonder, how do 107 00:06:09,160 --> 00:06:12,440 you actually get up onto these stages, 'cause I felt like I 108 00:06:12,440 --> 00:06:15,320 could do a better job of presenting than the current 109 00:06:15,320 --> 00:06:17,960 person I was watching. And so that's when I kind of 110 00:06:17,960 --> 00:06:22,120 learned, oh, there's this whole process of applying, submitting 111 00:06:22,120 --> 00:06:25,800 talk proposals to these conferences, organizers choose 112 00:06:26,000 --> 00:06:29,120 the best ones that fit their program, and then that's how you 113 00:06:29,120 --> 00:06:31,080 actually can become a tech speaker. 114 00:06:31,360 --> 00:06:35,160 And so that was a very big turning point because I was 115 00:06:35,160 --> 00:06:37,000 like, OK, let me just try this out. 116 00:06:37,000 --> 00:06:40,520 And as a new person, someone who's never been heard of on the 117 00:06:40,520 --> 00:06:44,520 speaking stage, the advice they tell you is to apply to as many 118 00:06:44,520 --> 00:06:46,080 as you can. Because the, you know, if you 119 00:06:46,080 --> 00:06:49,080 get rejected a lot of times, at least you have more chances. 120 00:06:49,320 --> 00:06:52,000 Well, at that first time, I actually got accepted to 7 121 00:06:52,000 --> 00:06:54,320 conferences. And so that was very, very 122 00:06:54,320 --> 00:06:56,520 interesting. And that's kind of gotten me to 123 00:06:56,520 --> 00:06:59,080 where I am today as a senior developer advocate. 124 00:06:59,320 --> 00:07:04,200 I wasn't expecting to transition into developer relations, nor 125 00:07:04,200 --> 00:07:08,840 was I expecting to be, you know, to actually really like speaking 126 00:07:08,840 --> 00:07:11,000 on stage. I'm usually very quiet and 127 00:07:11,000 --> 00:07:14,760 introvert, don't like talking to people at cocktail parties, so 128 00:07:15,000 --> 00:07:17,520 to get up on stage is something that's very different for me, 129 00:07:17,520 --> 00:07:18,880 but I actually really, really love it. 130 00:07:19,600 --> 00:07:21,040 Thank you for sharing your story. 131 00:07:21,080 --> 00:07:23,880 I think it's really interesting. You highlighted 3 different 132 00:07:23,880 --> 00:07:27,120 things right, which I think many engineers could actually relate 133 00:07:27,160 --> 00:07:28,720 to. I think the key learning for me 134 00:07:28,720 --> 00:07:31,600 is not to be afraid to try something different, something 135 00:07:31,600 --> 00:07:33,800 new, right? Even though something you didn't 136 00:07:33,800 --> 00:07:36,240 have a background. So take the step and probably 137 00:07:36,240 --> 00:07:38,800 look at you now, right? It's partly your main life now. 138 00:07:39,320 --> 00:07:41,760 This episode is brought to you by Miro. 139 00:07:42,200 --> 00:07:44,680 Collaborative development is one of the crucial. 140 00:07:44,680 --> 00:07:48,920 Aspects of Software Development and Code Review is one such core 141 00:07:48,920 --> 00:07:51,720 activity that can benefit from a good collaboration. 142 00:07:52,240 --> 00:07:54,960 As you will learn in this episode later about Code Review 143 00:07:54,960 --> 00:07:58,160 best practices, finding a collaboration tool that can 144 00:07:58,160 --> 00:08:02,280 assist code review process is extremely useful, especially if 145 00:08:02,280 --> 00:08:05,440 it can be done in a fun, engaging and collaborative way. 146 00:08:06,160 --> 00:08:09,160 One use case that I find mirror extremely helpful for code 147 00:08:09,160 --> 00:08:12,040 review is for diagramming and process mapping. 148 00:08:12,600 --> 00:08:16,080 You can use it for any of your technical diagrams such as Class 149 00:08:16,080 --> 00:08:19,800 Diagram, Sequence Diagram, Infrastructure and Architecture 150 00:08:19,800 --> 00:08:23,160 Diagram, Data Modeling Diagram and so many more. 151 00:08:23,760 --> 00:08:27,320 With a lot of readily available predefined templates, you can 152 00:08:27,320 --> 00:08:29,560 kickstart your collaboration within seconds. 153 00:08:30,360 --> 00:08:33,840 It also has support for popular diagram as code tools such as 154 00:08:33,840 --> 00:08:38,000 mermaid and plant UML. Explaining complex ideas and 155 00:08:38,000 --> 00:08:42,320 systems becomes much easier now and as a result it will lead to 156 00:08:42,320 --> 00:08:45,880 a higher quality work and most importantly more engaged and 157 00:08:45,880 --> 00:08:47,680 productive software development teams. 158 00:08:48,560 --> 00:08:52,400 Find simplicity in your most complex projects with mirror 159 00:08:52,960 --> 00:08:56,120 Your first three mirror boards are free when you Sign up today 160 00:08:56,120 --> 00:09:00,440 at mirror.com Podcast. That's three freeboards at 161 00:09:00,560 --> 00:09:05,360 miro.com/podcast. And now let's get back to our 162 00:09:05,360 --> 00:09:08,400 episode. So today we are gonna cover this 163 00:09:08,400 --> 00:09:10,480 book title. Looks good to me, which you're 164 00:09:10,480 --> 00:09:13,160 still writing, right? It's a book about code review 165 00:09:13,400 --> 00:09:15,320 first of all, maybe a bit of background. 166 00:09:15,320 --> 00:09:18,920 What is looks good to me or in some company they call it LGTM 167 00:09:19,040 --> 00:09:20,520 and why you are writing this book? 168 00:09:21,240 --> 00:09:25,120 Great question. So LGTM, the infamous acronym, 169 00:09:25,120 --> 00:09:28,000 or some people use the thumbs up emoji, right? 170 00:09:28,000 --> 00:09:31,480 With that. This is usually used to say 171 00:09:31,600 --> 00:09:35,680 everything looks, you know, OK, it's good enough, it passes 172 00:09:35,680 --> 00:09:37,880 muster. It'll be just fine. 173 00:09:38,160 --> 00:09:42,640 And this notion has kind of devolved, I think, into 174 00:09:42,640 --> 00:09:47,520 something that's not so good as a software developer who goes 175 00:09:47,520 --> 00:09:51,520 through code review and reviews other people's code or, you 176 00:09:51,520 --> 00:09:54,280 know, puts up their own code changes to be reviewed. 177 00:09:54,760 --> 00:09:58,320 There's a lot of things that have just become really, really 178 00:09:58,480 --> 00:10:01,520 mundane or unmanageable about the process. 179 00:10:01,880 --> 00:10:05,720 And that's really sad to me because this process is a very 180 00:10:05,720 --> 00:10:10,680 key part of software development and our jobs as software 181 00:10:10,680 --> 00:10:13,760 developers. So code review for anyone that 182 00:10:13,760 --> 00:10:17,080 might be unfamiliar, is it's this very, I'll start with a 183 00:10:17,080 --> 00:10:21,800 very basic workflow of you have code changes that you create. 184 00:10:21,800 --> 00:10:24,320 Usually we call this the author of the code changes. 185 00:10:24,680 --> 00:10:28,840 Once they're ready to have their code reviewed, they typically 186 00:10:28,840 --> 00:10:32,040 put it up in what's called a pull request or a merge request 187 00:10:32,040 --> 00:10:36,040 or a change request and they prepare it for someone else, a 188 00:10:36,040 --> 00:10:40,120 reviewer, to look at. And then this review process can 189 00:10:40,320 --> 00:10:42,640 take many forms. Sometimes it's tool based. 190 00:10:42,640 --> 00:10:45,720 A lot of people are familiar with the GitHub based thing 191 00:10:45,720 --> 00:10:47,920 where you put up a pull request and it's asynchronous. 192 00:10:47,920 --> 00:10:52,040 Someone looks at it online on a tool and there's a code DIF that 193 00:10:52,040 --> 00:10:55,240 shows the differences between the changes and what has been 194 00:10:55,240 --> 00:10:57,560 done. Sometimes people like to 195 00:10:57,560 --> 00:10:59,720 consider pair programming sessions. 196 00:10:59,720 --> 00:11:04,200 Also a code review where you may write some code and someone 197 00:11:04,200 --> 00:11:07,160 looks over your shoulder and reviews it at that time. 198 00:11:07,760 --> 00:11:10,400 Now review. This phase can mean many 199 00:11:10,400 --> 00:11:13,440 different things and this is kind of what I go into in my 200 00:11:13,440 --> 00:11:15,320 book too. But you know, you have different 201 00:11:15,320 --> 00:11:17,880 goals as a team when you do the review part. 202 00:11:18,120 --> 00:11:21,400 Sometimes it's looking to make sure code looks correct, 203 00:11:21,840 --> 00:11:25,640 sometimes it's seeing, does this match the rest of our code base? 204 00:11:25,640 --> 00:11:29,040 Will this improve the health of our code base? 205 00:11:29,040 --> 00:11:31,800 Is there anything outstanding that's missing? 206 00:11:32,040 --> 00:11:36,320 And are there things that we can learn from the changes, like why 207 00:11:36,320 --> 00:11:40,320 are we making these changes at this time and kind of getting 208 00:11:40,320 --> 00:11:43,960 the rest of the team on board as to what's changing in the code 209 00:11:43,960 --> 00:11:46,240 base? And once that's done, once, a 210 00:11:46,240 --> 00:11:49,480 couple people, ideally, other than the author, the one who 211 00:11:49,560 --> 00:11:51,960 actually wrote the changes, take a look at it. 212 00:11:51,960 --> 00:11:54,640 That's when you get that infamous looks good to me, you 213 00:11:54,640 --> 00:11:58,320 know, thumbs up, an approval that says, yay, this is OK, and 214 00:11:58,320 --> 00:12:00,720 then it goes on and merges into the rest of the code base. 215 00:12:00,720 --> 00:12:04,400 So very simplified at this point what I've just explained, but 216 00:12:04,400 --> 00:12:07,640 it's kind of the basis of what a code review is, and you'd be 217 00:12:07,640 --> 00:12:10,840 surprised at how many things can go wrong in that process. 218 00:12:11,920 --> 00:12:13,640 Right. I'm sure all developers 219 00:12:13,640 --> 00:12:15,000 understand what you're saying, right? 220 00:12:15,000 --> 00:12:17,720 About getting it wrong, about getting it mundane right. 221 00:12:18,040 --> 00:12:20,360 So first of all I think I just want to clarify, You mentioned a 222 00:12:20,360 --> 00:12:23,000 few things that which I think has been associated with code 223 00:12:23,000 --> 00:12:24,920 review workflow for quite some time, right? 224 00:12:24,920 --> 00:12:28,200 The 1st is about PR, Mr. or CR right? 225 00:12:28,560 --> 00:12:31,320 When you set it can also be done through pair programming right? 226 00:12:31,320 --> 00:12:33,800 It doesn't mean that code review can only go through you know, 227 00:12:33,800 --> 00:12:36,000 pull request, merge request, change request kind of 228 00:12:36,000 --> 00:12:38,600 mechanism. And the other thing is I think 229 00:12:38,640 --> 00:12:42,360 about the process of merging. So we are assuming people are 230 00:12:42,360 --> 00:12:45,760 using version control of course, and a different kind of a branch 231 00:12:45,880 --> 00:12:49,960 before they can merge. Any kind of maybe fun stories or 232 00:12:49,960 --> 00:12:53,040 horror stories that you have experienced with code reviews 233 00:12:53,040 --> 00:12:55,200 that you can share with us? Yes. 234 00:12:55,200 --> 00:13:00,800 So my very first experience was actually with Team Foundation 235 00:13:00,800 --> 00:13:05,040 Server, dating myself a bit, but this was Visual Studios and 236 00:13:05,040 --> 00:13:08,840 Azure, but Microsoft's way of version control, and this was 237 00:13:08,840 --> 00:13:13,000 still when it was kind of new, people were not really big on 238 00:13:13,000 --> 00:13:16,960 version control yet, but it was the system where you would kind 239 00:13:16,960 --> 00:13:20,680 of check out a file and you could work on it. 240 00:13:20,720 --> 00:13:23,920 And the notion was that kind of like checking out books in the 241 00:13:23,920 --> 00:13:26,320 library. When you check it out, you have 242 00:13:26,320 --> 00:13:29,360 your changes, you work on them and then you're able to check it 243 00:13:29,360 --> 00:13:33,120 back in. And this checking in process, at 244 00:13:33,120 --> 00:13:36,080 least in the team that I worked on, we didn't really have a 245 00:13:36,080 --> 00:13:38,800 review process. You kind of just assumed 246 00:13:38,800 --> 00:13:42,960 everyone that was checking out their things were reviewing it 247 00:13:42,960 --> 00:13:48,240 themselves and kind of gave them the accountability that, hey, 248 00:13:48,240 --> 00:13:51,720 anything you are checking in is on you, right? 249 00:13:51,720 --> 00:13:54,440 We hope that anything that you check back into the main project 250 00:13:54,440 --> 00:13:56,680 code base is good and that there's nothing wrong. 251 00:13:57,160 --> 00:14:01,400 So that was my first experience dealing with, well, what happens 252 00:14:01,400 --> 00:14:05,720 when somebody checks it out at the same time or there's a split 253 00:14:05,720 --> 00:14:08,680 difference where they may have overwritten something that was 254 00:14:08,680 --> 00:14:12,320 just checked in or, you know, people don't have that 255 00:14:12,320 --> 00:14:15,080 accountability and they just kind of check in their own 256 00:14:15,080 --> 00:14:17,080 changes and you don't know that it's bad. 257 00:14:17,400 --> 00:14:20,920 This is kind of the beginnings, the foundations of where I'm 258 00:14:20,920 --> 00:14:24,600 like, why isn't there some sort of process where we kind of take 259 00:14:24,600 --> 00:14:29,360 a look or just hold on for a second before we are sure, Let's 260 00:14:29,360 --> 00:14:31,280 take a look at what's about to go in. 261 00:14:31,520 --> 00:14:36,120 And I knew at that time I was very afraid to break things, so 262 00:14:36,120 --> 00:14:38,440 I was extra careful. I was like double checking, 263 00:14:38,440 --> 00:14:42,520 triple checking, and it was really hard to learn that not a 264 00:14:42,520 --> 00:14:45,640 lot of people are like that. Some people are just like, yeah, 265 00:14:45,640 --> 00:14:48,120 let me just put it in and if it breaks, it breaks. 266 00:14:48,120 --> 00:14:49,800 And I didn't have that mentality. 267 00:14:50,040 --> 00:14:53,600 So it was very difficult to experience that first stand of 268 00:14:53,600 --> 00:14:56,600 what happens when. In that sense, it was kind of 269 00:14:56,600 --> 00:15:00,840 like a merge conflict where the same file was changed and you 270 00:15:00,840 --> 00:15:03,040 don't know what's proper, you don't know what to keep, you 271 00:15:03,040 --> 00:15:06,520 don't know what's most current. And yeah, that kind of started 272 00:15:06,520 --> 00:15:09,240 out the basic foundations in my career. 273 00:15:09,240 --> 00:15:12,040 I didn't know it yet at the time, and as I started to learn 274 00:15:12,040 --> 00:15:14,640 Git, those things crept up in my head. 275 00:15:14,640 --> 00:15:18,760 But yeah, that's where it kind of all started for me and got my 276 00:15:18,760 --> 00:15:22,240 mind spinning about why there is no such review process on my own 277 00:15:22,240 --> 00:15:24,840 teams. I think I must also say from the 278 00:15:24,840 --> 00:15:27,880 experience I had right, the kind of version control that you use 279 00:15:27,880 --> 00:15:30,480 will also determine this kind of a code review process, right? 280 00:15:30,720 --> 00:15:33,280 So I think mostly people are familiar with git now, which is 281 00:15:33,280 --> 00:15:35,520 kind of like optimized for this kind of workflow. 282 00:15:35,520 --> 00:15:38,400 So I think we should appreciate that Git exists, right? 283 00:15:38,720 --> 00:15:41,720 So in your book you come up with this statement, which I think 284 00:15:41,720 --> 00:15:43,800 really, really interesting for us to discuss, right? 285 00:15:43,800 --> 00:15:47,680 You mentioned that we need actually code review, but most 286 00:15:47,680 --> 00:15:51,000 of us actually dread it. Tell us why many people actually 287 00:15:51,000 --> 00:15:56,040 dread code review process? There are a lot of reasons over 288 00:15:56,080 --> 00:16:01,200 my career, over the different people I've spoken to, different 289 00:16:01,200 --> 00:16:04,640 developers. It mainly comes down to a couple 290 00:16:04,640 --> 00:16:07,440 things. One, the process is 291 00:16:07,520 --> 00:16:11,840 unsustainable. So what I mean by that is the 292 00:16:11,840 --> 00:16:14,440 process itself is just really cumbersome. 293 00:16:14,600 --> 00:16:19,440 There's too many steps or the PRS, the reviews themselves. 294 00:16:19,440 --> 00:16:21,360 There's too many files to look at. 295 00:16:21,360 --> 00:16:23,960 There's too many lines of code to review. 296 00:16:23,960 --> 00:16:28,640 That's way too large for any single person to properly look 297 00:16:28,640 --> 00:16:33,400 at and give a review. Sometimes there's no description 298 00:16:33,400 --> 00:16:36,240 or context that comes with the code changes. 299 00:16:36,240 --> 00:16:40,320 So as a reviewer, it's very difficult to make sense of what 300 00:16:40,320 --> 00:16:43,720 it is that you're trying to do. And then there are just 301 00:16:43,960 --> 00:16:48,200 processes where if you try to leave feedback, you don't know 302 00:16:48,200 --> 00:16:52,360 how to leave feedback, or the feedback is not helpful. 303 00:16:52,360 --> 00:16:54,880 It's something like, oh, this could be better, and then that's 304 00:16:54,880 --> 00:16:56,200 it. That's the only comment you get. 305 00:16:56,200 --> 00:16:58,520 And you're like, OK, what am I supposed to do with that? 306 00:16:58,520 --> 00:17:01,960 As an author receiving that comment, you have nowhere to go 307 00:17:01,960 --> 00:17:06,960 from there, so it's become unmanageable, unsustainable #2. 308 00:17:07,280 --> 00:17:11,520 In the more extreme cases, there are reviewers who kind of take 309 00:17:11,720 --> 00:17:15,760 advantage of the little power that they do have as a reviewer, 310 00:17:16,119 --> 00:17:18,720 and sometimes they just don't know how to write effective 311 00:17:18,720 --> 00:17:20,760 comments. O There are a couple of examples 312 00:17:20,760 --> 00:17:25,000 in my book where I share these are on public open source repos, 313 00:17:25,520 --> 00:17:27,599 and people are very harsh in their comments. 314 00:17:27,599 --> 00:17:30,640 They'll say, oh, this is a stupid way to implement this, or 315 00:17:30,840 --> 00:17:34,320 oh, this looks terrible, or you know this or that. 316 00:17:34,320 --> 00:17:38,440 And I always think for someone who's either new to software 317 00:17:38,440 --> 00:17:43,280 development or new to the team, or someone who's just unfamiliar 318 00:17:43,280 --> 00:17:46,560 with the actual technology but could be a seasoned developer. 319 00:17:46,880 --> 00:17:50,080 When you have this kind of culture of just being so harsh 320 00:17:50,080 --> 00:17:52,880 and not being able to give constructive feedback, that 321 00:17:52,880 --> 00:17:56,840 definitely does not cultivate a good developer experience. 322 00:17:57,040 --> 00:17:59,880 You know, there's another thing I say, which is a lot of 323 00:17:59,880 --> 00:18:03,320 developers tend to tie their self worth to their code, which 324 00:18:03,320 --> 00:18:05,440 we shouldn't do. I talk about that too. 325 00:18:05,720 --> 00:18:09,280 But you know, sometimes you do. And for those who haven't 326 00:18:09,280 --> 00:18:13,400 learned to uncouple themselves from their code, getting a 327 00:18:13,400 --> 00:18:16,760 comment that's that harsh or direct or just unnecessarily 328 00:18:16,760 --> 00:18:21,400 mean can really break their day can make it really unnecessary. 329 00:18:21,400 --> 00:18:24,680 So a code review that's unnecessarily mean is something 330 00:18:24,680 --> 00:18:27,400 people dread. And then the last part is just 331 00:18:27,720 --> 00:18:31,760 when you go through the process, sometimes it's so stringent. 332 00:18:31,760 --> 00:18:35,120 So there's like the lack of code review, which is a terrible 333 00:18:35,120 --> 00:18:37,200 thing, right? You get this wild, Wild West of 334 00:18:37,200 --> 00:18:38,800 everyone stepping on each other's toes. 335 00:18:39,240 --> 00:18:41,440 That's not so prevalent. Now, like you said, the Git is 336 00:18:41,440 --> 00:18:43,480 more common. And not to say we all use it 337 00:18:43,480 --> 00:18:46,720 properly, but it's there. And then there is the other side 338 00:18:46,720 --> 00:18:48,640 of the spectrum where it's too stringent. 339 00:18:48,640 --> 00:18:52,320 So now you're dreading the process because you have to get 340 00:18:52,320 --> 00:18:55,480 like this approval and you have to fill out these things and you 341 00:18:55,480 --> 00:18:57,720 have to do this and that and that. 342 00:18:58,000 --> 00:19:01,840 And you know, in my book I do say we're not supposed to think 343 00:19:01,840 --> 00:19:05,680 about it as a burden. There are certain things we have 344 00:19:05,680 --> 00:19:09,080 to do, like if we're opening a change, we need to make sure 345 00:19:09,080 --> 00:19:11,960 context is there, we need to link it, we need to put labels. 346 00:19:12,200 --> 00:19:14,760 But that's a bare minimum thing. And I'm talking about the 347 00:19:15,120 --> 00:19:18,800 processes where they have extra approvals, they have so many 348 00:19:18,800 --> 00:19:23,200 unnecessary steps that what do developers do when they, you 349 00:19:23,200 --> 00:19:26,000 know, are inconvenienced at any step they're going to go around 350 00:19:26,000 --> 00:19:28,920 it. So any process that is made much 351 00:19:28,920 --> 00:19:32,800 more difficult than it needs to be without any additional value, 352 00:19:33,320 --> 00:19:35,160 developers are going to find a way around that. 353 00:19:35,200 --> 00:19:40,520 And so that's what I've seen are a few ways why developers just 354 00:19:40,520 --> 00:19:43,200 dread the process. It's not as easy as it needs to 355 00:19:43,200 --> 00:19:45,520 be. It can be very harsh and mean 356 00:19:45,520 --> 00:19:49,640 when it doesn't have to be, and it is unmanageable when it 357 00:19:49,640 --> 00:19:53,160 should be manageable. And it should really reel in all 358 00:19:53,160 --> 00:19:54,880 of the benefits if it's done properly. 359 00:19:55,680 --> 00:19:57,640 I'm sure like I can relate to many of them. 360 00:19:57,760 --> 00:20:00,120 I think it also boils down in your book you mentioned it's a 361 00:20:00,120 --> 00:20:02,720 lack of communication and trust between team members. 362 00:20:02,720 --> 00:20:06,640 Eventually you know, like these mean comments or ineffective 363 00:20:06,640 --> 00:20:10,280 review commands or maybe even the delays and number of lines 364 00:20:10,280 --> 00:20:12,640 of code and number of files that you submitted, right? 365 00:20:13,000 --> 00:20:15,120 And also I think the lack of clarity about the process 366 00:20:15,120 --> 00:20:16,960 itself. I think we assume that everyone 367 00:20:16,960 --> 00:20:21,080 is expert in doing code review, which I think in my experience 368 00:20:21,080 --> 00:20:23,680 not everyone actually is an expert and we need. 369 00:20:23,960 --> 00:20:26,720 Some kind of resources like this book to actually upscale 370 00:20:26,720 --> 00:20:29,600 ourselves. So apart from the obvious, you 371 00:20:29,600 --> 00:20:32,720 know, like looking at the code, finding bugs, making it better, 372 00:20:32,720 --> 00:20:35,840 what are some of the other benefits that you can share for 373 00:20:35,840 --> 00:20:37,960 doing code review? Yeah. 374 00:20:37,960 --> 00:20:41,880 So you've been finding bugs. That's a highly debated thing 375 00:20:41,880 --> 00:20:44,800 that people say happens. I talk about that in the book. 376 00:20:44,800 --> 00:20:47,000 There are a couple studies that do show, like there's a 377 00:20:47,000 --> 00:20:50,800 Microsoft study that actually says it doesn't find bugs, but 378 00:20:51,000 --> 00:20:53,560 depending on how your team uses it, sometimes it does. 379 00:20:53,840 --> 00:20:57,600 We'll leave that up to debate. One of the really good things 380 00:20:57,600 --> 00:21:02,920 that Code Review does is it leaves artifacts for your team. 381 00:21:02,920 --> 00:21:07,600 So it leaves a historical record of what changes in your code, 382 00:21:07,840 --> 00:21:11,600 and it also increases knowledge transfer among the team. 383 00:21:11,920 --> 00:21:16,200 I think to me as I'm continuing to write this book, those are 384 00:21:16,200 --> 00:21:20,760 the really the key things of why code Review really will stay. 385 00:21:21,080 --> 00:21:23,200 We might talk about this later, but people are talking about, 386 00:21:23,200 --> 00:21:26,320 oh, AI can just do the code review for us and we don't have 387 00:21:26,320 --> 00:21:28,640 to deal with it anymore. That's not going to happen. 388 00:21:29,120 --> 00:21:32,400 But knowledge transfer is a really big one. 389 00:21:32,840 --> 00:21:37,400 You know, as you code review and more eyes look at the code 390 00:21:37,400 --> 00:21:40,600 changes that are being presented, that's one of the 391 00:21:40,600 --> 00:21:44,120 greatest ways to share what is happening on the code base. 392 00:21:44,520 --> 00:21:49,160 And again, if the code changes are prepared properly, as in you 393 00:21:49,160 --> 00:21:54,000 give the context as to why the code is changing, you tie it to 394 00:21:54,000 --> 00:21:57,520 the tickets that may have started the you know the reason 395 00:21:57,520 --> 00:22:00,200 for why you're making the change in the 1st place, or you know 396 00:22:00,200 --> 00:22:04,160 you tie it to the initial bug report that ties to the fix 397 00:22:04,160 --> 00:22:07,160 you're about to change. It just paints a better picture 398 00:22:07,160 --> 00:22:11,080 for the rest of your team as to why you're making this change. 399 00:22:11,360 --> 00:22:15,560 And then the DIF itself, the code, That's the how, right? 400 00:22:15,560 --> 00:22:17,600 How are you doing it? You're looking at the code, 401 00:22:17,600 --> 00:22:21,640 you're seeing what you are implementing, the code review, 402 00:22:21,640 --> 00:22:24,000 the PR, the pull request or the change request. 403 00:22:24,240 --> 00:22:28,200 That's the why. And so that is very important to 404 00:22:28,200 --> 00:22:32,480 spread across the team. And as you go and evolve your 405 00:22:32,480 --> 00:22:35,240 code base, it's very important for the team to all be on the 406 00:22:35,240 --> 00:22:37,080 same page. Otherwise you get these 407 00:22:37,080 --> 00:22:40,360 scenarios where it's a single person or it's like a senior 408 00:22:40,360 --> 00:22:43,920 senior developer and they're the only one that knows anything 409 00:22:43,920 --> 00:22:45,800 about everything about the code base. 410 00:22:45,800 --> 00:22:49,120 So then you get this problem where they actually become the 411 00:22:49,160 --> 00:22:52,920 only person who reviews every single pull request, and then 412 00:22:52,920 --> 00:22:55,560 you get into the unmanageable case where it's like, oh, it's a 413 00:22:55,560 --> 00:22:57,760 bottleneck. I'm the only one who can look at 414 00:22:57,760 --> 00:22:58,920 these. Well, why are they the only one 415 00:22:58,920 --> 00:23:00,440 that can look at this? I'm the only one that knows 416 00:23:00,440 --> 00:23:01,960 about this code. Why are you the only one that 417 00:23:01,960 --> 00:23:04,440 knows about this code? Well, because I'm the only one 418 00:23:04,440 --> 00:23:07,840 that's familiar. So that in and of itself is a 419 00:23:07,840 --> 00:23:12,160 great reason to do code reviews and to do it with as many people 420 00:23:12,160 --> 00:23:14,720 on the team as possible to learn about it. 421 00:23:15,200 --> 00:23:19,280 The other side of that other really good goal of code reviews 422 00:23:19,280 --> 00:23:22,720 is historical record, specifically traceability. 423 00:23:22,960 --> 00:23:29,200 So when you have these official, formalized, documented reasons 424 00:23:29,200 --> 00:23:32,800 for why the code has changed, it makes it much easier for the 425 00:23:32,800 --> 00:23:38,280 team to see what has happened and if something goes wrong, if 426 00:23:38,280 --> 00:23:41,360 you do it correctly. Or I wouldn't say if you take 427 00:23:41,360 --> 00:23:45,600 the effort and time to, you know, have really good commit 428 00:23:45,600 --> 00:23:50,160 messages to link work tickets to the pull requests, to make pull 429 00:23:50,160 --> 00:23:55,400 requests and changes atomic and small, and to do them in a way 430 00:23:55,400 --> 00:23:59,200 that if something does go wrong, you can easily revert back and 431 00:23:59,200 --> 00:24:01,040 say, oh, I know when that happened. 432 00:24:01,040 --> 00:24:04,400 It specifically happened October 31st on this specific commit. 433 00:24:04,600 --> 00:24:07,440 That's the goal, right? That's the ideal state that 434 00:24:07,440 --> 00:24:10,960 developers would love to have. But that's possible with Code 435 00:24:10,960 --> 00:24:14,680 Review because you get to see what has happened, why it has 436 00:24:14,680 --> 00:24:17,400 changed and you are where you are today. 437 00:24:17,720 --> 00:24:20,760 And then if you use, you know there are things like I just 438 00:24:20,760 --> 00:24:24,400 watched a talk at a conference I spoke at where they use get 439 00:24:24,400 --> 00:24:28,160 release manager and they don't use pull requests, but they do 440 00:24:28,160 --> 00:24:32,640 use issues as kind of their central item that kicks off 441 00:24:32,640 --> 00:24:34,920 everything. And because they have good 442 00:24:34,920 --> 00:24:39,600 commit messages, because they use proper linking with issues 443 00:24:39,600 --> 00:24:42,640 and with pull requests and labels and things like that, 444 00:24:42,760 --> 00:24:45,960 they are able to automatically generate a change set. 445 00:24:46,200 --> 00:24:49,680 And then also if there was an issue that was fixed by a 446 00:24:49,680 --> 00:24:53,800 specific pull request, they automatically are able to send a 447 00:24:53,800 --> 00:24:56,680 message through a web hook that says hey, we just released 448 00:24:56,680 --> 00:24:59,040 something. This has been fixed in release, 449 00:24:59,040 --> 00:25:01,240 you know, 2.3 in this particular thing. 450 00:25:01,520 --> 00:25:07,280 So a lot of that accountability and traceability you can get if 451 00:25:07,280 --> 00:25:11,400 you do these processes, and I think those are really, really 452 00:25:11,400 --> 00:25:13,880 good things to have. That's what makes software 453 00:25:13,880 --> 00:25:16,920 development much more professional and much more fun, 454 00:25:16,920 --> 00:25:19,920 I think if you have it all laid out in a nice way like that. 455 00:25:21,000 --> 00:25:23,840 It seems like a really high bar and really interesting to learn 456 00:25:24,040 --> 00:25:27,240 as a case study, right? So I think things like spreading 457 00:25:27,240 --> 00:25:30,440 the knowledge, we kind of like think it can, it will happen 458 00:25:30,440 --> 00:25:32,640 right in the code review, but not all the time I believe. 459 00:25:32,680 --> 00:25:35,880 Because like what you said, right, LGTM is most common, 460 00:25:36,080 --> 00:25:38,960 common probably when you're looking at the PR, whether it 461 00:25:38,960 --> 00:25:41,600 happens constructively or not, we don't know. 462 00:25:41,880 --> 00:25:44,800 And many other things like for example making sure the code 463 00:25:44,800 --> 00:25:47,120 base follows the same standard in the team, right? 464 00:25:47,120 --> 00:25:50,360 I think that's also one of the other benefits if done properly. 465 00:25:51,080 --> 00:25:54,320 So I want to tackle the other topic that you cover really, 466 00:25:54,320 --> 00:25:56,560 really well in the book, which is about the roles and 467 00:25:56,560 --> 00:26:00,640 responsibilities of the people or entities involved in the code 468 00:26:00,640 --> 00:26:02,920 review, right? So you mentioned like reviewer, 469 00:26:02,920 --> 00:26:05,720 author and things like that. There are actually more roles in 470 00:26:05,720 --> 00:26:08,600 the code review process. Maybe if you can go highlight 471 00:26:08,600 --> 00:26:11,880 some of the roles and responsibilities, especially the 472 00:26:11,880 --> 00:26:12,560 author. Maybe. 473 00:26:12,560 --> 00:26:13,760 Let's start with the author first. 474 00:26:14,840 --> 00:26:17,800 Sure. So the author, like I mentioned, 475 00:26:17,800 --> 00:26:20,320 this is the one that puts up the code changes. 476 00:26:20,320 --> 00:26:23,560 They are the ones that write the code and they're asking someone 477 00:26:23,560 --> 00:26:27,680 to review it. And for the author, I put a lot 478 00:26:27,680 --> 00:26:30,440 of responsibilities. I kind of call it a contract for 479 00:26:30,440 --> 00:26:33,200 them to adhere to in the code review. 480 00:26:33,520 --> 00:26:37,840 And one of the first things is to kind of forget your ego. 481 00:26:37,840 --> 00:26:41,760 So we touched on this earlier. Like I said, sometimes we feel 482 00:26:41,760 --> 00:26:44,560 like our code is an extension of ourselves. 483 00:26:44,840 --> 00:26:48,320 We've been working on this really nice change, or we've 484 00:26:48,520 --> 00:26:52,440 fixed this bug and we've prepared this really nice code 485 00:26:52,440 --> 00:26:56,080 and it's our baby, right? And so when we put it up for 486 00:26:56,080 --> 00:27:01,200 review, we become vulnerable. Now we don't want to hear 487 00:27:01,200 --> 00:27:04,440 sometimes feedback on it because what do you mean it's perfect 488 00:27:04,440 --> 00:27:07,000 the way that it is. Our baby is perfect the way that 489 00:27:07,000 --> 00:27:10,560 it is. But sometimes not being open to 490 00:27:10,560 --> 00:27:15,360 that feedback means we don't get the best code possible. 491 00:27:15,360 --> 00:27:19,800 Or when we're so in the weeds, working on our particular 492 00:27:19,800 --> 00:27:24,000 feature, just being down at that level, focusing on the code. 493 00:27:24,240 --> 00:27:27,360 Sometimes we don't see particular edge cases, or 494 00:27:27,360 --> 00:27:30,960 sometimes different skill sets and different perspectives. 495 00:27:31,040 --> 00:27:34,960 Take a look at our code and can actually offer some good 496 00:27:34,960 --> 00:27:38,400 feedback. So being open to that means we 497 00:27:38,400 --> 00:27:42,520 have to let go of the ego of, you know, our code is perfect 498 00:27:42,520 --> 00:27:44,760 and the way that I've submitted it is perfect. 499 00:27:44,760 --> 00:27:47,880 Alongside of that you might be open, you know, you're like, no, 500 00:27:47,880 --> 00:27:49,920 I know that I have room for improvement. 501 00:27:50,240 --> 00:27:53,480 Sometimes people will get feedback but then they'll say, 502 00:27:53,480 --> 00:27:56,320 Oh well, actually, no, I don't think that is correct. 503 00:27:56,680 --> 00:28:00,240 So that's the other side is, you know, you say you're open to 504 00:28:00,240 --> 00:28:02,960 feedback, but then you actually do get feedback and then you're 505 00:28:02,960 --> 00:28:05,720 like, no, you know, you disagree and you try to fight it off. 506 00:28:05,720 --> 00:28:10,760 So being able to let go of your ego and understanding that if 507 00:28:10,760 --> 00:28:15,000 we're asking the reviewers to objectively review our code and 508 00:28:15,000 --> 00:28:18,520 to only look at the code, not consider us, then we need to do 509 00:28:18,520 --> 00:28:21,240 the same thing. We need to objectively say this 510 00:28:21,320 --> 00:28:24,800 feedback is based on the code as I have presented. 511 00:28:24,800 --> 00:28:28,400 It has nothing to do with me, has nothing to do with anything 512 00:28:28,400 --> 00:28:29,720 about me. It's just the code. 513 00:28:29,720 --> 00:28:33,160 Focus on the code. The next thing that authors are 514 00:28:33,160 --> 00:28:38,080 responsible for is we need to have due diligence when we 515 00:28:38,080 --> 00:28:40,280 actually create these pull requests. 516 00:28:40,280 --> 00:28:43,880 So I don't know about you, Henry, but have you ever 517 00:28:43,880 --> 00:28:47,360 received a pull request where it's like the title is like bug 518 00:28:47,360 --> 00:28:52,800 fix and then everything else is empty and you have no idea what 519 00:28:52,800 --> 00:28:55,480 is going on? Have you ever had like, a very 520 00:28:55,800 --> 00:28:58,680 nondescript review that you've had to do? 521 00:28:59,720 --> 00:29:03,600 Yes, not only PR, sometimes the code commit command as well. 522 00:29:03,680 --> 00:29:04,040 Right. Yep. 523 00:29:05,520 --> 00:29:10,240 So that is another big thing that I see authors don't do is 524 00:29:10,240 --> 00:29:12,880 we don't set up the reviewer for success. 525 00:29:12,880 --> 00:29:17,280 We don't give them the context that they need to properly 526 00:29:17,280 --> 00:29:21,360 review our code. So that starts with a very good 527 00:29:21,360 --> 00:29:24,560 descriptive title. Like I need to know what I'm 528 00:29:24,560 --> 00:29:28,760 about to review in the 10 seconds that I read the title 529 00:29:29,120 --> 00:29:32,480 and then when I go into the description, the description is 530 00:29:32,480 --> 00:29:36,120 where you tell me the why. Why am I making this change? 531 00:29:36,120 --> 00:29:39,080 Is it because a ticket? It was the catalyst for this? 532 00:29:39,080 --> 00:29:43,160 Is it because we had a planned? You know, in our retrospective 533 00:29:43,160 --> 00:29:46,640 we said this work was going to be done and this is now the work 534 00:29:46,640 --> 00:29:49,600 that is happening. Is it because it was someone off 535 00:29:49,600 --> 00:29:52,200 conversation with our manager that said you need to do this 536 00:29:52,200 --> 00:29:56,640 right now, like why is it here? And then adding that context 537 00:29:56,640 --> 00:30:00,120 there, it tremendously helps put the reviewer in the correct 538 00:30:00,120 --> 00:30:05,240 mindset to review it. Not only that, but making our 539 00:30:05,240 --> 00:30:09,120 PRS manageable. So I've been guilty of this 540 00:30:09,120 --> 00:30:11,840 before, especially when I had, you know, I was just learning 541 00:30:11,840 --> 00:30:14,800 Git. But you know, there are PRS out 542 00:30:14,800 --> 00:30:22,360 there with like 5000 files changed or 8372 lines changed 543 00:30:22,360 --> 00:30:26,640 and you're like really, you really expect me to properly 544 00:30:26,640 --> 00:30:32,760 review this a single person. So not making the PR manageable 545 00:30:32,760 --> 00:30:36,600 is something I think is a very big responsibility that we have 546 00:30:36,600 --> 00:30:39,320 as an author. So that's why in my book I do 547 00:30:39,600 --> 00:30:43,600 have, you know, rough guidelines from several research papers and 548 00:30:43,600 --> 00:30:46,760 studies that have been done. Basically, the smaller that you 549 00:30:46,760 --> 00:30:50,760 can make your PR, the better and it makes sense for everybody 550 00:30:50,760 --> 00:30:53,200 involved. The more atomic the change, the 551 00:30:53,200 --> 00:30:56,360 less files you have in your PR, the less lines of code that 552 00:30:56,360 --> 00:30:59,640 someone has to look at. The more likely that the person, 553 00:30:59,640 --> 00:31:02,920 the reviewer, is going to be able to properly review that, 554 00:31:03,200 --> 00:31:06,600 the less likely someone can have an excuse to say, oh, I don't 555 00:31:06,600 --> 00:31:09,480 have time to review because it's such a small PR. 556 00:31:09,640 --> 00:31:12,160 If you could do it in 10 minutes, it's going to be 557 00:31:12,160 --> 00:31:14,560 reviewed. There's no excuse to not review 558 00:31:14,560 --> 00:31:17,040 it. And then again, it's the 559 00:31:17,080 --> 00:31:21,600 traceability aspect of if something goes wrong, it's much 560 00:31:21,600 --> 00:31:25,320 easier to pinpoint. This particular pull request was 561 00:31:25,320 --> 00:31:29,280 the source or the route, right, versus having like a gigantic 562 00:31:29,280 --> 00:31:32,960 pull request with several changes muddled in there along 563 00:31:32,960 --> 00:31:37,520 with configuration changes and some package, Jason changes. 564 00:31:37,520 --> 00:31:41,920 Like you put that cognitive load on the reviewer and on yourself 565 00:31:41,920 --> 00:31:45,200 later on if you have to ever have to untangle something from 566 00:31:45,200 --> 00:31:49,200 that particular pull request. So that's another responsibility 567 00:31:49,200 --> 00:31:53,120 that I say is very important for authors is to make sure that the 568 00:31:53,120 --> 00:31:56,160 pull request that they're preparing is primed, is what I 569 00:31:56,160 --> 00:31:58,960 say it's ready for the reviewer to look at. 570 00:31:59,160 --> 00:32:00,960 And I think those are the big ones. 571 00:32:00,960 --> 00:32:03,760 I'm sure there's a couple more that I'm forgetting, but those 572 00:32:03,760 --> 00:32:07,080 are the ones off the top of my head are very, very important 573 00:32:07,080 --> 00:32:09,760 that we tend to forget. Yeah, you mentioned about 574 00:32:09,760 --> 00:32:11,600 cognitive load. I think sometimes we don't 575 00:32:11,600 --> 00:32:14,760 empathize the reviewer, right. So we are in the mood to finish 576 00:32:14,920 --> 00:32:17,200 whatever the things that we are working on, right. 577 00:32:17,200 --> 00:32:20,120 And we didn't realize that the number of files, number of lines 578 00:32:20,120 --> 00:32:23,400 of code are too big and we just submit the Pi in one go, right? 579 00:32:23,640 --> 00:32:26,680 So I think you mentioned a very, very important responsibility as 580 00:32:26,680 --> 00:32:29,560 the author as well is be your first reviewer, right. 581 00:32:29,560 --> 00:32:32,760 So you look at the PR, look at the number of changes and make 582 00:32:32,760 --> 00:32:35,240 it more manageable, right. So I think making it more 583 00:32:35,240 --> 00:32:39,320 manageable to me is maybe one of the biggest important thing that 584 00:32:39,320 --> 00:32:41,760 we can do in order to make it less dreadful. 585 00:32:42,280 --> 00:32:45,680 So now the author has created the PR, so there's a reviewer. 586 00:32:45,760 --> 00:32:47,760 I think this is also another important role. 587 00:32:47,760 --> 00:32:50,040 So tell us the responsibility for the reviewer. 588 00:32:50,720 --> 00:32:54,280 Yes, so for the reviewer, I also say the same thing, right? 589 00:32:54,280 --> 00:32:57,320 Forget your ego. Focus on the code, not the 590 00:32:57,320 --> 00:33:00,720 developer. So as reviewers we need to be 591 00:33:00,720 --> 00:33:03,280 objective. We can't bring in. 592 00:33:03,280 --> 00:33:06,320 You know, if you're having like an argument with your Co worker, 593 00:33:06,320 --> 00:33:09,200 but they ask you to review your code, that's like another issue. 594 00:33:09,200 --> 00:33:11,840 You should sort that out. But if you are reviewing 595 00:33:11,840 --> 00:33:14,120 someone's code, that shouldn't come into play. 596 00:33:14,120 --> 00:33:15,480 You should just look at the code. 597 00:33:15,480 --> 00:33:21,080 This is also not the time to be giving a better implementation 598 00:33:21,080 --> 00:33:25,400 for the sake of giving a better implementation without reason, 599 00:33:25,480 --> 00:33:27,960 right? So a lot of what I hear when I 600 00:33:27,960 --> 00:33:31,680 talk about these reviewer responsibilities is that this 601 00:33:31,680 --> 00:33:35,160 means that I can't just give feedback anymore at all to the 602 00:33:35,160 --> 00:33:37,520 author. Like it has to be walking on egg 603 00:33:37,520 --> 00:33:38,840 shells. Everything is happy. 604 00:33:38,840 --> 00:33:40,720 I'm like, no, that's not what I'm trying to say. 605 00:33:40,720 --> 00:33:43,760 What I'm trying to say is, if you have feedback to give, you 606 00:33:43,760 --> 00:33:46,320 need to do it effectively. So that's another thing that I 607 00:33:46,320 --> 00:33:51,000 say is a responsibility of the reviewer is to actually learn 608 00:33:51,000 --> 00:33:54,960 how to write effective comments. This is something I think a lot 609 00:33:54,960 --> 00:33:58,240 of reviewers have trouble with, and rightfully so. 610 00:33:58,240 --> 00:34:00,480 Some are actually empathetic, right? 611 00:34:00,480 --> 00:34:03,400 They're like, I don't want to offend the developer. 612 00:34:03,480 --> 00:34:05,520 I don't want to hurt their feelings. 613 00:34:05,640 --> 00:34:08,880 I want to bring up something that is important, but I don't 614 00:34:08,880 --> 00:34:12,280 want to do it in a way that will scare them off or make them 615 00:34:12,280 --> 00:34:15,080 upset. And so something that I've 616 00:34:15,239 --> 00:34:17,960 developed in the book, I call it the Triple R pattern. 617 00:34:18,320 --> 00:34:21,560 If you are going to ask someone to make a change or you have a 618 00:34:21,560 --> 00:34:24,199 request or something, I say the three Rs. 619 00:34:24,480 --> 00:34:28,080 Name the request, give your rationale for it, and then give 620 00:34:28,080 --> 00:34:31,400 a result. And that will almost always be 621 00:34:31,400 --> 00:34:35,400 an objective way to write a comment that gives feedback to 622 00:34:35,400 --> 00:34:38,360 the author, because you tell them what you want, which is the 623 00:34:38,360 --> 00:34:41,560 request you give, the reasoning behind it, which is the 624 00:34:41,560 --> 00:34:45,480 rationale, and then the result that you are sharing with them 625 00:34:45,639 --> 00:34:49,159 is a way for them to actually compare and say, am I on the 626 00:34:49,159 --> 00:34:51,440 right track or did I do it correctly? 627 00:34:51,719 --> 00:34:54,760 And that could be, you know, If you need something to change 628 00:34:54,800 --> 00:34:59,000 visually, have a screenshot exactly of what you are 629 00:34:59,000 --> 00:35:01,920 expecting, because that will almost always be helpful than 630 00:35:01,920 --> 00:35:04,640 trying to describe it. If it's like a performance 631 00:35:04,640 --> 00:35:07,280 optimization, give them a benchmark to hit. 632 00:35:07,520 --> 00:35:11,400 The more specific that you can be, the less likely the author 633 00:35:11,400 --> 00:35:13,880 is going to take this as feedback that says, oh, this 634 00:35:13,880 --> 00:35:17,120 person just wants me to do more work or you know, they're trying 635 00:35:17,120 --> 00:35:20,160 to make me achieve a standard that's unnecessary. 636 00:35:20,160 --> 00:35:22,000 Right now I just need to get the work done. 637 00:35:22,200 --> 00:35:26,240 The less you are vague about it, the more outcome focused you 638 00:35:26,240 --> 00:35:29,200 are, the more specific you are and the more objective you are 639 00:35:29,200 --> 00:35:34,040 about it, the better your comments will be received by the 640 00:35:34,040 --> 00:35:37,640 author with that. Another thing that I say is a 641 00:35:37,640 --> 00:35:42,920 responsibility of the reviewer is we should also be thorough in 642 00:35:42,920 --> 00:35:46,160 our reviews. So there's been a notion to kind 643 00:35:46,160 --> 00:35:49,040 of skim reviews. This is why we get the looks 644 00:35:49,040 --> 00:35:52,400 good to me, right? And as an author, this time, 645 00:35:52,400 --> 00:35:56,480 when you know you've prepared a nice review, maybe you have a 646 00:35:56,480 --> 00:35:59,960 nice set of changes, it's actually manageable and you put 647 00:35:59,960 --> 00:36:02,920 it up for requests, you're like oh, can someone review this? 648 00:36:02,920 --> 00:36:06,520 And then within like 2 minutes you get a thumbs up and you're 649 00:36:06,520 --> 00:36:09,520 like there's no way somebody looked at this properly. 650 00:36:09,680 --> 00:36:12,920 There's no way they actually gave this the due diligence that 651 00:36:12,920 --> 00:36:16,280 it needed. So as a reviewer you know it can 652 00:36:16,280 --> 00:36:21,440 be tempting, but This is why it's a two way effort of when 653 00:36:21,440 --> 00:36:24,280 the PRS are manageable and they're small enough, then 654 00:36:24,280 --> 00:36:27,240 there's really no excuse for you as a reviewer to skim them. 655 00:36:27,560 --> 00:36:30,960 So if they are small enough and they are manageable, being able 656 00:36:30,960 --> 00:36:34,640 to review in 1020 minutes at most, give them the proper 657 00:36:34,640 --> 00:36:37,520 review that it needs. Go through them, go through the 658 00:36:37,520 --> 00:36:41,680 code and really give it your thorough eye as if you would 659 00:36:41,680 --> 00:36:46,640 want to have your own code reviewed and then also giving 660 00:36:46,680 --> 00:36:51,000 that time to yourself. Sometimes you need to talk to 661 00:36:51,000 --> 00:36:54,560 yourself and say you know before you asks the author to make a 662 00:36:54,560 --> 00:36:57,160 change. For example, first ask yourself, 663 00:36:57,360 --> 00:36:59,840 why am I asking the person to make this change? 664 00:36:59,840 --> 00:37:04,200 Because if you yourself can articulate why you are going to 665 00:37:04,200 --> 00:37:06,960 make that request, then you're going to be able to better 666 00:37:06,960 --> 00:37:10,120 articulate it to the author. That's how you get rid of those 667 00:37:10,120 --> 00:37:12,920 comments of like, this could be better and then it's done. 668 00:37:12,920 --> 00:37:16,160 Or, you know, a comment that's like, I think there's something 669 00:37:16,160 --> 00:37:18,920 wrong here, but then that's it and you leave it at that. 670 00:37:19,360 --> 00:37:22,920 That's where the discussions go back and forth because you 671 00:37:22,920 --> 00:37:25,760 didn't take the little extra time to articulate it a little 672 00:37:25,760 --> 00:37:28,640 bit better, to understand yourself why you're making that 673 00:37:28,640 --> 00:37:31,120 comment. And so you know, doing that due 674 00:37:31,120 --> 00:37:34,240 diligence, not skimming in your review, giving it a proper 675 00:37:34,240 --> 00:37:38,120 review, and, you know, forgetting your ego and making 676 00:37:38,120 --> 00:37:42,160 sure that we write effective comments right now are on the 677 00:37:42,160 --> 00:37:45,280 top of my head the most important responsibilities for 678 00:37:45,280 --> 00:37:47,800 the reviewer in a code review. Yep. 679 00:37:48,000 --> 00:37:50,120 Also, you mentioned something really, really important that I 680 00:37:50,120 --> 00:37:51,680 want to highlight here as well, right? 681 00:37:51,680 --> 00:37:55,200 Eventually, the reviewer also responsible for the code that 682 00:37:55,200 --> 00:37:57,240 gets through right to the production. 683 00:37:57,600 --> 00:38:00,320 So I think if we just skim it and we think that it's the 684 00:38:00,320 --> 00:38:03,080 author's problem, I think that's also not the right way, right? 685 00:38:03,080 --> 00:38:05,760 So I think the reviewer should also own the responsibility to 686 00:38:05,760 --> 00:38:07,640 make sure it works really, really well. 687 00:38:08,240 --> 00:38:09,960 Yes, I'm glad you brought that up. 688 00:38:10,120 --> 00:38:11,320 I'm really glad you brought that up. 689 00:38:11,320 --> 00:38:13,880 The last thing I'll say about that is there's this notion with 690 00:38:13,880 --> 00:38:17,160 reviewers to be like, oh, it's the developer's fault, right? 691 00:38:17,160 --> 00:38:21,000 If something does go wrong and it goes through their review, we 692 00:38:21,000 --> 00:38:23,120 are quick to blame and say, oh, it's not. 693 00:38:23,160 --> 00:38:26,680 And really what we should be thinking is, oh, how did that 694 00:38:26,680 --> 00:38:29,880 actually pass through my review? So that's like a really big 695 00:38:30,160 --> 00:38:34,520 mindset shift that I'm hoping to convey in the book, and I hope 696 00:38:34,520 --> 00:38:37,200 more people will start to think about when it comes to code 697 00:38:37,200 --> 00:38:39,080 reviews specifically as a reviewer. 698 00:38:39,520 --> 00:38:41,240 But thank you for reminding me of that part. 699 00:38:41,560 --> 00:38:44,600 Yep, the other aspect about reviewer is like how long the 700 00:38:44,840 --> 00:38:47,840 code review lasts, right? So I think there's sometimes a 701 00:38:47,840 --> 00:38:50,760 power dynamics, especially if you ask senior to review your 702 00:38:50,760 --> 00:38:52,800 code, right? You kind of like just wait and 703 00:38:52,800 --> 00:38:54,960 kind of like back them please review my code. 704 00:38:55,200 --> 00:38:57,880 So I think as a reviewer you also have the responsibility to 705 00:38:57,880 --> 00:39:00,800 make sure it doesn't drag right. It doesn't delay for too long, 706 00:39:01,320 --> 00:39:04,800 which brings us to the aspect of team right, A-Team, maybe 707 00:39:04,800 --> 00:39:08,160 standard or convention right. So the team here also has a role 708 00:39:08,160 --> 00:39:10,360 to play. So tell us, what is the 709 00:39:10,360 --> 00:39:16,200 responsibility for the team? So the team is basically the one 710 00:39:16,200 --> 00:39:19,840 that sets the tempo for the rest of you know, for all the 711 00:39:19,840 --> 00:39:22,640 individuals. Sometimes there are some 712 00:39:22,640 --> 00:39:26,760 suggestions in my book where I say have extra steps, you know, 713 00:39:26,760 --> 00:39:31,600 oh, use a lot more caution and be very intentional about what 714 00:39:31,600 --> 00:39:35,320 you do. But sometimes you have a team 715 00:39:35,320 --> 00:39:39,640 who might be like a team of five senior developers. 716 00:39:39,640 --> 00:39:42,400 They've all been working very well with each other. 717 00:39:42,400 --> 00:39:45,960 They know each other. Maybe they have the luxury of 718 00:39:45,960 --> 00:39:50,600 actually being in an office on site together, and so if that's 719 00:39:50,600 --> 00:39:55,040 the case, maybe you may not need such a formalized process. 720 00:39:55,040 --> 00:40:00,000 Maybe you trust each other and you have less of a checklist 721 00:40:00,000 --> 00:40:01,800 when it comes to your code review. 722 00:40:02,120 --> 00:40:06,320 But then if you have maybe, say, a more mixed team, you have a 723 00:40:06,320 --> 00:40:10,120 couple senior developers, you have a more intermediate and 724 00:40:10,120 --> 00:40:13,680 junior developers, or you have an intern or someone that's 725 00:40:13,680 --> 00:40:16,760 maybe new to software development, then it's the 726 00:40:16,760 --> 00:40:20,240 team's job to help. Integrate the rest of the 727 00:40:20,240 --> 00:40:24,920 members so that number one, they know what the process is and #2 728 00:40:24,920 --> 00:40:27,760 to keep the process enforced for everyone. 729 00:40:28,120 --> 00:40:31,720 Doing that becomes a lot easier. There's a whole chapter I 730 00:40:31,720 --> 00:40:36,000 dedicate to this concept, which is what I call the team working 731 00:40:36,000 --> 00:40:40,320 agreement, and this is a document that's Co produced by 732 00:40:40,320 --> 00:40:42,560 the team. It's basically all of the 733 00:40:42,560 --> 00:40:46,040 implicit expectations that you have as a team. 734 00:40:46,400 --> 00:40:48,600 You make them explicit, you write them out. 735 00:40:48,600 --> 00:40:51,360 So what are the steps of your code review? 736 00:40:51,360 --> 00:40:54,320 What are the expectations around the code review? 737 00:40:54,640 --> 00:40:59,400 Things like if somebody leaves feedback on my code review, do I 738 00:40:59,400 --> 00:41:02,960 have to answer every comment? Do I have to mark all of it as 739 00:41:02,960 --> 00:41:05,600 resolved, or is there some wiggle room there? 740 00:41:05,720 --> 00:41:08,840 What counts as done? What counts as an approval? 741 00:41:09,200 --> 00:41:12,000 How long is too long on a code review? 742 00:41:12,000 --> 00:41:15,440 If someone hasn't looked at my code, when is it appropriate to 743 00:41:15,440 --> 00:41:17,960 follow up? These are all the sources of 744 00:41:17,960 --> 00:41:22,320 frustration, of tension among the team, because it may be 745 00:41:22,320 --> 00:41:25,680 different for everybody in everyone's head and specific 746 00:41:25,680 --> 00:41:27,760 preferences. You know, someone might want it 747 00:41:27,760 --> 00:41:31,520 done the next day, 24 hours, someone might be more LAX and 748 00:41:31,520 --> 00:41:33,400 like, oh, I'll get to it by the end of the week. 749 00:41:33,640 --> 00:41:38,080 So having all of that figured out on your team not only makes 750 00:41:38,080 --> 00:41:42,240 it easier for everyone to be on the same page, but if someone is 751 00:41:42,240 --> 00:41:46,040 going off base and is not following the process, then you 752 00:41:46,040 --> 00:41:50,760 can use that document and use it to enforce on the team to say, 753 00:41:50,760 --> 00:41:54,440 hey, look, we all agreed this is what we said for this particular 754 00:41:54,440 --> 00:41:58,240 situation and this is what we should be following. 755 00:41:58,240 --> 00:42:02,240 And so it's much easier to bring that up versus just saying it's 756 00:42:02,240 --> 00:42:06,320 a he said, she said Or you know, it's one preference versus 757 00:42:06,320 --> 00:42:09,960 somebody else's preference, it's the team's preference and this 758 00:42:09,960 --> 00:42:13,720 is the team's standards and this is what the team's way of going 759 00:42:13,720 --> 00:42:16,560 about things. And the other thing about this 760 00:42:16,560 --> 00:42:20,080 document is that, you know, a lot of people say, well, you 761 00:42:20,080 --> 00:42:23,000 know, once these are written like what if things change? 762 00:42:23,000 --> 00:42:27,240 Well, the document changes with the team, so it's not this set 763 00:42:27,240 --> 00:42:30,040 in stone thing where it's like, these are the rules forever. 764 00:42:30,040 --> 00:42:34,200 No, your team's going to change, your code base is going to 765 00:42:34,200 --> 00:42:36,120 change. Your project's expectations are 766 00:42:36,120 --> 00:42:38,320 going to change. There are a lot of things that 767 00:42:38,320 --> 00:42:40,600 are going to change. And so if you find that there 768 00:42:40,600 --> 00:42:44,360 are parts of the process that don't work for your team, change 769 00:42:44,360 --> 00:42:47,760 it and then also change that team working agreement. 770 00:42:47,760 --> 00:42:50,800 That's the great thing about this is that as long as you have 771 00:42:50,800 --> 00:42:55,160 an open culture on your team, a culture of feedback where people 772 00:42:55,160 --> 00:42:58,720 are open to talking to each other and open to discussing 773 00:42:58,720 --> 00:43:03,360 things like this, then a change can be made the next day. 774 00:43:03,360 --> 00:43:05,800 And then, you know, it might take a while for people to get 775 00:43:05,800 --> 00:43:08,240 used to it, but the change is implemented and then you 776 00:43:08,240 --> 00:43:12,600 continue to enforce it that way. So that is one really good way 777 00:43:12,600 --> 00:43:17,440 to keep the team accountable. And then the last part is not 778 00:43:17,440 --> 00:43:20,000 everything is going to go the happy path, right. 779 00:43:20,000 --> 00:43:22,240 It's not just we're going to change some code, we're going to 780 00:43:22,240 --> 00:43:25,480 review it, Some people look at it and then it's goes into the 781 00:43:25,480 --> 00:43:28,240 rest of the code base. There's going to be times where 782 00:43:28,240 --> 00:43:32,360 there are emergencies, so production goes down. 783 00:43:32,360 --> 00:43:36,360 There's some really big thing that happens that requires the 784 00:43:36,360 --> 00:43:38,880 attention of the team. What do you do in that 785 00:43:38,880 --> 00:43:42,480 situation? This is where a lot of other 786 00:43:42,480 --> 00:43:46,680 problems become larger, when you don't have a process in place. 787 00:43:46,680 --> 00:43:51,280 So sometimes emergencies are used to circumvent the code 788 00:43:51,280 --> 00:43:55,880 review process, and that's not a good use case for it. 789 00:43:56,000 --> 00:44:01,160 What would be better is to anticipate emergency scenarios 790 00:44:01,360 --> 00:44:03,480 and then have that written on your team. 791 00:44:03,480 --> 00:44:06,320 Say what do we do in an emergency scenario? 792 00:44:06,320 --> 00:44:09,480 There's things called an emergency playbook where you 793 00:44:09,520 --> 00:44:13,680 write down, OK, if it's this kind of emergency, what are we 794 00:44:13,680 --> 00:44:18,960 allowing in our process? Are we saying we can allow one 795 00:44:18,960 --> 00:44:23,000 approval instead of two? Does a manager approving you 796 00:44:23,000 --> 00:44:26,440 know, does that count in an emergency situation versus our 797 00:44:26,440 --> 00:44:31,200 normal two people or more? Could we open a pull request 798 00:44:31,200 --> 00:44:35,320 straight to production? What are those processes and 799 00:44:35,320 --> 00:44:38,280 what's happening in them? And then not only that, if you 800 00:44:38,280 --> 00:44:43,000 are gonna go around the process, your normal code review process, 801 00:44:43,280 --> 00:44:47,960 make sure that in your emergency processes you have a very 802 00:44:47,960 --> 00:44:52,040 outlined detailed communication plan of this is happening and 803 00:44:52,040 --> 00:44:55,920 This is why we're doing it. So it's one thing to circumvent 804 00:44:55,920 --> 00:44:58,120 the normal process. It's another to make sure 805 00:44:58,120 --> 00:45:02,080 everyone knows that that is happening, knows that's OK 806 00:45:02,080 --> 00:45:05,120 because you've anticipated it, and also knows that you're 807 00:45:05,120 --> 00:45:07,640 circumventing it because it's an emergency. 808 00:45:07,640 --> 00:45:10,720 So that's very important, to anticipate it and then to 809 00:45:10,720 --> 00:45:14,480 communicate to as many people as possible that whatever you're 810 00:45:14,480 --> 00:45:17,920 doing is happening because of the emergency situation. 811 00:45:18,720 --> 00:45:20,640 Right. I find these two the team 812 00:45:20,640 --> 00:45:23,240 working agreement and also emergency playbook is something 813 00:45:23,240 --> 00:45:24,960 that everyone should check from your book, right? 814 00:45:24,960 --> 00:45:27,960 Because it really details a very very important thing that I 815 00:45:27,960 --> 00:45:29,920 think most teams are liking, right? 816 00:45:29,960 --> 00:45:31,720 Especially the team working agreement. 817 00:45:32,000 --> 00:45:34,520 So what you describe is like you make something like an 818 00:45:34,520 --> 00:45:37,640 expectation becoming more explicit and that you can 819 00:45:37,640 --> 00:45:40,240 enforce to the team, right, as a standard practice for them. 820 00:45:40,600 --> 00:45:43,000 So do check these two out from the book and I think really, 821 00:45:43,000 --> 00:45:45,520 really a lot of insights there. Of course there are other roles 822 00:45:45,520 --> 00:45:48,160 which we will not cover, things like who is in charge right, the 823 00:45:48,160 --> 00:45:51,640 TL, the EM or even the leaders from engineering right and also 824 00:45:51,640 --> 00:45:55,160 the organization at the end. So let's move to the next 825 00:45:55,160 --> 00:45:57,360 section which is about automation, right. 826 00:45:57,560 --> 00:46:00,480 I think in some teams which are advanced enough or using some 827 00:46:00,480 --> 00:46:03,600 system that provides all this automation is always nice. 828 00:46:03,920 --> 00:46:06,960 Tell us the role of automation in making code review better. 829 00:46:07,520 --> 00:46:13,200 So you be surprised at how many teams that I've spoken to that 830 00:46:13,200 --> 00:46:18,320 don't make more use of automation in terms of 831 00:46:18,640 --> 00:46:22,880 development automation. So there are a lot of code 832 00:46:22,880 --> 00:46:29,560 reviews that are spent fixing formatting or fixing spacing or 833 00:46:29,560 --> 00:46:34,160 changing naming conventions to a particular language. 834 00:46:34,440 --> 00:46:39,320 And these are all such low stakes issues that they really 835 00:46:39,320 --> 00:46:42,760 should not be in the code review because they waste a lot of time 836 00:46:42,920 --> 00:46:46,200 and they are also very subjective. 837 00:46:46,280 --> 00:46:49,160 They can get into very heated debates among developer 838 00:46:49,160 --> 00:46:52,120 preferences. And so that's another thing that 839 00:46:52,120 --> 00:46:54,320 should be decided upon by the team. 840 00:46:54,600 --> 00:46:57,400 Have a proper style guide, add that to the team working 841 00:46:57,400 --> 00:47:01,800 agreement and then make use of the automation that is for that. 842 00:47:01,800 --> 00:47:07,120 So linting, formatting, static analysis, running security tests 843 00:47:07,120 --> 00:47:10,200 that you can locally. These are all things that should 844 00:47:10,200 --> 00:47:14,280 be done during development. We have so many tools at our 845 00:47:14,280 --> 00:47:17,720 disposal as developers that integrate with our IDE. 846 00:47:17,720 --> 00:47:20,720 We have extensions galore and VS Code. 847 00:47:21,120 --> 00:47:23,320 We have you know, things like Resharper. 848 00:47:23,480 --> 00:47:27,120 There are so many things that are there to help us to do all 849 00:47:27,120 --> 00:47:29,080 of these things during development. 850 00:47:29,080 --> 00:47:33,280 And I really believe that if you fix all of these things as they 851 00:47:33,280 --> 00:47:37,160 happen, as you write code and when you can during development, 852 00:47:37,440 --> 00:47:40,160 the less likely they'll interrupt the code review 853 00:47:40,160 --> 00:47:43,440 process. And the more likely you'll get 854 00:47:43,440 --> 00:47:47,520 to use the code review process for those more important things, 855 00:47:47,720 --> 00:47:51,320 for actually focusing on the things that matter, either 856 00:47:51,320 --> 00:47:54,840 allowing the reviewer to actually just see higher stakes 857 00:47:54,840 --> 00:47:57,760 issues because they're not bogged down with like, oh, 858 00:47:57,760 --> 00:47:59,960 there's a space here, or you're missing a comma here. 859 00:48:00,200 --> 00:48:03,480 You are letting them focus on higher stakes issues, but you're 860 00:48:03,480 --> 00:48:07,440 also allowing people to actually get that knowledge transfer 861 00:48:07,440 --> 00:48:11,640 without being interrupted or bothered by all of these lower 862 00:48:11,640 --> 00:48:15,760 stakes issues. So automation is for those 863 00:48:15,760 --> 00:48:19,840 things first hand so that you can get rid of all of that noise 864 00:48:19,840 --> 00:48:22,400 in the code review. And then another part of 865 00:48:22,400 --> 00:48:25,960 automation that I know sometimes it's not really part of the code 866 00:48:25,960 --> 00:48:29,400 review, but I'm big proponent of because sometimes you can 867 00:48:29,400 --> 00:48:32,960 integrate it with them and those are all the kinds of checks that 868 00:48:32,960 --> 00:48:37,840 you can do in a process. So on GitHub specifically and on 869 00:48:37,840 --> 00:48:41,480 some other tools like Azure Repos or I'm sure AWS has an 870 00:48:41,480 --> 00:48:45,200 equivalent, there are some initial checks that you can do. 871 00:48:45,200 --> 00:48:49,520 For example on GitHub when you open up a pull request you can 872 00:48:49,520 --> 00:48:54,200 set it up to have an initial build go through and see if it 873 00:48:54,200 --> 00:48:58,160 will break and it's a pre build check. 874 00:48:58,400 --> 00:49:02,640 There are other tasks that you can have read also when you open 875 00:49:02,640 --> 00:49:05,840 APR. So if you have things to check, 876 00:49:05,840 --> 00:49:09,520 like there are some GitHub actions I think that allow you 877 00:49:09,520 --> 00:49:11,600 to check for a multitude of things. 878 00:49:11,600 --> 00:49:14,240 Do you have secrets in your code? 879 00:49:14,240 --> 00:49:18,440 Do you have some security? You know, like OS 10 issues that 880 00:49:18,440 --> 00:49:22,280 are glaring that somebody else may not have the time to check? 881 00:49:22,640 --> 00:49:25,320 Those should certainly be checked during development, but 882 00:49:25,320 --> 00:49:30,160 they act again as another checkpoint at the PR point to be 883 00:49:30,160 --> 00:49:33,960 able to check that as well. You have things like being able 884 00:49:33,960 --> 00:49:39,200 to automatically deliver, let's say, APR template to someone 885 00:49:39,200 --> 00:49:41,600 depending on the type of issue that they're doing. 886 00:49:41,600 --> 00:49:46,200 So, you know, I always go on and on about how the author needs to 887 00:49:46,200 --> 00:49:49,200 put as much information as possible in their PR. 888 00:49:49,440 --> 00:49:51,280 Sometimes that's a lot of information. 889 00:49:51,280 --> 00:49:55,400 Sometimes you just forget or you really don't remember all the 890 00:49:55,400 --> 00:49:57,640 things. And so something like APR 891 00:49:57,640 --> 00:50:01,680 template, for example, you can have a checklist basically that 892 00:50:01,680 --> 00:50:04,560 says, hey, you help the author. You say these are all the things 893 00:50:04,560 --> 00:50:06,960 that you need based on the type of pull request that you're 894 00:50:06,960 --> 00:50:09,040 opening. And then again, you set them up 895 00:50:09,040 --> 00:50:11,920 for success. You know, you let them help 896 00:50:11,920 --> 00:50:15,040 themselves by having this automatically appear when they 897 00:50:15,040 --> 00:50:19,360 open APR. So using automation in this way 898 00:50:19,560 --> 00:50:23,440 is, again, it's to help us help ourselves, right? 899 00:50:23,440 --> 00:50:27,480 It's to get rid of all of the noise, all of the lower stakes 900 00:50:27,480 --> 00:50:31,840 issues, and to give the reviewers the best chance of 901 00:50:31,840 --> 00:50:35,640 reviewing it in like a clean of a slate as possible. 902 00:50:36,000 --> 00:50:39,720 To really focus on the things that that automation can't find. 903 00:50:39,720 --> 00:50:43,680 That's where the human judgment, the nuance, the understanding. 904 00:50:43,920 --> 00:50:47,160 The example I really like giving is you can have all of these 905 00:50:47,160 --> 00:50:50,160 checks, you can run all of the static analysis and all of that, 906 00:50:50,520 --> 00:50:55,400 but the reviewer is the only one that can in a code review, say, 907 00:50:55,400 --> 00:50:59,360 hey, this variable name is actually not that meaningful. 908 00:50:59,360 --> 00:51:03,080 I don't understand it. Can you make it more meaningful 909 00:51:03,360 --> 00:51:07,800 And meaningful is completely lost, at least at this point, 910 00:51:07,880 --> 00:51:12,240 for AI, for any kind of static analysis tool, it would never be 911 00:51:12,240 --> 00:51:16,280 able to give that kind of surface, that kind of comment to 912 00:51:16,280 --> 00:51:20,080 help us fix it. So if we can eliminate all of 913 00:51:20,080 --> 00:51:24,040 the noise and allow the reviewers to focus on those 914 00:51:24,040 --> 00:51:27,520 kinds of issues, then I think we're doing a good job and we're 915 00:51:27,520 --> 00:51:31,480 really using the code review to its largest advantage and 916 00:51:31,480 --> 00:51:34,760 largest benefit. I think if as a team right you 917 00:51:34,800 --> 00:51:38,800 manage to remove all these accessories thing, I say like 918 00:51:38,800 --> 00:51:41,640 formatting, spacing, comma here and there, right? 919 00:51:41,800 --> 00:51:44,080 I think that's a good part that everyone should aspire. 920 00:51:44,480 --> 00:51:47,480 Another thing that you include in this automation is actually 921 00:51:47,680 --> 00:51:51,240 inclusive language. Kind of a check which brings me 922 00:51:51,240 --> 00:51:53,160 to the next thing about commenting right. 923 00:51:53,160 --> 00:51:56,920 Maybe a little bit of advice how we can comment code reviews 924 00:51:56,920 --> 00:51:58,240 better. Yes. 925 00:51:58,240 --> 00:52:02,560 So aside from the really good pattern I shared, which is the 926 00:52:02,560 --> 00:52:06,600 Triple R pattern, the you know, request rationale results. 927 00:52:06,600 --> 00:52:11,440 I think following that for most comments would be very good 928 00:52:11,440 --> 00:52:16,280 because it gives the author somewhere to work off of, and it 929 00:52:16,280 --> 00:52:20,240 also gives them some understanding or context behind 930 00:52:20,240 --> 00:52:23,120 why you're making it a request or that comment. 931 00:52:23,520 --> 00:52:27,160 But other things, I don't know if you've ever used a nitpick 932 00:52:27,320 --> 00:52:30,280 comment label. This is very debated. 933 00:52:30,280 --> 00:52:33,520 Like, I feel like you should not have nitpicks at all. 934 00:52:33,520 --> 00:52:37,880 But if you must have some sort of nitpick, which is like, it's 935 00:52:37,880 --> 00:52:42,520 purely subjective, but you have to tell the author that this is 936 00:52:42,520 --> 00:52:44,120 you know, you don't have to do anything about it. 937 00:52:44,120 --> 00:52:46,120 It's not a change. You just want to let them know. 938 00:52:46,120 --> 00:52:48,880 This is purely A subjective, preferential thing. 939 00:52:49,160 --> 00:52:52,960 Then my team likes to label that as a nitpick very clearly. 940 00:52:53,000 --> 00:52:57,640 And along the same lines, if you're able to kind of label 941 00:52:57,640 --> 00:53:01,280 your comments in that way, for example, if something needs to 942 00:53:01,280 --> 00:53:04,720 actually be done, make that very clear on the comment. 943 00:53:04,720 --> 00:53:08,120 So just like you would label a nitpick, nitpick at like bold 944 00:53:08,120 --> 00:53:11,400 first nitpick and then the nitpick you would say needs 945 00:53:11,400 --> 00:53:15,440 change for example and then you write your comment or needs 946 00:53:15,440 --> 00:53:17,360 rework. Let's take this discussion 947 00:53:17,360 --> 00:53:19,240 offline. And then you put your comment, 948 00:53:19,520 --> 00:53:23,960 you know, having that very clear upfront and being able to let 949 00:53:23,960 --> 00:53:26,880 the author read it. Having them read that as the 950 00:53:26,880 --> 00:53:31,280 first part of the comment saves a lot of cognitive load for them 951 00:53:31,280 --> 00:53:33,560 because then they can say, oh, these are the ones that are 952 00:53:33,560 --> 00:53:35,440 important that I should focus on. 953 00:53:35,720 --> 00:53:38,080 And then these are the ones that are not so important. 954 00:53:38,560 --> 00:53:41,920 And I think that is important because I don't know how many 955 00:53:41,920 --> 00:53:45,640 people are like me. But every time I see any comment 956 00:53:45,760 --> 00:53:49,760 on a pull request that I open, I just immediately associate that, 957 00:53:49,760 --> 00:53:53,040 oh, it's work I have to do. Every comment means I have to 958 00:53:53,040 --> 00:53:56,560 change something or update a line or do something so that I 959 00:53:56,560 --> 00:53:58,720 get a little bit worried. I'm like, oh, I got to do all of 960 00:53:58,720 --> 00:54:02,360 these changes. But then being able to organize 961 00:54:02,360 --> 00:54:06,160 my expectations as an author and says, oh, I actually only have 962 00:54:06,160 --> 00:54:08,480 one thing to work on. The rest of these are nitpicks. 963 00:54:08,800 --> 00:54:12,960 That's a much better way to receive feedback as an author. 964 00:54:12,960 --> 00:54:16,760 So that's one thing to help in the discussion. 965 00:54:17,240 --> 00:54:22,040 And then there are also code compliments, which I'm a big fan 966 00:54:22,040 --> 00:54:25,840 of, but within reason. So, like, you know, everybody 967 00:54:25,840 --> 00:54:29,480 thinks about code review as this very judgmental process. 968 00:54:29,480 --> 00:54:32,600 You're supposed to find what's wrong with each other's code, 969 00:54:32,840 --> 00:54:34,920 and it's part of why people dread it, right? 970 00:54:34,920 --> 00:54:37,520 It's typically seen as a negative thing. 971 00:54:37,840 --> 00:54:41,760 But if we try to turn this around and we say, you know, 972 00:54:42,080 --> 00:54:45,920 let's say somebody implemented something in a really novel way, 973 00:54:45,920 --> 00:54:49,840 like you haven't seen it before or they have a really awesome 974 00:54:50,120 --> 00:54:53,600 performance savings that you just are very impressed by. 975 00:54:54,000 --> 00:54:56,960 Say something, you know, say that's really awesome or good 976 00:54:56,960 --> 00:55:00,360 job in figuring that out. Or it's some, like, bug that 977 00:55:00,360 --> 00:55:03,640 everyone has been stuck on and then someone actually finds a 978 00:55:03,640 --> 00:55:07,000 fix for it with tests, we hope, right? 979 00:55:07,000 --> 00:55:09,840 And then documentation and all of these things. 980 00:55:10,080 --> 00:55:13,400 Please feel free to actually leave a comment or two. 981 00:55:13,440 --> 00:55:15,680 That's a compliment. If you see something you like, 982 00:55:15,920 --> 00:55:20,120 say so where it gets too much, as if you're leaving it for the 983 00:55:20,120 --> 00:55:22,920 sake of leaving it, you know, that's where I draw the line. 984 00:55:22,920 --> 00:55:26,200 Like if every other comment you're like oh this is awesome 985 00:55:26,200 --> 00:55:29,840 or oh that's great or oh this is it kind of loses its value 986 00:55:29,840 --> 00:55:33,040 because now you're saying everything is great, everything 987 00:55:33,040 --> 00:55:35,880 is awesome. And so I say code compliments 988 00:55:35,880 --> 00:55:37,920 within reason. If you find something really 989 00:55:37,920 --> 00:55:42,760 novel, I'm sure the author would really appreciate and like that 990 00:55:42,760 --> 00:55:45,520 you found some appreciation in the way that they wrote 991 00:55:45,520 --> 00:55:47,360 something. Just in general. 992 00:55:47,360 --> 00:55:50,040 Like you can sum it up as don't be a jerk. 993 00:55:50,600 --> 00:55:54,280 I know that might be harder for some people or maybe some people 994 00:55:54,600 --> 00:55:58,120 don't know if they are like you know, some people are more 995 00:55:58,120 --> 00:56:00,920 direct, some people are not as direct. 996 00:56:00,920 --> 00:56:06,320 So maybe that's actually one way where I would say AI, an AI 997 00:56:06,320 --> 00:56:10,880 based tool could help where if you are unsure you can you know, 998 00:56:10,880 --> 00:56:15,120 use an extension that says oh your sentiment is a bit more 999 00:56:15,120 --> 00:56:19,440 negative or it seems passive aggressive or you know, the tone 1000 00:56:19,440 --> 00:56:23,080 could be more neutral. Something like that is where I 1001 00:56:23,080 --> 00:56:26,120 think AI based tools could help in a code review. 1002 00:56:26,120 --> 00:56:31,360 But there are people that say you know, let's have AI write 1003 00:56:31,360 --> 00:56:36,360 the comment for us. Sure you can, but again I always 1004 00:56:36,360 --> 00:56:39,720 hesitate to say fully lean on that because you still have to 1005 00:56:39,720 --> 00:56:42,520 review it. Anything that is AI generated or 1006 00:56:42,560 --> 00:56:45,800 AI produced, it still needs to be reviewed by a human. 1007 00:56:45,800 --> 00:56:49,760 So sure, if it saves you some time and you can let some tool 1008 00:56:49,760 --> 00:56:54,120 write the comment for you in a more neutral tone, just know 1009 00:56:54,120 --> 00:56:57,720 that you still need to look at it to make sure it's correct and 1010 00:56:57,720 --> 00:57:02,040 #2 to make sure it's actually saying what it wants. 1011 00:57:02,080 --> 00:57:05,840 You know, the intent of what you actually wanted to say to make 1012 00:57:05,840 --> 00:57:09,000 sure that's still in there, so you don't want to lose that part 1013 00:57:09,000 --> 00:57:13,280 of the communication in there. So in terms of comments, yeah, 1014 00:57:13,280 --> 00:57:15,880 you know, code compliments within reason. 1015 00:57:16,160 --> 00:57:20,400 Don't be a jerk and try to be as objective as possible, as 1016 00:57:20,480 --> 00:57:25,520 outcome focused as possible, and just try to write the comments 1017 00:57:25,520 --> 00:57:29,000 that you would want to receive as a reviewer in the most 1018 00:57:29,000 --> 00:57:31,680 objective way as possible. Yeah, don't be a jerk. 1019 00:57:31,680 --> 00:57:33,280 I think have a lot of empathy, right? 1020 00:57:33,280 --> 00:57:36,120 For the people who either the author or the reviewer, right? 1021 00:57:36,400 --> 00:57:39,120 And also I think it's very important, like you mentioned, 1022 00:57:39,120 --> 00:57:41,840 forget the ego, right? Like you should not have ego 1023 00:57:42,080 --> 00:57:43,800 Attack the code, not the person, right? 1024 00:57:43,800 --> 00:57:46,920 I think that's also important. So one very crucial aspect. 1025 00:57:46,920 --> 00:57:49,600 The last question I have from the conversation is about you 1026 00:57:49,600 --> 00:57:51,440 know, code reviews that takes too long. 1027 00:57:51,480 --> 00:57:54,200 I think this is quite common in many teams and this is also an 1028 00:57:54,200 --> 00:57:57,200 anti pattern I believe. What can you say for teams to 1029 00:57:57,200 --> 00:58:00,240 actually maybe making the code review speed faster? 1030 00:58:00,520 --> 00:58:03,600 And also interestingly right the last state of DevOps report 1031 00:58:03,600 --> 00:58:06,360 mentioned about code review speed as the one technical 1032 00:58:06,360 --> 00:58:09,520 practice that teams should aspire to become a more elite 1033 00:58:09,520 --> 00:58:12,200 kind of a performance. So maybe a little bit advice 1034 00:58:12,200 --> 00:58:13,520 here. Sure. 1035 00:58:13,680 --> 00:58:19,440 So code reviews can take a long time due to a lot of reasons. 1036 00:58:19,440 --> 00:58:26,080 But it mostly comes down to the PR itself is too large, and then 1037 00:58:26,080 --> 00:58:31,000 the PR itself being too large is a symptom of other issues. 1038 00:58:31,160 --> 00:58:33,680 So we've touched on the other ones before, right? 1039 00:58:33,680 --> 00:58:36,200 They take too long because they're gigantic. 1040 00:58:36,320 --> 00:58:40,680 They take too long because the author has not put enough 1041 00:58:40,760 --> 00:58:45,160 information in the pull request. So as a reviewer, you're kind of 1042 00:58:45,360 --> 00:58:47,400 sifting through. You're trying to make sense of 1043 00:58:47,400 --> 00:58:51,640 what's happening, and you can't really understand why this code 1044 00:58:51,640 --> 00:58:53,760 change is happening the way it is. 1045 00:58:53,960 --> 00:58:58,320 If you have a person, a single person who's doing a code 1046 00:58:58,320 --> 00:59:01,320 review, that could be a reason why it takes too long. 1047 00:59:01,320 --> 00:59:04,840 So it's something I call the single senior developer 1048 00:59:04,840 --> 00:59:07,760 bottleneck problem. They're the only person that can 1049 00:59:07,760 --> 00:59:11,840 review, which means they must be on all the PRS and they're only 1050 00:59:11,840 --> 00:59:14,720 a single person. So, you know, take that into 1051 00:59:14,720 --> 00:59:18,360 account and then multiply that. If the PRS that they have to 1052 00:59:18,360 --> 00:59:22,440 look at are large and are non descriptive, you get the idea. 1053 00:59:22,440 --> 00:59:26,000 It can take very long that way. So we've talked about how 1054 00:59:26,240 --> 00:59:28,480 spreading the knowledge might ease that problem. 1055 00:59:28,480 --> 00:59:31,400 You level up the rest of your team so that there's not just 1056 00:59:31,680 --> 00:59:37,000 one reviewer available, but multiple reviewers available who 1057 00:59:37,000 --> 00:59:41,120 are able to share the load and then the thing that I said which 1058 00:59:41,120 --> 00:59:46,520 is try to make your PRS smaller. So one thing that is very common 1059 00:59:46,520 --> 00:59:49,920 is, you know, I say atomic changes or like keep it to a 1060 00:59:49,920 --> 00:59:55,080 single feature or single bug fix and sometimes the feature 1061 00:59:55,080 --> 01:00:00,080 itself, it is a single feature, but it's still like 30-40, fifty 1062 01:00:00,080 --> 01:00:03,120 files. Or a lot of lines. 1063 01:00:03,360 --> 01:00:07,000 And so when it gets to that point, the thing you have to 1064 01:00:07,000 --> 01:00:10,920 look at at that point is maybe there's something that's lacking 1065 01:00:10,920 --> 01:00:13,360 in your design or planning phase. 1066 01:00:13,680 --> 01:00:18,920 And so why did the feature get scoped to be that large? 1067 01:00:19,320 --> 01:00:22,600 Did it actually start out small and then scope creep made it 1068 01:00:22,600 --> 01:00:25,080 that large? Or did you actually all agree 1069 01:00:25,080 --> 01:00:27,840 and say no, Yeah, this is a single feature. 1070 01:00:27,840 --> 01:00:30,760 I think we can do it, and this is going to be fine. 1071 01:00:31,240 --> 01:00:35,440 If it happens to be that case, that's where you need to start 1072 01:00:35,440 --> 01:00:38,160 planning on your next design and planning phase. 1073 01:00:38,160 --> 01:00:42,440 And for the foreseeable future, if you don't want large PRS, 1074 01:00:42,440 --> 01:00:45,400 then you're going to have to scope your features down much 1075 01:00:45,400 --> 01:00:48,840 smaller. So really think about the 1076 01:00:48,840 --> 01:00:52,640 feature and if it has to have everything that you are thinking 1077 01:00:52,640 --> 01:00:55,960 about. Because if you absolutely must 1078 01:00:55,960 --> 01:01:00,160 have it all together in a single feature or at least in one task 1079 01:01:00,160 --> 01:01:04,040 that is being worked on, then just anticipate that the PR is 1080 01:01:04,040 --> 01:01:06,880 going to be that large, especially if you're trying to 1081 01:01:07,240 --> 01:01:09,320 keep to the single feature PR role. 1082 01:01:09,320 --> 01:01:13,880 So just to have people be a little bit pedantic about that, 1083 01:01:14,000 --> 01:01:18,520 it's more about is the change itself as small as it can be. 1084 01:01:18,880 --> 01:01:22,360 And then if you are able to get it that way, the resulting PR is 1085 01:01:22,360 --> 01:01:24,640 going to obviously be much smaller. 1086 01:01:24,920 --> 01:01:29,000 So the other thing that makes PRS really, really long is, 1087 01:01:29,240 --> 01:01:33,840 let's say somebody asks a question, somebody answers back, 1088 01:01:34,040 --> 01:01:37,480 somebody doesn't understand, so they ask to clarify and then 1089 01:01:37,480 --> 01:01:40,280 someone tries to explain it and then they don't get it. 1090 01:01:40,280 --> 01:01:43,640 And then it just becomes a really long discussion online 1091 01:01:43,640 --> 01:01:48,120 asynchronously. If you have the ability to get 1092 01:01:48,120 --> 01:01:52,040 on a video call, or if you have the luxury of seeing the person 1093 01:01:52,040 --> 01:01:56,120 in person, sometimes it's best to just if it's like two or 1094 01:01:56,120 --> 01:02:00,040 three comments or four comments in, just take it off and 1095 01:02:00,040 --> 01:02:03,440 actually speak with each other synchronously, because then you 1096 01:02:03,440 --> 01:02:07,280 have a greater chance of getting through whatever communication 1097 01:02:07,280 --> 01:02:10,400 block that is happening. You can walk through the code 1098 01:02:10,400 --> 01:02:14,920 together, You can go through the description and have them fill 1099 01:02:14,920 --> 01:02:18,320 in what is happening. Sometimes the result of that 1100 01:02:18,320 --> 01:02:22,000 conversation means there's actually stuff that was missing 1101 01:02:22,000 --> 01:02:25,040 from the description, and so you can just ask the person, hey, 1102 01:02:25,040 --> 01:02:27,200 can you add this to the description and then 1103 01:02:27,200 --> 01:02:29,960 everything's fine. Sometimes that discussion 1104 01:02:30,200 --> 01:02:35,200 actually reveals that oh, this is actually work or changes that 1105 01:02:35,200 --> 01:02:40,240 don't need to happen right now and we shouldn't have this PR. 1106 01:02:40,240 --> 01:02:44,800 And then you find the reasons for it and then you close the PR 1107 01:02:44,800 --> 01:02:46,320 because you don't actually need it. 1108 01:02:46,520 --> 01:02:49,200 And then the important thing here is to actually let everyone 1109 01:02:49,200 --> 01:02:53,680 else know on the team, Hey, we are closing this PR because of 1110 01:02:53,680 --> 01:02:56,560 XYZ reason that we figured out on this discussion. 1111 01:02:56,920 --> 01:03:00,800 This is the important part about discussions that happen offline 1112 01:03:00,800 --> 01:03:05,400 is whatever the result of that discussion is, it's important to 1113 01:03:05,400 --> 01:03:08,720 let everybody else know, not just for knowledge transfer and 1114 01:03:08,720 --> 01:03:12,440 for any of those teammates who may be looking at the PR with 1115 01:03:12,440 --> 01:03:15,800 you, but later down the road. Again, the documentation, the 1116 01:03:15,800 --> 01:03:19,920 historical aspect, if you are wondering why did this PR close 1117 01:03:19,920 --> 01:03:22,440 or how come we never merged this in, this actually would have 1118 01:03:22,440 --> 01:03:25,320 fixed something. And then if you just leave one 1119 01:03:25,320 --> 01:03:28,600 single comment that says, hey, we took this conversation 1120 01:03:28,600 --> 01:03:31,800 offline, we talked about this, this and this and this was the 1121 01:03:31,800 --> 01:03:34,640 result of our conversation. This is why we're closing. 1122 01:03:35,240 --> 01:03:38,840 Think of how much headache you would have, you know, eliminated 1123 01:03:38,840 --> 01:03:41,720 in the future, even for yourselves, because you just 1124 01:03:41,720 --> 01:03:43,800 took the time to write that out, right? 1125 01:03:43,800 --> 01:03:47,760 So if you find discussions are getting too long, try that. 1126 01:03:47,760 --> 01:03:51,360 Try taking it synchronously and trying to work it out, and then 1127 01:03:51,360 --> 01:03:56,040 please go back to the PR and write down what happened as a 1128 01:03:56,040 --> 01:04:00,520 result of that discussion. So all of these reasons, they 1129 01:04:00,520 --> 01:04:05,080 tend to prolong the code review process because there's just too 1130 01:04:05,080 --> 01:04:08,920 much cognitive load for the people involved, right? 1131 01:04:09,400 --> 01:04:12,080 And that can come from the multitude of reasons that we've 1132 01:04:12,080 --> 01:04:15,640 spoken about. But if you focus on making the 1133 01:04:15,640 --> 01:04:19,840 PRS smaller, if you focus on, you know, airing on the side of 1134 01:04:19,840 --> 01:04:23,200 communication and trying to talk to people and getting that 1135 01:04:23,440 --> 01:04:28,000 understanding faster, I think a lot of the reasons that make 1136 01:04:28,000 --> 01:04:32,000 code reviews long will actually go away and then code reviews 1137 01:04:32,000 --> 01:04:35,320 will be the quicker, more efficient process that they're 1138 01:04:35,320 --> 01:04:37,000 supposed to be. Right. 1139 01:04:37,600 --> 01:04:40,040 And I would also say that make it as part of the work, right, 1140 01:04:40,040 --> 01:04:42,680 make it as part of the definition of done so that you 1141 01:04:42,680 --> 01:04:45,440 kind of like expect code review to be done as part of the work. 1142 01:04:45,760 --> 01:04:48,920 And I think what you mentioned all of that could be included in 1143 01:04:48,920 --> 01:04:51,360 the team working agreement. So again I think it's a very, 1144 01:04:51,360 --> 01:04:54,320 very important artifact for the team to kind of like agree on 1145 01:04:54,320 --> 01:04:57,920 the expectation explicitly. So thank you so much for you 1146 01:04:57,920 --> 01:05:00,720 know covering a lot of code review best practices. 1147 01:05:00,720 --> 01:05:03,600 Adrian, I have just one last question before we end our 1148 01:05:03,600 --> 01:05:06,280 conversation, which I call the three technical leadership 1149 01:05:06,280 --> 01:05:09,560 wisdom we can think of it just like an advice that you wanna 1150 01:05:09,560 --> 01:05:13,440 leave to us to learn from you. That's a good question. 1151 01:05:13,440 --> 01:05:18,200 I think the first one stemming from my own experiences is don't 1152 01:05:18,200 --> 01:05:21,440 be afraid to switch to something new. 1153 01:05:21,680 --> 01:05:25,160 That happened to me when I got my student technician job. 1154 01:05:25,160 --> 01:05:28,520 I had no idea I was first resetting passwords for people 1155 01:05:28,520 --> 01:05:33,280 and then it soon blossomed into why is my computer slow to how 1156 01:05:33,280 --> 01:05:36,280 do I install this. It becomes more and more problem 1157 01:05:36,280 --> 01:05:39,680 solving of a higher level and I found out that I actually really 1158 01:05:39,680 --> 01:05:43,600 enjoyed that. And then taking the chance and 1159 01:05:43,600 --> 01:05:47,280 applying to technical conferences and actually 1160 01:05:47,680 --> 01:05:50,120 speaking at conferences and doing that. 1161 01:05:50,120 --> 01:05:51,800 I didn't know I was going to be good at that. 1162 01:05:51,800 --> 01:05:55,040 I tried it Anyway. That's my full time job now and 1163 01:05:55,040 --> 01:05:58,440 I'm actually pretty good at it. So you never know what might 1164 01:05:58,440 --> 01:06:01,760 happen if you don't try it, especially if it's something 1165 01:06:01,760 --> 01:06:05,440 that seems interesting to you. Try it and then you can say, oh, 1166 01:06:05,480 --> 01:06:08,440 that's not for me and that's the worst thing that can that 1167 01:06:08,440 --> 01:06:11,600 happen. Second piece of wisdom, I wrote 1168 01:06:11,600 --> 01:06:15,840 this in a really long article back then, but I I said don't be 1169 01:06:16,080 --> 01:06:18,960 afraid to fail. I know I said in the beginning I 1170 01:06:18,960 --> 01:06:24,960 was afraid to fail, but what I find is if you are unafraid to 1171 01:06:24,960 --> 01:06:31,840 fail gracefully, I'll put that you will not only increase your 1172 01:06:31,920 --> 01:06:36,000 skills and knowledge in whatever it is that you're doing, but 1173 01:06:36,000 --> 01:06:41,040 you'll actually level up faster because you learn what it is to 1174 01:06:41,160 --> 01:06:44,800 solve the problem that initially made you fail in the 1st place. 1175 01:06:45,120 --> 01:06:49,040 And so instead of being afraid, or instead of being like me 1176 01:06:49,040 --> 01:06:51,920 initially where I didn't want to break anything, I walked around 1177 01:06:51,920 --> 01:06:54,320 egg shells. I stayed stagnant for a while 1178 01:06:54,320 --> 01:06:56,760 because I just didn't want to break anything. 1179 01:06:56,960 --> 01:07:00,080 But then the first time I broke something where I actually 1180 01:07:00,080 --> 01:07:04,920 deleted a couple rows in a table in a database, I was so freaked 1181 01:07:04,920 --> 01:07:07,400 out like, Oh no, I'm going to lose my job, Oh my gosh. 1182 01:07:07,680 --> 01:07:12,600 But that's where I was able to talk to ADBA to say, hey, this 1183 01:07:12,600 --> 01:07:15,000 is how you actually roll back something. 1184 01:07:15,200 --> 01:07:19,280 Here's how you can do these operations in a staging 1185 01:07:19,280 --> 01:07:22,960 environment where it's not as problematic or not as scary. 1186 01:07:22,960 --> 01:07:26,440 And then here are the ways that you might go through this 1187 01:07:26,440 --> 01:07:31,240 process again, but do so in a way where you can easily revert 1188 01:07:31,240 --> 01:07:34,480 back so that you're not as scared to make changes. 1189 01:07:34,760 --> 01:07:38,480 So if I had never made that failure, if I had never deleted 1190 01:07:38,480 --> 01:07:41,640 those rows, I would have just been stuck, you know, doing 1191 01:07:41,640 --> 01:07:43,640 everything as carefully as possible. 1192 01:07:43,880 --> 01:07:47,960 And I would have never gained that knowledge of environments, 1193 01:07:48,000 --> 01:07:51,640 of doing things in a way that can be reverted back. 1194 01:07:51,840 --> 01:07:54,760 And in doing things in a way with more confidence. 1195 01:07:54,760 --> 01:07:58,200 Because I know that I can fix it if I need to. 1196 01:07:58,200 --> 01:08:01,160 So fail fast and fail gracefully. 1197 01:08:01,520 --> 01:08:05,280 And then the last thing I will say for tech leadership wisdom. 1198 01:08:05,720 --> 01:08:08,880 As I've gone through my career, I've gone through what I feel 1199 01:08:08,880 --> 01:08:11,440 like is everything. I've written a book. 1200 01:08:11,440 --> 01:08:13,880 I'm writing one now. I'm doing courses. 1201 01:08:13,880 --> 01:08:17,399 I've done the Instagram thing, trying to get followers, all of 1202 01:08:17,399 --> 01:08:20,840 that stuff. And at this point in my career, 1203 01:08:21,040 --> 01:08:23,640 I feel like you should have one hobby that you just don't 1204 01:08:23,640 --> 01:08:27,479 monetize. I feel like it's very prevalent 1205 01:08:27,560 --> 01:08:31,520 in the tech industry to always have another hustle. 1206 01:08:31,520 --> 01:08:35,439 You're building a SAS on the side, you are monetizing. 1207 01:08:35,479 --> 01:08:39,080 I don't, I don't know, building websites SEO friendly, like all 1208 01:08:39,080 --> 01:08:42,359 of these things are cool. But at the end of the day, if 1209 01:08:42,359 --> 01:08:47,359 everything becomes a goal to make money, it kind of loses its 1210 01:08:47,359 --> 01:08:49,760 luster. You kind of get burnt out. 1211 01:08:50,160 --> 01:08:54,880 And I've gone through it before where things that were fun, that 1212 01:08:55,040 --> 01:08:58,279 did become a job because the focus was to try to make money. 1213 01:08:58,600 --> 01:09:02,000 Just I didn't like it anymore. And not only that, you put a lot 1214 01:09:02,000 --> 01:09:05,080 of pressure on yourself to make it in the tech industry. 1215 01:09:05,080 --> 01:09:08,479 And so I say have a hobby that's just for you. 1216 01:09:08,520 --> 01:09:13,000 You bake and you eat bread and you don't post about it, You 1217 01:09:13,000 --> 01:09:15,800 just enjoy it. You enjoy it with butter or jam 1218 01:09:15,800 --> 01:09:17,880 or whatever, and that's your thing. 1219 01:09:18,120 --> 01:09:19,800 You don't have to share it with anybody. 1220 01:09:19,880 --> 01:09:21,319 You don't have to make money on it. 1221 01:09:21,319 --> 01:09:22,640 You don't have to share the recipe. 1222 01:09:22,640 --> 01:09:25,600 You don't have to have a bread baking course like there's 1223 01:09:25,600 --> 01:09:28,200 nothing about it. Just have it for yourself. 1224 01:09:28,200 --> 01:09:32,040 And I think a lot more of us will stay a lot more balanced 1225 01:09:32,040 --> 01:09:34,560 when it comes to navigating the tech world. 1226 01:09:35,399 --> 01:09:36,479 Thank you for sharing the last one. 1227 01:09:36,479 --> 01:09:38,200 I think it's really, really beautiful, right? 1228 01:09:38,200 --> 01:09:40,399 So not everything should be monetized, right? 1229 01:09:40,399 --> 01:09:43,040 Have a hobby that only you can enjoy it? 1230 01:09:43,040 --> 01:09:47,200 That's also perfectly fine. So Adrian, if people want to 1231 01:09:47,200 --> 01:09:50,840 follow you or maybe contact you for more discussions, right? 1232 01:09:50,840 --> 01:09:53,600 Especially regarding code review, where can they find you 1233 01:09:53,600 --> 01:09:57,280 online? Yes, so LinkedIn actually it's a 1234 01:09:57,600 --> 01:09:59,840 pretty good place. I'm pretty active on there. 1235 01:09:59,840 --> 01:10:04,680 And then Twitter X depending on what you wanna know it as and 1236 01:10:04,680 --> 01:10:07,320 then Instagram. I'm also there so my handle will 1237 01:10:07,320 --> 01:10:10,640 be Adrian. Teca is my first and last name 1238 01:10:11,040 --> 01:10:14,040 and you should be able to find me there and I will do my best 1239 01:10:14,040 --> 01:10:16,200 to answer. So please, if you have any 1240 01:10:16,200 --> 01:10:19,960 questions about code review, I'd be more than happy to answer 1241 01:10:19,960 --> 01:10:21,960 them. And when I'll be expecting the 1242 01:10:21,960 --> 01:10:25,000 book to be published. So that just changed. 1243 01:10:25,320 --> 01:10:28,000 Yeah, it's going to be summer of next year. 1244 01:10:28,120 --> 01:10:33,280 So Chapter 7 is actually going to be released very soon on Meep 1245 01:10:33,280 --> 01:10:36,360 and that's the chapter on code review comments. 1246 01:10:36,360 --> 01:10:41,240 So keep an eye out for that. But after that I have 8910, I 1247 01:10:41,240 --> 01:10:45,560 have four more chapters to write and I have a lot of stuff that I 1248 01:10:45,560 --> 01:10:48,720 want to add to the existing chapters actually. 1249 01:10:48,720 --> 01:10:51,240 So a summer 2024? Keep an eye out. 1250 01:10:52,240 --> 01:10:55,160 Looking forward for that, I've read the MIP right, so I find it 1251 01:10:55,160 --> 01:10:57,080 really really good. So I think all developers should 1252 01:10:57,080 --> 01:10:59,400 check this book out. So thanks again Adrian for your 1253 01:10:59,400 --> 01:11:01,440 time. Thank you, Henry. 1254 01:11:01,560 --> 01:11:06,600 It's great being here. Thank you for listening to this 1255 01:11:06,600 --> 01:11:09,000 episode and for staying right until the end. 1256 01:11:09,360 --> 01:11:12,520 If you highly enjoyed it, I would appreciate if you share it 1257 01:11:12,520 --> 01:11:15,480 with your friends and colleagues who you think would also benefit 1258 01:11:15,480 --> 01:11:18,280 from listening to this episode. And if you're new to the 1259 01:11:18,280 --> 01:11:21,280 podcast, make sure to subscribe and leave me your valuable 1260 01:11:21,280 --> 01:11:24,240 review and feedback. It helps me a lot in order to 1261 01:11:24,240 --> 01:11:27,560 grow this podcast better. You can also find the full show 1262 01:11:27,560 --> 01:11:30,400 notes of this conversation on the episode page at 1263 01:11:30,400 --> 01:11:33,880 techlitjournal dot dev website, including the full transcript, 1264 01:11:34,120 --> 01:11:37,760 interesting quotes, and links to the resources mentioned from the 1265 01:11:37,760 --> 01:11:40,600 conversation. And lastly, make sure to 1266 01:11:40,600 --> 01:11:43,600 subscribe to the show's mailing list on techlitjournal dot dev 1267 01:11:43,960 --> 01:11:46,400 to get notified for any future episodes. 1268 01:11:47,000 --> 01:11:50,480 Stay tuned for the next Techly Journal episode, and until then, 1269 01:11:50,680 --> 01:11:51,240 goodbye.