Coding Blocks - Google’s Engineering Practices – Code Review Standards

Episode Date: May 11, 2020

We dig into Google's engineering practices documentation as we learn how to code review while Michael, er, Fives is done with proper nouns, Allen can't get his pull request approved, and Joe prefers to take the average of his code reviews.

You're listening to Coding Blocks, episode 132. Subscribe to us and leave us a review on iTunes, Spotify, Stitcher, and more using your favorite podcast app. And stop trying to mess me up. I see you guys talking and mimicking me and mocking me as I do this. It's not going to work. You didn't trip me up. I don't know what that outburst was about.

We'll just keep going though. So you can visit us at codingblocks.net. We can find show notes, discussion, and a lot more. And if you have questions and rants, you can be sent to comments at codingblocks.net. So if you speak in a language other than English or any known language to man, then nobody else can try and act like they're speaking like you. So follow us on Twitter at codingblocks or head to www.codingblocks.net and find all our social links there at the top of the page. With that, I'm Alan Underwood.

I'm Joe Zacht. And I'm Michael Outlaw. This episode is sponsored by the University of California, Irvine Division of Continuing Education. One of the top 50 nationally ranked universities, UCI offers over 80 certificates and specialized programs designed for working professionals.

And Datadog, a cloud-scale monitoring and analytics platform that unifies metrics, traces, and logs so you can identify and resolve performance issues quickly. All right, and hey, today we're talking about some Google engineering practices, and we're talking about some Google engineering practices.

And we're going to tell you what that's about in a minute here. But first, we got a little bit of news. And as we do, got to say a huge thank you for everyone at Letras Review. We've got a couple names to shout out here. From iTunes, thank you, Jay Barger, Podcast Devourer. I dig it. And Darachi, thank you so much.

And from Stitcher, we have Diamond C. Wait, Damien? Damien C? Damien C, yeah. That would probably have been a better way to read it the first time. Whatever. We all know that names are hard.

Proper nouns, man. Just all all proper nouns can we do away with them uh we're gonna just go by goods i think wouldn't it be nicer really it could be like star wars the clone wars man you know just like everybody go by a number man i'll pass you don't understand there was a day not that long ago we spent all day trying to figure out why the heck something wasn't working and turned out that the the guids were slightly different there was like one number difference in like the middle of them in the middle i remember that like we spent hours scratching our heads trying to figure out what was wrong and nobody picked up on that one it was like an A was an O or something. They were super

close. I'm saying, let's just go by numbers. You can call me fives. How's that? I'll go by fives. You remember in that case it was the Java library we were using, UUID, you would take in a seed and then generate a number, but then it would like or

two of the bits in the number. So two of them were always off. But we didn't realize because it always looked like we thought two libraries are generating goods the same way but one of them just like always generated the good and then slightly tweaked it so frustrating man it's insane all right so my apologies then to damien for messing up your name and with that let's get into discussing the Google engineering practices. So this was a, uh, I'm like 99% certain that this was something that micro G shared with us in Slack that I've been saving for a while, but, um, it's possible that I might be attributing that wrong, but I thought it was at the time. At any rate, we'll have a link to this document or this set of documentation, basically Google's engineering practices.

There's a lot of great content in here, and we're going to start with code review and their developer guide to how to perform them. So before we do this, are we just totally abandoning designing data intensive applications? We just taking a break from it for a minute. Is this like a Ross and Rachel thing? We're on a break. I think, I think we're on a break because that stuff was deep. So we'll probably go back to it, but yeah. All right. So, so the first question is what is a code review? I don't know. Isn't it one of those things where somebody sends you a link and you just go click approve? Well, I mean, the reason I say I don't know is because depending on who does it can be very different.

Yeah, so we'll probably cover some of that. The definition that they gave on their page was when somebody other than the author of the code examines the code that, I mean, it's pretty simple and that sort of makes sense, right? Sounds complicated. Depending on who reviews it, it can be. Um, so then the next question is, but why? Because I mean, you guys, I know you remember the days when there wasn't any such thing, right? Like you wrote some code and then you deployed it out somewhere. Or you were like, yo, could you give me the link to production so I can copy these files up? You know, there was a day where that was how things were done. I mean, think 20, 25 years ago, you didn't even have commits or pull requests. Like that wasn't even a thing.

You're just like, okay, I wrote some code. I checked that file in and have at it. Trust that I did a good job and then it works. Well, in the days of SourceSafe, remember those, it wasn't even your stuff needs to work. It was just, hey, yo, check it in before you leave so that the files aren't locked so that when I come in tomorrow, I could get them, right? Yep. Yeah. And source safe is exactly what I was thinking of as I described what I was just talking about. Yeah. Yeah. So, so yeah, the real reason here is we're long past those days now, right?

Now the day is, hey, we want to make sure that whatever code is being checked in is actually maintaining or improving the standard of quality of the product as a whole, right? I do want to just go back one moment, though, because there's probably some people who are new to programming that all they know is the wonder of Git. And so they're like, wait a minute, I don't understand why Visual Source Safe was such a problem. So I want you to just imagine real quick for a moment. If you, if, if when you needed to check out code,

then it worked the same way that going to your local library worked. And being that like, if you check out that book, no one else could check that book out until you returned it. And that's the way visual source safe work worked. When you checked out that file, no one else could check out that file and modify it until you returned to the file until you checked it in.

So brutal. It was so brutal. It really was. And that's our 1990s in review. Right. All right, so what to look for then when we do a code review. So we've established that we want to do a code review to keep the quality of our code high and to improve upon it. But when we're asked to review a code, what should we look for? Yep. Yep.

And looking at the list here, definitely it's nice to kind of have a list. I'm just wondering how nice it would be to have a checklist that you kind of were able to go through and and kind of rate this stuff is uh if you don't keep this stuff in mind it can be easy to just kind of look at it and be like i don't see anything obviously wrong moving along so it's nice to kind of have a formal structure for this yeah i mean we'll go through the structure real quick but it is even some of these items though before we get to it i'll say that like even when i've done code reviews, some of these, I'm like, I don't know how much time I would be willing to spend on some of these areas that they, that they asked for. So one thing that's curious is I haven't seen in this documentation,

like how much time they expect a reviewer to spend on a code review. So I think we should do this because I think where Joe is going with this is, and we've done this in the past is maybe as we hit each one of these bullet points, just say how important it is to us, high, medium, or low. Right? And because I think that's a good way for us to just throw in what we feel is important when we're looking at one. Okay. So up first is design. Oh.

Right? Is the code well designed and appropriate for your system? What does this even mean to you guys? Yeah, that's so nebulous. That's such a big one. This is vague. Yeah. Yeah, like well-designed?

Like, wait, I'm supposed to go and rethink and re-engineer? Like, well, you know, you used the wrong data structure here when this other one would have been more appropriate. Like, I don't know, maybe some of those will jump out to you, but this is an example of one of those where it's like, how much time are you really expected to spend on this? Like, oh, you didn't recognize that this was a graph problem and, you know, you should have solved it this way. Like, yeah.

I think this one might be more along the lines of design patterns, right? Did you use an appropriate design pattern? Was this clearly a factory thing? Was this clearly something that should have been a template design pattern? Something like that. So I don't know. I feel like that's kind of the stuff you'd be looking for there. And by the way, so I said I was going to share this on the show.

The irony is when I started reading this, I just did my first major pull request into an open source repository, like a big one out there. And this stuff drove me crazy, man. I put in this stuff that I was like, oh man, this makes it better. This improved the functionality. And I've now got 500 comments back and forth between myself and the people who are maintaining the project. And it's, I'm not even kidding. It's almost to the point where it's like, yeah, I'm just going to let this die because I really don't want to spend this much time on it. Right. But on the flip side, I kind of get that they want to make sure that they're maintaining a high quality of standards going into theirs. Right. So it's, it's a trade-off, but it can be frustrating, especially when you're like, yo, i'm giving you guys something here right like take it be happy with it um but well here's some things

that aren't in other areas is like um maybe you have some stuff hard-coded that should have been settings or maybe you kind of invented a new pattern for settings or something when there's already established pattern maybe you kind of reinvented something that's done like maybe you go out and get uh customer orders and there was already a method for getting customer orders that could have been more appropriate for your use case. Oh, so this is the design.

So I think the important part here is you're basically saying the reviewer has more knowledge of the overall system. And so they can tell you if you're doing something that you design something in a way that wasn't kosher with the way that the rest of the system works. I like that. It was like, Hey, why are you using integer primary keys here when everything else uses goods?

Like why, why break that pattern? Do you have a good reason for that? Well, we're kind of, I mean, I get your point. This is kind of skipping ahead though,

to like picking who should be your reviewer. Right. Yeah. And it would be like, you know, uh, somebody who is the, like the owner that would know would be like, you know, uh, somebody who is the, like the owner that would know, like be able to know, uh, you know, I mean, we'll, we'll come back to it, but

you know, that's kind of where you're going at it with Joe. So, so how important is this design thing to you guys? Like you outlaw, where, where do you rate on this high, low hmm i mean it's definitely not a low uh it is it is tough to like you know for that person to always know like what was the best way to attack the problem so because of that i would say it would rank medium to high somewhere in that range what about you joe i i would say uh i would say hi okay i'm actually high on this one too based off how joe kind of brought this to the front right like why are you not following the patterns of the rest of the product and the code here whatever i think that's super important to keep the complexity of your project down right and, and make developers be able to work in the thing properly. So that one's good.

So we're, we're all basically high or medium high on this one. So the next one that we have is functionality. Does the code behave as the author likely intended is, is the way the code behaves good for the users that are using that code. So functionality, what do you guys think? High, medium, low? I'm going to go high with this one.

I mean, if it doesn't work right, then it doesn't work right. And I've definitely seen code checked in where the equal signs or whatever, it's like checking the wrong thing in some weird case. I'm staying here. High. If it doesn't do what it's supposed to do, then don't let it in the code base. Yeah. I mean, this one goes let it in the code base. Yeah.

I mean, this one goes without saying. This is automatically a hi. I don't think anyone would ever question. It's actually shocking to me that it's even a bullet point here, to be completely honest with you. So you needed to add a button that does X. Did you add a button that does X? No. Okay.

Well, then this doesn't go. Yeah. I don't get it. Well, I mean, keep in mind this is a Google documentation and they did hire Owen Wilson and Vince Vaughn. I saw. There was a documentary movie called

Interns where you can see their escapades trying to get hired at Google. So it makes sense why they would have to spell that out. Did you see that? I tried to watch it. It was not funny.

Yeah. I didn't watch it. I wanted to watch it. I tried to. So next up we have is complexity. Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it? So what are you guys' take on this one?

Low, get good. Okay. So, so compared to the other two, compared to the other two, I, I would definitely say it's more like a low to medium kind of range.

So, um, yeah. Okay. So we're kind of at yes, yes, yes on this one. I, too, think that this is a low. However, depending on whether somebody was just trying to be clever in what they were doing, you know what I mean? Like, there can be a case where complexity makes sense for performance or things like that, right?

So, I'm things like that. Right. Um, so, so I'm fine with that. And I think that if people don't understand it, then yeah, level up, right. Like be able to understand the code. And I hate it.

I hate being that way, but this one to me is as long as it's not intentionally trying to be clever, which I have seen code like that, then I think this is a low. Yeah. It really depends on like what you're doing. That's, that's complex. And that's why I ranked it like low to me. And like, if you were doing something like really, really complex unnecessarily, so then I might be more willing to, uh, you know, call you out on it. And like, like with the purpose of like, no, no, no, you really, this is way too much.

You know, you need to fix this before we merge this in. But otherwise, yeah, I might be willing to let it go if it's just like, Oh, I wouldn't have done it that way, but whatever you want to factory hammer,

hammer, hammer, or hammer factory, factory factory. You know, what jumps out at me is like the code coders that you've seen where people try and solve a problem. That's obviously, you know, it should have been a 20 line thing and they do it in a single line with 80

lambdas and okay like you went too far that that's that's kind of where where i feel like that's where you can basically say yeah don't do something like that nobody can read it um but outside of that kind of stuff meh yeah what about uh somebody says i don't need to bring in a third party i can uh write a dependency addiction framework just for the little pieces that i need that would be simple and easier to maintain those people get slapped hard that's got to be a different deal there that's more along the lines of design right yeah yeah i i we can't even talk about that. My blood pressure is going to go up.

Um, so this one, the outlaw, I think you should read this one off. I think this one's sort of your wheelhouse here. Tests. Does the code have correct and well-designed automated tests? I would rank this one as high.

I, I, I like the idea that you have Tess with your kit. Yeah, of course, Wink. I definitely also rank this one high. Wink, wink. So I'm on this one too high. And it's funny because I've always known the importance of Tess, but it's definitely been within the past couple of years where it's actually sunk into something that I feel like needs to be practiced.

This is actually something, by the way, that drives me crazy with the PR that I put in the other day is I put one in. And the thing that I did, you can create 500 different tests for this thing. It's dealing with dates. Okay. And so just consider the two episodes that we did on dates and date formats and, and trying to convert from one to another and all that kind of stuff. And so I had like, I don't know, 10 tests in there and they're like, well, maybe you should test more. And I'm like, what's the limit? Like, I don't really know what I'm supposed to do here. But yes, I still, I'll rank this one high, but it's definitely frustrating because there's some things that just aren't that easy to do. You went from zero to 10 and your response is maybe we should have more.

Man, that's where I would have gotten frustrated. You know, I mean, whatever it is, what it is, like I said, they're trying to make sure that their code quality stays good. And, you know, I mean, whatever it is, what it is, like I said, they're trying to make sure that their code quality stays good. And, you know, I'm trying to do my best. So. All right. So we've all agreed on all of them so far. All right. Naming.

Did the developer choose clear names for variables, classes, methods, et cetera? Mike. This one is a low in my mind like I mean if you gave it some awful names this the reason why I gave this one low is like I might call it out and be like wait what why would you name it that

but I'm not going to stop a pull request from being merged in just because you know you use the variable X and I'm like why can't you give it a more meaningful name? Or even if you called some class a helper and I'm like, I really hate the name helpers and managers and things like that. But I'm not going to stop the pull request for it.

What about you, Joe? Yeah, same deal. Some people are really strict about how things are named and get methods should only get. I mean, there's people that are really strict about like how things are named and like get methods should only get and stuff. And, you know, I agree with all that. That's all good. It's all good stuff. I care more about other things more. So I'm going to put this one as like low medium.

All right. I'm going to break the trend here. This one to me is medium straight up. I think like X. Yeah, probably not going to care. Right. But if you have a method name, that's public, that, that things are going to be using, especially APIs, that kind of stuff, I'm absolutely going to hammer you on something that, that is not

clear, right? Like this thing says that it saves, what is it saving? What are you doing? Like, like there will be certain things. So again, this one, it's not going to ring the bell on a bunch of stuff, but if, if it's obvious, then it's a medium for me. All right. All right. Comments are the comments clear and useful. This should be an interesting one.

So this one kind of feeds into an upcoming one in my mind, but for me, uh, like I'm assuming they're talking about code comments, not the comments on the PR. I'm assuming they're talking about comments in your code, right? Yeah. That's what I assumed too.

Okay. And for the most part, I've kind of grown to this style. Like I would prefer you to not need the comments period. Like if you're going to put a comment in there, there better be a really good reason why it was needed. The only comments that I would like to see in it would be summary level documentation comments that an automation tool can pull out and create, like here's an API documentation for you. Those comments, fine with those,, those should be clear and useful.

Yes. But the other kind of comments I'd prefer to not see, you know, they aren't in line in your method, you know, which to me goes back to the naming and why the naming so important. And that's why I made that one a medium. Cause if you have a good name for your method,

your classes, that kind of stuff, your comments should be less necessary at that point. And keeping in spirit of, you know, things that we've learned from Uncle Bob is that if you keep your methods small to where you can just easily look at it and reason about it because it's only a few lines long, then you shouldn't really need a comment in a short method. So outside of public API type stuff, like what you're talking about, things that generate Java docs and that kind of thing, you're kind of a low on this one.

Well, because of the other, because of the summary level documentation, I'm kind of like torn on how to rate this one. So I'm guess, I guess I would call it... Yeah, I guess I'll go with low. Low, sure. Okay. Joe? You know, medium, I don't love comments, so if I see a bunch of useless comments or if I see commented out code, then I'm not too

keen on that. But yeah, it's not the worst thing in the world. There's definitely been times that I've read comments in code and been thankful that they've been there. So I could let that one slide low. All right. Yeah, I think I'm also a low on this one. I think after going through the Uncle Bob thing with the whole comments and all that,

and like you said, smaller methods, all that, I too would rather the code be easier to reason about, but I also do agree. And this is one thing that drives me crazy about most open source projects is sure. They'll put in comments that create your Java docs that are, that are just one step away from being useless because they'll tell you, Hey, there's a variable here. It says file. Okay, it's a file name. Of what? What is it saving?

What is this? Or it'll be a method named like get customer ID, and the documentation says returns the customer ID. And you're like, really? Of what? Yeah, yeah, totally. So if you're doing public API stuff, I would probably bump this up higher. But in general speaking, I would go low here.

Well, I mean, like by public API, though, you really mean like if it's summary or Java doc kind of documentation. It doesn't mean that it has to be an API that's made available to the public. Yeah, yeah. That's what I mean. Exactly what you said. Okay. All right. So next up, style.

Does the code follow our style guides? I'm sure that we might be all over the board on this one. If you have a published style guide and you've gotten people to agree to it, then it better fit the style guide. If you don't have a style guide, then no one gets to say crap in my opinion. You don't get to enforce your personal style preferences unless you've all agreed as a team to go with one okay so this i think the way that this thing is worded assumes that there is a style guide so does the code follow our style guide so in this case how important is this to you when a pull request goes in hi okay this google absolutely has a style guide and they link to it in this, in this article.

So what about you outlaws? Is this a high, medium, low? Um, well, okay. So here's the thing.

Like personally, if I'm reviewing it, it's kind of like a low. And the reason why I say that is because you can catch this with automation tools to where you don't have to like, I don't have to like memorize the style sheet, the style guide in my head. And then,

you know, look, cause I've definitely had pull requests that I've submitted to other repos before this. Like, Nope, you didn't follow the, uh,

the style guide. So it got rejected. And I'm like, Oh man, what did I. Like, nope, you didn't follow the style guide, so it got rejected. And I'm like, oh, man, what did I miss? And then, you know. So I'm kind of like, for me as a person to look at this one, I look at it as low. And I think you said medium, Joe? I said high.

You said high. If you have a style guide, yeah. I'm sorry. So I hate, like, if your code base at all looks the same, like, I remember a time when we had a bunch of variables that were named me and JavaScript to represent the scope. And regardless of whether or not you like that variable name, it was there everywhere.

And one person came in, decided they didn't like it, and so they did self. Right. And this particular framework made a lot of use of that. And you would see self, self, self. And so you always knew when you were looking at their code. And so it was just annoying to me to see, like, oh, the function here above it is me. The function below is me.

Everywhere in this code is me. The function in between is self. So, yeah, I don't know. It felt like a big F you from that person to say, you know, like, I'm not doing it your way. This is the way I like it, and I don't care about you. But hold up. You just said you were low on this a second. No, so he was like you said you're high all right yeah my bad yeah i'm a high if if one person or a minority of people are flouting the accepted rules and i think

it's yeah not cool okay so on this one then i'm probably splitting the difference i'm a medium for for both the reasons that you guys said. So one, there is automation tools, especially for typed languages that can catch most of that. And it will actually fail the build. Right. So from that regard, I shouldn't be looking at the PR if it hasn't passed the build. Right. My opinion. Don't send me the pull request until this thing's passed the build.

So that's one. On the other side, though, there probably are some tooling, like, I don't know, SQL. I'm not sure if there's a bunch of style guides that are enforceable for things like SQL, that maybe it's not. But here's where I'm toward, and this is me being a jerk. Like, if somebody makes a style guide and they tell me that you have to do Hungarian notation, I'll be like, no, I'm not doing it, right? Or the type where they put the type at the front of a variable, like, I'm not doing it. Right. Or, or the type where they put the type at the front of a variable. Like I won't follow it. I refuse to. Wait, that is Hungarian notation there, right? Is that Hungarian? Yeah. Like I, I will try and sneak in and change the style guy,

but yeah, so I'm probably a medium on this one. I think it's important that people follow at least with the conventions that have been set. Yeah. Yeah. Yeah. And that's a, yeah, I guess it's important that people follow at least with the conventions that have been set. Yeah. Yeah. Yeah. And that's, yeah, I guess it's if everyone doesn't agree on the style guide, then that's kind of a different story. Right. I mean, at some point you just got to, you know, commit and move forward, but still.

And then the last one that we have here, documentation. Did the developer also update relevant documentation with whatever the code was that they wrote? Hardly ever. No, no they don't. Maybe it's important, but they don't. But how important is this to you? I guess if it's a public API or something, if that's really what we're talking about, then it's super important. So if the documentation matters at all, then it's probably very important. So it's a high to you in some cases. If the documentation matters, which so rarely does. Yeah, it's a tough one.

What about you, Outlaw? I'm going to go medium. I mean, like it really is going to vary by project. If your project already has good documentation that is being shipped with it, then yes, you would need to update it as you change things. But more often than not, like the projects that I'm working in, you don't have that luxury. So this isn't a thing. This doesn't come up.

Because it's not like the developer is shipping the documentation with it, is my point. Right, right. The apps that we're writing, the code's not being used developer is shipping the documentation with it, is my point. Right, right. The apps that we're writing, the code's not being used. The application's being used. So it's a different thing. For me, this one's tough, right? Like if I'm writing something as a developer that other developers are going to use, this is a high to me. Because this is where I feel like Microsoft kills it.

Like they are so good. If you go to a Microsoft documentation page for how to do some code, typically they'll have the code there. They'll have the syntax. They'll have the variables. They'll even have some nice human written descriptions of what they are. And then they'll have some examples. That, to me, should be the standard that everybody should shoot for when developers are going to be the consumers of the documentation. Oh yeah.

They've set the bar high. They set the bar super high. And that's where I get frustrated with open source projects in general is like, there's this, we're building something, you know, go dig through the code and figure it out. Right.

And I mean, it, it raises the barrier of productivity to extreme levels when you're digging through tons of java code or tons of c-sharp code to figure out how to do something right so so to me this one's a high depending on on who the consumer of that documentation is did you know that google also has a big guide on technical writing no same website no yeah i'll put a link in the show notes cool so so you were a high on this one yeah i was a high on this one all right so i've been taking some notes here and uh my my uh oops

wait where'd i go? Lost my mouse. I, you were like a little bit more optimistic about like, Hey, we all agreed on these than I was. And by my count, we didn't. So looking through it now, uh, wait, that can't be right. Google, you messed me up. We matched on like five of them. I want to say five of the eight or something. No, no, we did.

Really? We only, we only all agreed on three of them. We agreed on functionality tests and comments. We all said we were all basically the same. I think you had said like medium high or something and we were like, hi. Oh, so you're making those distinctions. All right. Well, yeah, because that counts, right? Well, we were supposed to choose three. You went and changed the rules on us. Well, yeah. Refer back to the functionality

and whatnot. Our style. See? See the style. I was like, no, forget it. I will use self. And that was really awkward when you were talking about me there, Joe. Yeah, we only agreed on functionality where we all agreed that was high, tests where we agreed high, well, I was going to say it was always two to three, but there were two of them that we all completely disagreed on. Uh, so the, the ones that we, the three of us completely disagreed on were naming, uh,

where you, Alan was a medium, Joe was a low to medium and I was a low. And then style was the other one that we all three had a different opinion on. So Alan, medium, Joe, high, and me, low. But the other ones that we didn't agree on, it was usually like two of us were on the same side and somebody else was the odd man out. Cool. So the next up we have is how you pick the best reviewers for the code.

And this one's interesting. And I mean, I think we all three do do similar things on this, but the first is get the best reviewer you can. And that basically means someone who can review your code, who has the knowledge and can do it within the given timeframe. Right. Yeah. Yeah. I think that's, that's something I definitely, uh, am a big believer in. So I don't like the idea of throwing it out and just saying anybody,

but I mean, don't get me wrong. I do that all the time, but I, uh, I try not to, if it's anything less than, you know, if it's anything more than trivial. Right. Yeah. I mean, that's the thing is it really depends on what the pull request is. If it's a pull request that is really minor, then I don't care. Let everybody get a little bit of understanding into how the system works, even if it's a part that they're not, you know, intimately familiar with, right? You know, if it's something, if the pull request is something simple as like, you know, you, I'm trying to think of something like really simple, but like maybe you renamed a variable, for example, like Alan didn't like my previous one.

And so, yeah. Or there was a typo. Yeah, sure. There was a typo in the UI. And so you correct that. Then fine. But when it's a decent pull request where it has functionality in it, then I definitely go after a reviewer who is most knowledgeable in that specific area or maybe most recently familiar with it because code evolves too.

So the person who might have been like the original author and, and owner of it, you know, two years ago might not know squat about it today. Right. Right. And this is, this is one of the things that they point out here.

So the best reviewer is the one who can give you the most thorough review. And to your point, that may just be whoever's most recently been in the files, right? Not, not the original creator or whatever. It's kind of on you. Now, one of the things that they said about this, and this was interesting, I didn't dig into it, is they said this may or may not be somebody that's in the owner's file. So it sounds like they have some sort of cataloging of where they put people who are the owners of certain portions of a system right

and and i know the three of us have talked about this with the companies that we've worked with over time because on big systems it can be sort of a pain in the butt to find out who uh hey who owns this over here and and if you got 50 people on a team that's a lot harder than if you've got three people working on a team. Right. So, which way everyone's like, nah,

not me. I'm busy. Right. Right. So I don't know. I didn't, I didn't dig into that one, but that is interesting that they said it may or may not be something in the

owner's file. So, you know, you kind of got to go find basically what they're getting at is it's sort of on the person who coded it to go find the right person to do it. Like when Outlaw said that, or I think it might have been Joe said, you know, sometimes I'll just throw it out there and be like, hey, whoever approve it. And what we're talking about is like maybe you're on a system like Slack or Hangouts or Microsoft Teams, and you'll just throw a pull request link out there and be like, hey, somebody go look at this, right? When it's something more important than it's not, Hey, let me throw this into a public lobby.

Let me throw this to Joe Zach because I know that he worked in this code last week. I need him to look at it. Yeah. And their next point here too, is that like different people might be needed to review different portions of your changes. And this one actually hits close to me, like, you know, close to the heart for me, because it's actually a pet peeve I have with, um, so, you know, we use Azure DevOps and I love Azure DevOps. There's a lot of things that I love about Azure DevOps. It's it, it, you know, it, it makes a lot of things in your life simple, right? But one of the pet peeves that I have is that you can set up a policy so that when you do want to create a pull request into a particular branch, you can say, hey, the pull request will require X number of reviewers before it can be approved

and merged into that particular branch. Like let's say you wanted to merge into master, right? And you know, you might, you might say, okay, fine. Let's say you have to have some kind of a branch policy on it to exist. And, but if you do have a branch policy, then that means that you as the creator of the pull request can just say, Hey, you know what? When all of the branch policies have been met, auto-complete my pull request and merge it into master, right? All of that sounds great and wonderful, and it is. But where it falls apart, in my opinion, is in the case where it's like, okay, let's say that I know that I want to have multiple people review this pull request because it touches a couple different pieces. And so maybe I want to have multiple people review this pull request because it touches a couple different

pieces. And so maybe I want somebody who's, you know, more familiar with the data portion of our application and like, you know, the rules about like how we how we get to the data, what data is available for what reasons, like, am I even adhering to the standards of like, how you know, I'm accessing the data. And then maybe there's like a, you know, somebody else from like a front end tier that I want to have review some of it and some of it from a middle tier. Cause you know, as full stack developers, we we're, we're committing pieces that touch all parts of the system. Like that's kind of in our job title. Right. Right.

And, and so you might want to have like in that case, three, three different reviewers. Well, if you have a branch policy that requires fewer than that, you can't, as the author of the pull request, say, hey, do not automatically merge my pull request in unless all of these people that I suggest approve it. Because if any one of them rejects it, I don't want it to automatically be approved. But instead what happens is let's say that the policy was two people are required to review it. And if they, if they approve it, then it automatically gets merged in and I have a sign and I want three people to review it. Well, the first two to review it, boom, it's done. And then the third person comes along. It's like,

well, why did you even ask me to look at it? It's already merged in. Yeah, basically what you're saying is you can't override the behavior on a single PR. You set the policy for that branch, and that trumps everything. And that does kind of stink because I've definitely run into that before. You could turn auto-merge off, but I like that. Well, you can. Yeah, right.

So that's the alternative, right? That's how you get around that lack of ability to override is instead you say, well, I just won't turn on auto complete, right? But now that's kind of, I don't like that either because then it's like, hey, what if I want to give this thing to you and I'm going to be out of town and I'm going out of town, right? Like I want it to be able to auto complete whenever it's ready and agreed upon. Right. Yeah. It's definitely a shortcoming of the tool. The last thing that they mentioned here that I think is important. And I think all three of us probably do this as well is if the best person isn't available because they're swamped,

they have other things that they need to do. You should still CC them on whatever the CL is, which by the way, in all of this stuff, we're talking PRs. So pull requests, that's sort of a get term, I think more or less, maybe it's a more standard universal type term, but in the documentation, they refer to everything as a change log. So a CL. Change list. That's right. Um, so they're just saying that,

Hey, if, if you can't find, like if Michael's not available, I should still CC him on that change list so that he knows what's happening. And, and maybe he can't do anything before the thing gets merged in, but he might have some thoughts that you might go in and,

and after the fact, go in and change those things. Right. So it's a good practice. People used to put the changes like at the top of the file. Those are awful. I hate. Yeah.

You have like this long header file of people maintaining it. Like, you know, we added this or whatever. Yeah. August, 2016. Yeah. Michael touched this file to add blah blah i hate that it's but if your changes it like encourages you to do changes in one file rather than multiple because it's just kind of weird to

right add a change to support something yeah and you add that in 10 places yeah it's annoying yeah not a fan you don't want to be the one jerk that like breaks the chain like you see like oh man this thing goes back to like 1997 i'm gonna be the one that uh doesn't add to the top here you gotta buck the trend man yeah i've actually been that guy yep same so i guess i would have broken the comments and style portion and documentation of uh what to look for in my code review. Those are highs, man. Guilty.

All right. So next up they have is this section called in-person reviews. And I thought this was kind of interesting. Like they said that, like, hey, if you pair program to somebody that would have been qualified as somebody to have done a code review, then you can consider that code reviewed. Don't worry about it, right? That can be merged in. See, I didn't agree with this one. Did you? Oh, I totally do. I completely do. I see it so biased.

But again, it's if the person was qualified, like not just pair program with anybody, but if you were the creator or the owner of a chunk of code and you're pairing with me, then you should be giving me constant feedback during that pair programming thing. Right? Here's the reason why I don't, here's the reason why I thought this one was, um, well, I didn't agree with this one is because it's kind of like saying, Hey, you're the owner of it. Nobody has to review your code. You're the owner. No, but you got two people working on it at the same time.

Right. So, so let's pretend that one of those is an intern that's just starting and you're mentoring him and now, and now he, yeah, you're pairing with him, but you're the owner of it. And now it doesn't have to be reviewed because you're quote the owner. I still think that like, even if there were the two people and even if one of them did happen to be the owner, as you suggested, it should still go to an unbiased set of eyes to take a look at it. I can see that. I think that that's more of an edge case than the norm. So, yeah, I see your point, but I'd still say that on a large portion of the time,

this would probably be okay to merge in. Yeah, totes. I'm fine with it. No. Outlaw trusts nobody. He's got a trust nobody policy. All right. Yeah, yeah yeah and then the other one was um you can also do reviews and this one was really interesting because i've never done this you can do reviews where the reviewer can ask questions and the coder is only allowed to speak

when answering those questions so the coder can't go in there and be like, hey, let me show you everything I did. They have to keep their mouth shut until they need to answer a question. It felt like a very rude way to say it. Yeah. Where it's like, you only speak when you're spoken to, sir. Like, uh-uh, uh-uh, I didn't ask a question. Stop it. Simon didn't say.

That's right. But I kind of like it. I kind of like it because that almost puts the reviewer in the mindset of, I need to understand this stuff on my own. Can I understand this by going through and looking at it? And then if I need to ask a clarifying question, I can. Right? And so instead of somebody leading the witness, the witness can go through and find out what they need and then inquire about the things that they need some additional info on. Now, the three of us, though, have worked in it from the opposite form where like for a code review, you would have a reviewer and then you would walk that reviewer through and say, like, here's what I did here.

Here's why I did this change. And, you know, that way the person reviewing it doesn't have to like spend their afternoon trying to reason about what you did, why you did it or whatever. You could just like walk them through it and then they could immediately go like, hey, wait a minute. You know, why didn't you just use like a Bellman Ford algorithm here or, you know? They might come at it with their infinite knowledge. You should probably just get three different reviews and take the average of the approval. It even kind of makes sense to have two different people implement the work and just pick your favorite, maybe even more. If you got those Google dollars or whatever. Google dollars.

Oh, man. If you got those Google dollars or whatever. Google dollars. Oh, man. This episode is sponsored by the University of California, Irvine Division of Continuing Education. Maybe you've heard of this thing called Python. Python is one of the fastest growing programming languages and UCI's Python Programming Certificate Program will prepare students for opportunities in web development, data analytics, core software development, and a wide range of scientific and mathematical applications.

Students will learn programming concepts including programming styles, idioms, libraries, data structures, data retrieval, processing, visualization, and networked application program interfaces and databases. UCI certificates in data science, predictive analytics, and machine learning will prepare students to gain the necessary skills to land the job in data science. Additionally, those interested in predictive analytics and machine learning will learn to improve and optimize business performance. If you're looking to become competitive in the global marketplace, advance your career, or start a new one, UCI has the resources to support you on your new path. And let's face it, right now is an awesome opportunity to take advantage of remote learning. Registration is now open. Visit ce.uci.edu slash codingblocks to learn more and reserve your seat.

That's ce.uci.edu slash codingblocks and reserve your seat today. Okay. All right. Yeah. Go on to the bathroom. I got this.

Yep. Yeah. You guys take a break. Don't worry. Don't worry. All right. It's just us. So I got us crypto jacked. Don't worry. All right. It's just us.

So I got us crypto jacked. We got ransomwared. It's a bit of a problem. And this is Joe, by the way, being silly again. Allah and Alan are out of the room. And I made a big mistake. And the only way to get me out of this is with more reviews. If you can go to cuttingblocks.net slash review,

that helps us find more people. More people means more money. More money means I can pay off this ransomware and get my WordPress back. So I'd appreciate it if you go to codebox.net slash view and hook us up because yeah, everything's good. Yeah, everything's fine. Everything's fine.

Okay, good. We ready to do this? Yeah, yeah. WordPress is great. Let's do this. WordPress? Did you have a pull request? Survey says.

That's my line, sir. Oh, yeah. Sorry. Continue. All right. So with that, let's get into my favorite portion of the show. Survey says. All right.

So a few episodes back, we asked this very important question to just find out, like, how much are you like, Mike? And the question is, do you leave your laptop plugged in the majority of the time? And your choices were all the time. I don't care about the battery or no. I try to maintain my battery's life expectancy by neither fully charging it nor discharging it or no, but not because I care. All right. Let's say Joe goes first. Which one do you think is the most popular answer and by what percent?

Okay. So I remember that McDonald's won the French fry contest. Based on that, carry the one. I'm going to say all the time. I don't care. And I'm going to give it, time. I don't care. And I'm going to give it, oh, I don't know, 30, 40%. 30, 40. What?

30, 40. 30, 40%. All right. Very good. Does that mean 35%? Like, I need a percent. What's the number? I'm sorry.

30, 40%. You did it again, sir. I got that. We're good. Because I'm going to go ahead and I'm going to also jump on this bandwagon of nobody's like Mike. And they're going to say all the time, I don't care about the battery. And we're going to go with 41%. So it trumps his 30-40%.

Okay. Yeah. All right. So Joe goes with all the time with 30, 40%. And Alan goes with all the time with 41%. And the winner is... It's Alan. Of course it is.

Wow. It's probably like 70% is my guess. It was 68%. is. Wow. It's probably like 70% is my guess. It was 68%. Oh, yes. My people. Wow. How many people actually did what Outlaw does, which is, by the way, I try to maintain my battery's life expectancy by neither fully charging or discharging it. How many did this?

5%? No. Six. It was, it was more than that. Gosh, it was, I don't know.

8%. Shut up, man. Don't judge me. But you know what? That 8%, they're there on top of it. They're a really smart 8%.

All right. Let me give you guys a little story behind this. Oh, geez. Here we go. So Outlaw, who is the— Outlaw lines up his battery before he puts the— Yeah. Before he salts it. so in all truthfulness like he is the most meticulous about maintaining this stuff right

but we've both had the same like macbook pros for years with the companies we've worked with and his turned into a rocking chair well before mine did meaning the battery swelled did all kinds of nastiness right i don't know that i had ever unplugged outside of bed like but yours was swollen too in fairness the difference was i i had mine repaired two years later mine started to swell so while you baby yours i abused mine and treated it like every person should treat their computer. And mine went a lot longer before it turned into that swollen rocking chair. So, yeah, I'm sure there's some truth to this actually saves your battery a little bit. But I'm of the opinion I don't want to think about it.

And I'll just buy a replacement when it's time. So, you know, it's funny. My wife hates stuff not charging. It drives her nuts. Like my phone isn't 100% it's time. It's funny. My wife hates stuff not charging. It drives her nuts that my phone isn't at 100% all the time. So she's constantly going behind my back and plugging stuff in. Actually, I am more like outlaw. I'm not strict about it, but if I'm done with something, I unplug it. I don't need it charging.

So I'm constantly somewhere between 20% and 80% most of the time. Really? Oh, man. There you go. Dude. You don't see my phone right now. Boom. You want to guess what the percent is?

Coding blocks. 66% is in this category. 84%, actually. Mine is 100% and plugged in. Because that's what it needs to be. I don't want to think about it. My wife is constantly, I'm like, where's my phone? I know it's on the couch.

She's like, it's plugged in. Now you're welcome. That's not what I wanted. I wanted it on the couch. Oh, man. Wait, wait, wait. You have a Nintendo Switch. Do you charge that to 100%?

No, actually, I do the same thing. Actually, just right before we were coming up, she came up here and she asked for the switch because she assumed that it wasn't in its proper place charging. And actually it was for once. That's awesome. She doesn't even check there first anymore.

She just automatically asked me because who knows? It could be anywhere. For the record, my phone is at 92%. Yeah. So 66 percent of coding blocks follows my rule of thumb by not the eight percent of the people eight percenters right here no no no but i'm talking about of of the host 66 percent of this crew followed eight percent of the rest of the world who decided to vote on that. You know, I mean, I'm just saying.

We're a very smart crowd. And, yeah. Hey, listen, when your computer stops working, you'll know how to find us. I'm going to come steal your battery because I know it'll have been treated right. Well, you won't be able to. I'm sorry. Apple glues them in. I know.

God. I know. God. All right. So how about for this survey, for today's survey? I thought this would fit well with our topic here. Do you always include new or updated unit tests with your pull requests? I see Joe winking. It's like really painful to watch him drive. It doesn't look like he's really practiced at it.

Winking does not come natural to you, sir. Nope. All right. So your choices are, yes, of course, not a psychopath. Or not with every pull request, but more often than not. I mean, what kind of psychopath has the time to do it for every pull request? Or lastly, no, I rarely include any tests with my pull requests. My friends think I'm crazy.

Did we ask a question about how many people unit tested it was like 20 percent yeah nobody nobody so we're good we're gonna get 100 of those 20 that answer these questions well or hey hey give them some credit though they that that that uh survey was a long time ago they may have grown personally since then and now they do a lot more unit tests. You don't know. That's true. You know what? I am actually stunned that Joe Zack, the king of having a 40% – saying that somebody is going to win with 40% of the vote when there's two answers, calling out our math abilities.

Okay. You're going to talk to me, Mr. Babillions? Hey, these things need to scale. That's right. That's all there is to it. You can't argue with internet scale. Oh, God. We have a true or false question.

I'm going to go with true and 30%. Pretty sure I won that one, just saying. Joe, Zach, you have some epic moments that have been recorded for the internet. Your math and your cheese dust are definitely up there, sir.

Yes, they are. Although, we've gotten some interesting tips on how to get the best Dorito out of the Dorito bag. Oh, really? Yeah, Jamie from Gaprogman from the.NET Core show, his brother actually had a really nice tip for

laying the bag on its side and then souffle-ing it open with an interesting kind of knife or something in order to vertically be able to uh sift through and find the best no but dude that just that's such a mess then what you're gonna do assuming you don't eat the entire bag in one sitting like that creates another problem you're assuming you didn't. I only eat the best chips. Dude, Joe, I remember you actually posting a picture on Amazon about some Lays chips that you bought. It was amazing. I remember this.

It's still up there. It's the top review for that one, by the way. Oh, man. I want to go back and find it. What was it again? It was cheddar. It's cheddar and sour cream chips on Amazon. Let's see if it's still the top review.

Oh, the rating has dropped. So just for people who are not familiar with this, because I'm sure nobody is. I don't know that we've ever talked about it. He was probably one of the first people ever to buy a bag of chips on Amazon because who doesn't do that at the grocery store? But he was worried that when they came, they were going to all be crushed up. That was the concern. You got the UPS guy kicking the bag of chips around.

That's going to be a problem. So his review, take it away, Joe. I'm not having a good time finding because there are like several different like ways of, I don't know, just the way they have, there's like 20 different kinds of chip now for this kind of chip. Well, yeah, the whole point was he basically took a chip out of the bag. He's like, they're not broken at all. See this chip. And he had it like next're not broken at all. See this chip? And he had it like next to it. Was it your hand or your wife's hand?

It was Sarah's hand. Yeah. So I've never gotten chips delivered before. I always got them from the grocery store. And I was worried that it would be all broken. But man, they weren't. Every single chip in this bag that I got was perfect. It was like perfectly round.

Everyone was giant. Yeah. And one of them I put in my wife's hand, it was the size of a potato. It was the whole chip. You would never get that at a grocery store. UPS did better than the grocery store ever did. This was right when they...

Oh, I found it, Joe. You did? I did find it, yeah. That's awesome. This was right when Amazon first started delivering food, if I recall. We need this link. This has got to go in the show notes. Here it is. I just sent you guys the link.

Oh, wait. I'll put it in the show notes, though. That is amazing. So that I can remember to. Oh, then you guys can see my other reviews. Yeah, this should be interesting. We might have just opened up Pandora's box. I don't see it.

Do I not see my own reviews? Yeah, no, no, no. You go to scroll down to all customer images and then you can scroll down. Yours is toward yours is like one of the first like seven or eight pictures. You know, so you got to scroll down to the bottom of the list and you'll see this giant chip in a hand. Man, there are so many pictures of this. Yeah, I think that, I think Joe said, he was a trendsetter here. How do you not see this?

Really? I'm actually trying to get to, this is ridiculous. I gave you guys the link, right? I'm going to, oh, most recent. You can't go to, you have to go to all reviews. Yeah, yeah. No, just click on see all customer images. Oh, here we go.

And then scroll down to the bottom. Yeah, yeah. Click on see all customer images. Scroll to the bottom of that. And then it's on the second to bottom row. Or at least on my screen it is. You'll see this gigantic chip being held in an open palm. Yeah, man.

Look how big that is. And he says, we saved it for a while. I like how Joe still gets excited about it. Look how big that is. Look at this. Flawless execution. This is from 2013. That is amazing.

Yeah, man. Well, if you look, the new reviews aren't so kind, man. I see people talking about bugs and chips and people complaining about them being broken, man. This is at the prime. This is at the peak Amazon period, 2013. This is back when I still worked for them. That's all you know. Today's episode of Coding Blocks is sponsored by Datadog, the unified monitoring platform for real-time observability and detailed insights into Docker performance.

Enhance the visibility into your container orchestration with a live container view and easily detect clusters that are consuming excess resources using an auto-generated container map. Let's face it, guys. Like, in a Kubernetes kind of world today, you want to know what's happening inside your pods. You want to know what your containers are doing and you need a tool like data dog so that you can easily see what's going on. And it's hard, but that's okay because out of the box data dog collects critical metrics from each Docker container. So you can get immediate visibility into aggregated and disaggregated service-level traffic. So try Datadog today by starting a free 14-day trial and receive a Datadog t-shirt after creating just one dashboard.

Visit datadoghq.com slash codingblocks to get started. Again, that's datadoghq.com slash coding blocks to start today. All right. So now we are back with how to do a code review. And I guess this is where the rubber meets the road, right? Wow. We talk a lot. It's already an hour. Oh my gosh. Yeah. Oh yeah. So we'll try going any faster with that kind of talk, sir. Yeah.

I just, I looked down and saw the numbers. I was like, wow, that's crazy. All right. So we'll try and, uh, try and blow through these.

Not really, but whatever. Um, so first is the standard of a code review. And, and we said it at the very beginning. What, what is the purpose of it is to make sure that the quality of a code review. And we said it at the very beginning. What is the purpose of it? It's to make sure that the quality of your code is improving over time.

Always. This is where those, what were they? The standards that we reviewed, like what to look for. This is where that list of things come into play. Because there's going to be a set of trade-offs. And that's why when we were ranking things like low, medium, high, it's like, well, would I block a pull request for this? Like, I don't know, maybe, maybe not.

And this is why. Yeah. The first bullet points hilarious to me because developers actually need to accomplish a task. It's so true. It goes back to this PR that I have into the open source project. It's like, man, like, I don't know how many times I want to go back and forth. And I'm not trying to throw stones at these people that are trying to, you know, ensure that my pull request is perfect.

But it's also like, man, you guys are going to wear me down to the point where I'm just going to abandon it. Right. Like, I'm just not going to deal with it. And that's, you know know i hope it doesn't come to that but you know whatever yeah i mean if the reviewers are are difficult to work with like they're overly critical then you're just de-incentivized to bother right you ever see a bug in uh github and you like start reading through the comments and you see that someone's fixed it and there's a pull request and you click the link and it's never been merged.

There's like six comments going back and forth and it's never merged. You're like, oh, no. So frustrating, too. That's more often than not, too, especially on really big projects. I know that the three of us have been working with Kafka and Elasticsearch. I can promise you there's been so many bugs that you go out there, you search, it comes up in a PR, and it's just been sitting there for months. And it's like, really? Like somebody did the work.

I think that's the one thing I would say to anybody that manages an open source project. project, yo, if you expect people or want people to help improve your product, you better be willing to jump on and look at those things. Prioritize it, right? Yeah. I mean, there was some project recent, I don't remember which one it was now, where like what you were saying, there was a pull request and this one had been sitting out there for like a year that had gone unmerged. And it was like, well, if you want it to work, you just got to check out that particular branch.

That's ridiculous. Because then it's a pain over time to maintain your local version of that thing, right? Because you need the latest changes they've done, but you also need the side PR that was never done. It's ridiculous, right? Yeah. Not a fan. Go ahead. It's still the duty of the reviewer to make sure that

you're recording and you don't want to record the product and create it over time. Move them right along. It's still the duty of the reviewer. So it's just speaking to the responsibility of the reviewer. Obviously, no one wants the health of the product to degrade over time. I also like getting my code review because then if there's some sort

of problem with it and anyone wants to point the finger at me i can be like well i approved it so i mean he sucks too so you like being able to have a finger to point at somebody yeah i'm taking you down with me that's awesome that's not how get blame works, sir. Yeah, I feel like, who wrote this crap? Well, you approved it. And they do mention here, too, again, the reviewer still has the ownership and the responsibility to make sure that they're ensuring a high-quality standard, right? And it's a tradeoff, right? It's a balance. You can't expect your pull request to go in perfectly every single time. On the flip side, the reviewer should balance out even if the changes aren't perfect. Because they go on to say there's no such thing as perfect code.

There's better code, but there's no such thing as perfect. So when you're reviewing a pull request, don't hold it to some unachievable standard and say, well, I'm not going to let you merge it in until it's perfect. Yeah, I completely agree with that. Yeah. They also say here reviewers can actually reject a set of changes even if it's quality code, but they feel it doesn't belong in our system.

This goes back to the functionality thing that we all said was a high for us earlier. Like, why would code be – now, maybe somebody wrote some code thinking that it should belong over here, but really it should have been in some other layer. Right. So I totally get that. So maybe that's what it is, right? Like maybe you did something in, in the data layer that really should have been in some sort of reporting layer or something like that. Right? Like I can get that and I could see totally rejecting it being like, yo, this, this is not something that you do here, right? You need to take the same code and port it over here. And I ran into a problem I had with a JavaScript package a while ago where I was trying to do something with it. And it turned out that the code I was using had like a built-in minification step, which is just kind of weird. And I was bringing in this package for something else, and I already had a minification step. So I was doing this extra work, and there was a problem with the way they minified. So I sent out a pull request and tried to add a flag to disable.

Like, this package really shouldn't be doing minification 2. You know, like, it's just not appropriate here. And I never heard back on it. I don't know what happened there but um yeah it was just uh it was a weird decision where maybe somebody needed to do that and rather than using two packages they tried to kind of shoehorn it into someone else's project it was just a weird weird problem and it had downstream side side effects that weren't good and then later if people are using other tools for minification or whatever it is uh other tools that are better and you know whatever and if you can't separate that stuff out of your program

then you've you're going to get replaced right uh going back to the to the quote that outlaw liked a second ago that one of the other things that they mentioned is reviewers should not seek perfection they should just be seeking constant improvement. Is the product being improved? Is the code quality either improving or, you know, maintaining status quo or whatever. Here's where there was another idea that they had in here that I really liked though. So they say that, you know, this doesn't mean that as a reviewer, you have to remain silent. You can still point things out. But if something is just like, maybe not a showstopper, but you're just nitpicking on something like,

hey, this is something where you could learn and improve upon better, then they suggested prefixing the comment with the word nit, N-I-T, so that the developer of the change list could see like, oh, this is one where I don't necessarily have to do this change in order to adjust the pull request so that they like it enough to approve it and merge it in. Right. But it's still something that they could like know that like, OK, hey, you know, when I if I get around to it, like this is something I can learn from. I like it that they have a standard for what they prefix it with. However, I hate the word nit.

Because it feels like when I hear nitpick, it automatically sends off almost like overly... I don't know the way to put it. It's almost like... It's exactly what nitpicking is, right? It's picking at little things that shouldn't matter that much. And I, and I feel like typically if it's something worth putting a note on a pull request, it shouldn't be a nitpick. It should be, Hey, yo side note, you know, what you did here is fine, but you know, maybe consider doing this because it would overall improve things and it's more in line with what we're doing. Right.

So I don't know, Yeah, but you need something. You need some kind of identifier there so that the other developer knows that like, hey, this comment isn't critical to your pull request getting merged in. Right. Yeah. I think I always put, hey, this isn't a blocker, but blah, blah, blah, which is obviously a lot longer than the word knit. But in typical Google fashion, though, they needed to compress that. So they compressed it into three letters, which is knit.

Right. I actually love that they didn't even call it knit pick. They just used knit. It's just knit. Right, yeah, knit. And if you think about the size of their repositories, that might matter. That's absolutely true. Yep.

The next one that's, this one's sort of funny to me because we've all been in these situations, but they basically say any code that worsens the quality of the product shouldn't be allowed in unless it's an emergency situation. And that's – go ahead. Well, I'm going to take us down a little tangent here because then it was like, okay, what constitutes an emergency? Right. And they actually – you've got to love Google.

They actually document what constitutes an emergency and what doesn't. Nice. And so quickly they say that it's a small change that either one of these situations is true. And again, small change I think is important here. It allows a major launch to continue or it fixes a significant production bug impacting users or it addresses a legal issue or it patches a security hole. Those are the four things that in Google's mindset constitutes an emergency.

Now, the things that they say do not constitute an emergency, I love these because these are awesome. You just want your change in sooner rather than later. Or you've worked hard on the feature for a long time and you just want to be done with it. Or the reviewers are either away or they're in another time zone. Or here's our favorite, because it's Friday and you want the code merged in before the weekend. Yeah.

Or your boss says that it has to be merged in today because of a soft deadline, which they actually go and define what are soft versus hard deadlines. Or lastly, what doesn't constitute an emergency is that rolling back causes test failures or breaks the bill so the boss one is really interesting to me because half the time emergencies not defined here are emergencies because the powers that be have told you we have to have this in today yeah i found that one super interesting. And so I'm like, wow, that's a – How many times have we been in a place where our boss has been like, that's got to get in today.

And you're like, I don't know, man. Google says your definition of emergency doesn't count. You're wrong. I mean, we've all at some point during our career had a situation where that's come up. So, you know, yeah. And now I just want to be like, I can't wait for the next time for it to come up. Be like, Google says. I like it that it's this well-defined because it does make it to where at least a developer can have the confidence to be like, I should not do this.

And you can point to it because it's a policy sort of, I guess it's a policy by Google, right? At least if you work for that company, you'd be like, no, we can't do this and this is why. Now, here's the thing too, though, like in fairness, the one about your boss is the way they actually worded it was like, they were just saying a manager says this. And I was kind of like interpreting that as like,

okay, but you know, if it's, does that mean like a manager above you? So then technically they're your boss at some level, right? Uh, if they're higher up the tree than you,

like, so I don't know if they mean like a boss, a manager who's maybe a peer, like, yeah, he's technically a manager, but he's a manager of other people and just, you know,

happens to be one of your peers or he's some manager. I don't know. Yeah. That one's harder to say, but, but still, I think that's a nice defined set of rules as to what constitutes calling something an emergency and allow it's a nice defined set of rules as to what constitutes calling something an emergency and allowing some bad code to go in.

Or some maybe not even bad code, just not up to snuff code, right? Yeah, code that doesn't necessarily live up to the quality of the rest of it. There's four situations that they allow it to go through. And they make sense, too, right? They do. And they make sense too, right? Like, I mean, if you had some major legal issue where it's like, hey, you can't use this library because of X, Y, and Z. And you're like, okay, well, we just got to like cut it out real quick. You know, you just got to, it might be gross how you do it, but you got to get it done because, you know, you don't want some GPL license affecting your code, for example. Right.

So the next one that we had up was mentoring, which I thought this was good. Code reviews can be a great tool when you're actually mentoring people. I know I've used it this way. I'm sure you guys have too. Like if you have an intern or you have somebody that's newer to the organization, it's absolutely a good learning process, right?

Like, hey, you have a ticket you're working on, you guys put it on the thing, and then, you know, it's easy to go in and look at that and be like, hey, I like what you did here, but be aware that this is actually something that affects these other two things over here that you may not have known about, right? By the way, mentoring, though, it doesn't have to be like,

you know, someone who's, it doesn't have to be like someone who's – New, right? Yeah, it doesn't have to be like the Jedi Master and the Padawan. It could just be like I'm new to this area of the code or this technology or this stack, and somebody else who's spent some more time in it could help you along the way. You guys, there's only one pull request that I absolutely remember from, from all my years doing stuff. You guys remember when Vlad joined us at Amazon, we had him on long time ago talking about a spectacular, right? But he looked at,

he looked at a pull request. He's like, these variables are being boxed. I never heard anybody ever referred to the technical term of basically taking a type variable and throwing it into an object before. I never heard anybody use that term. And we had just done that episode not too long in the past. Yeah, we all

looked up over the wall of our cubicle and were like, what did you just say? Right? Okay. Only pull request I ever remember. A lot of you out there, we miss you. Yeah, we do. He's actually working for AWS now.

He's one of their architects, I think. Oh, nice. Congrats. Yep. Cool. All right. So, the principles... Am I going or are you going? You guys can't go silent right after you both say it. Why don't you got to say something?

You got to write a random back off period and then try again. Oh, okay. That's how it works. All right. I'm going to go. Oh. All right. Technical facts and data overruled opinions and preferences.

All right, Alan, take it. So, principles. Technical facts and data overrule opinions or preferences. So, that's pretty sound. Good advice there. The style guide is the authority. If it's not in style guide, it should be based on a previous coding style. Otherwise, it's personal preference. Assuming that you have in style guide it should be based on a previous coding style otherwise it's personal

preference assuming that you have a style guide i i would not be surprised if most places don't have style guides i'll probably create one's a good idea well no but i mean this is covering either if you don't though right because like they say if you don't have have a coding style then it would if there isn't a style guide then it should go on the previous coding style. So in the example that Joe gave earlier of the me versus self, if everything else was going with the me variable, then, and you come along and you're like self, self, self, then it should technically be commented in the pull request to be like, no, no, no, this should really change. Right. Yeah. I think having a style guide though is a nice thing, right? Being able to point

developer, especially when you get to teams that are bigger than just a few people, right? And then that way everybody gets on the same page. So you're not going through the pull request telling people to do things, right? You can basically tell them before they ever do some of this stuff, hey, here's a style guide. Go look at it. You know what's nice is that if you don't have a style guide, you can just go find somebody else's. Every language has one. You can just go find it and if there's multiple competitors or people

disagree on which one's the best, just have a vote and pick one and just move on. Yeah, but I don't know about you guys, but I've definitely been in situations where I've had team members where there is a style guide that we have agreed on that is like, say, you know, let's say, for example, Microsoft's like, hey, here's the style guide for this particular language or whatever. And everybody agrees like, yep, we're going to follow the Microsoft style guide for that. Right.

And then you have team members that don't follow it. And you're like, hold up. We have a style. We've agreed on this style. Please follow this style. And then repeatedly the developer is like, eh. Every time you comment on it, you're like, okay. At what point am I doing my part as a reviewer to let you know that we've agreed on this standard and you're choosing to ignore me every time I say it?

And that only happens if it ultimately gets approved by somebody else. Right. And this goes back to what you were talking about with the problem with some of the tooling where it's like, Hey, if anybody hits approved, then it's in right. If, if it was truly blocked,

like, yo, no, you will fix this, go to the style guide. Then I guarantee that kind of stuff stops. So it's almost like a cultural type thing that you have to enforce, which sometimes can be harder.

Yeah. Well, let's go find the one other person that agrees with me and then I get them to approve all my pull requests. And that happens in a team based, you know, environment. Yeah,

that definitely happens. Yup. Yup. There, there are people that I've worked with in my career that, you know, I never saw their, their pull requests cause they would never bring it to me.

Because you're very thorough and they don't want that, right? Yeah. I don't know that that's why, but I just, I wouldn't get it. Yeah, that's basically it. The next one is the reviewer may just request that the code follow the existing code based pattern if there's no style guide, which is kind of

what we just said. So, you know, it's all fine. This next one's fun. Yeah, they have a whole section on like resolving conflicts. And at first I was like, oh, they're, you know, I'm here's stupid me and my stupid get mind. And I'm like, oh, merge conflicts. I know all about this. That is not the type of conflict they're talking about, sir.

No, they're talking about fistfight conflicts. Yes. Yes. So, yeah, if there are conflicts between the coder and the reviewer, which this will happen, if you do this long enough, it will happen. They should first attempt to come to a consensus based on the information discussed here in this guide that we're discussing, as well as what's in the change list author's guide or the reviewer's guide. Now, we haven't gone into all those. That'll probably be the next episode where we dive into some more of this stuff. So basically, look at what's allowed, look at what you're supposed to do and try and come to agreement on that, right? Take the emotion out of it and use the facts that are available. If the conflict remains, it's probably worth having a face-to-face to discuss the issues

and then make sure notes are taken and put on the code review for future reference for other readers of that pull request, right? So, hey, Joe, Zach, I don't like your code changes. You and I are going to talk about it. We're going to make some notes. We're going to put it on the PR and hopefully that resolves it. And if it does, we'll hit approve with the comments on there and everything's good. Right. If however, you still have it come to a consensus, then it's time to escalate. And, and what they say here is actually what I've seen happen in the past is you'll basically escalate it to a larger group, right? At that point, you're not trying to be a one-on-one type thing. You're going to get the opinion of the team and potentially a team lead or somebody that's a principal or lead in that group to sort of weigh in on the topic. And then that way, it's not me versus Joe Zach.

It's, hey, what is the entire group as a whole thing should happen here? A different kind of mediator pattern. It really is. Honestly, and you know, what's funny, like, and I know that you guys have seen this because we've all been through like the Amazon interview loops and that kind of stuff.

These are the type of questions that come up in the HRE type of things, right? Like, Hey, when you have a disagreement with somebody, how do you handle that? Right. And,

and honestly, this is the path that you're supposed to take in general terms, right? Talk to the person. If you can't resolve it, then raise it up to the larger group a whole. And then that way it's not a me versus them. It's a, you know, what does everybody agree upon here? And it takes the bite out of it or it should.

Yeah, I'm going to need some more specifics on that. What was the date that it happened? What did your manager say? How did you feel about it? Star. Welcome. And by the way that it happened? What did your manager say? How did you feel about it? Star. Welcome. And by the way, it is funny. I don't know about you guys.

Developers tend to be very tied emotionally to the code that they write. And so it can very much be that if you reject a pull request that somebody's put a lot of blood, sweat, and tears into, that emotions can run high and this stuff can happen, right? And it may not be the intent of the reviewer to take somebody off. And it's also probably not the intent of the coder to have ticked somebody else off. But when you've spent hours and hours and hours doing something that maybe you weren't even that fond of doing in the first place, it's really easy for people to get electrically charged on these things. I mean, it makes sense though. I mean, like this is,

this was like you thinking like how to solve a problem and then somebody else comes along and be like, no, I'm not good enough. Right. I mean, depending on how they approach that response, it can definitely set you off. You're like, wait, wait a minute. Yeah. Yeah. I'm not good at managing other people's feelings. Like, I can't even manage my own, you know? Don't put that on me.

Like, we got a conflict. It's time for paper, rock, scissors. And you said it wrong. It's rock, paper, scissors, man. How do you say paper, rock, scissors paper scissors man how you say paper rock scissors what game are you playing i've never heard this what hey we're gonna google this that wait paper i bet it don't even come up with anything rocks rock paper scissors wikipedia no it's a different game, man. You're hurting my head, dude.

Put in a pull request. I'm going to deny it. I thought it was just Rochambeau, though. Maybe like... I'm going to Rochambeau you for it. Yeah. Yeah. Which I don't understand how that's a thing either, by the way.

Isn't that the French version of it? Is that what it is? I would think so. I don't know how to spell any of it, so I couldn't tell you. But at any rate. All right. So the last little bit we have here, and this is super important, especially if you have a fast-changing code base. Don't ever just let a pull request or a change thing sit around because the coder and the reviewer can't come to an agreement.

Because all that does is hurt the product as a whole, right? I mean, we've all seen it happen and pull requests sit out there for a week or two. And by the time that you want to actually hit commit, so much stuff has changed that people have to go back in and modify the code to make it work again. So it's a waste of time, right? So you need to try and come to agreements quick and escalate things quickly if they need to be, but don't just hold things up. Yeah. Just merge it. That's the real lesson here. Just merge it.

And that is why I said my pull request to Joe. That's right. I don't like it, but one, two, three, four, let's be able to thumb war. All right. Well, that's right i don't like it but one two three four let's be able to thun more all right well uh we will have uh plenty of links to this uh set of documentation in the resources we like uh maybe even a link to uh joe's chip review which yeah i mean you never thought you needed to review a bag of chips until... I know, but you saw how big it was. You see Joe's review. You think I'm going to sit on that one? That was a miracle.

All right. So with that, we head into Alan's favorite portion of the show. It's the tip of the week. I totally forgot we still had this section. How about that? What? All right. All right. All right.

Well, it looks like I'm up first. So just today, my buddy over at Yala Coding YouTube channel, which you should follow, streamer, awesome. He sent me, you know, I've been doing streaming challenges online every day at LeetCode. Somebody went through and did a speed run on them. And I believe it was his first time uh doing them and he solved all 30 of them in one hour wow and uh this guy is a programming uh like a competitive programmer uh he might even be in high school maybe a prodigy i don't know but uh he uh he competes he uh does himself on youtube doing like coding challenges for basically like

uh google contest facebook contest all sorts of contests, Facebook contests, all sorts of contests and he wins quite frequently and we'll put the videos up online of him doing these kind of coding challenges and he also has quite a few tips on like how to kind of program at that level which is like a whole other kind of set of skills. He just

blasts through hard problems so you can see him like in the speedrun videos of watching like he kind of scrolls through and is like, okay, XOR, blah. And then he spits out the code, moves on to the next problem. He's like, okay, this is – he kind of classifies the problem, knows how to get there fast, and goes there. So obviously he's got a lot of time in there and just really good at it. So it was just cool to see someone solving problems at that level and just to kind of see how he approaches the problems. Dude, I'm going to watch that.

Yeah, it's cool. That's awesome. When you guys were in college, did you ever do any of the ACM competitions? No. I found out about it right before I quit. And I took one class and I was like, oh, this is really cool, Program Challenges. And now I still like Program Challenges. Wait, no, I mean like, did you

actually compete in the competition? Not as a class. No, I didn't know that they had it, but then when I mean, like, did you actually compete in the competition? Not as a class. No, I didn't know that they had it. But then when I took the class, it was like the only class that was scheduled, like fitting up my schedule was a class called problem solving. It had never been offered by the school before. And I was like, well, whatever. I need an elective. So I took it.

And the guy's like, hey, yeah, I'm the programming team coach here. And I was like, we have a programming team? We have competitions? Anyway, so it was like this class is kind of I'm the programming team coach here. And I was like, we have a programming team? We have competitions? Anyway, so I was like, this class is kind of based around just writing code quickly. And that was my favorite class. And it just turned out. Oh, wow. That's cool.

I wish we had that. So I did the ACM competitions twice while I was in school. And my team did horrible in it. But it was more like, you know, we would be in like a C++ class and the teacher would be like, oh, hey, by the way, there's this ACM competition coming up in like next week. And we would like to be able to send a team to it. Is anybody interested in going? And it was like really afterthought. It wasn't like a class devoted to it like what you're describing. So, yeah, I mean, that would have been interesting to see like how

i might have done had there been a class for it but yeah that's all yeah it was really cool and yeah there was um at the time i've been working i worked all the way through college uh and uh i uh you know i was like oh i'm already i've been coding for years like this class is gonna be no problem and then there was one guy on there who was on the programming team and he just kind of did this stuff for fun and man he was just like every problem we had we had like three hour lab he's just like and then just hanging out and do different things and have fun with i was like crap mom maybe i guess i don't know how to do this as well as i thought i did yeah it was really interesting because like uh you know like i said like we were at in a c++ class at the time and both both times that i, it was the C++ team that went.

And so we would go to these competitions and we were coming at it from trying to solve things in C and C++. After school, I later worked with a colleague who turned out – it turned out coincidentally that he and I competed against each other. And each year he was on the winning team when I went and they used Java. And so there was just a whole different mindset of the way they attacked the problems. And like, even as coworkers, like I would look at his code and like his way of approaching something was just like radically different. Like I'd be like, Hmm, now this is going to pad, this is going to fail my pull request, you know, or, uh, you know, code review of your code. Cause like we just had such

different styles about it, you know? Um, yeah. So it was interesting. That's cool. You know, what's really crazy is that kid probably, or I don't know if it's kid or not, but that guy probably makes more money than everybody else that we know, winning competitions, doing these things. The competitive programming guy? Yeah. I think he's still in high school, maybe. That's awesome. It doesn't mean that he doesn't earn more than us.

There are kids on YouTube that are like eight years old making more money. It reminds me of the episode that Outlaw turned me on to of the Darkneck Diaries, where people would go and do the competitions at, what's the security conference? Oh, Black Hat? Black Hat, yeah. They always have competitions, and it was just crazy. People stayed up 48 hours working on these things. Maybe we'll find a link and put that in there, because that was a really fun episode to listen to. Yeah. All right, Outlaw,. Yeah. So anyway,

all right, outlaw your turn. So I have two for you. And I wanted to say that this first one I found, this was mentioned in our Slack channel. Probably there's probably a good chance that this was micro G that said it first, but I saw angry Zoot talking about it. So because of what our current new normal is,

Microsoft Build event is usually a big deal to get to go to that, right? It's usually a costly endeavor and it's a pretty big deal if you get to go, but this year nobody gets to go. Uh, and so they're doing everything digitally online and free. So I'll have a link in the show notes, but the, um, if you're following along register.build.microsoft.com May 19th through the 20th, uh, for the Microsoft build and following suit and Apple with the WWwdc conference june 22nd will also be free this year so i'll have a link to that one and that one is unfortunately a much longer uh url so i'm not gonna bother to read that one a little bit a little bit yeah that's really cool all right so my turn since i was clicking through to those websites.

So in the vein of this show and in vain of the pull request that I put in that I've now had a very lengthy back and forth on, this whole thing that we talked about or that Outlaw mentioned at the beginning with styles is he doesn't care that much about them because you can automate them. So if you're doing Java development, which is basically what I am now, is a Java slash sometimes Kotlin slash sometimes any kind of developer, I'm doing Java, there's a thing called check style for Java that you can basically build into a pipeline, and it's what in the world? Yeah, it's a link to our Zoom, so just join in on our call right here, and we'll have this link in our show notes. Hold on a second.

Well, I'll find the right link. Yeah, that's all jacked up. But there's a link below it, so I don't know if that's the link you're looking for. No, that's a separate one. Or if this is like an Obi-Wan thing and this is not the link you're looking for. Man, that's so ridiculous. So this thing is called CheckStyle. I'll get the real link in there. I mean, it does link to a CheckStyle plugin for IntelliJ.

That's the second link. So that's what I was going to say here. I got the real link back in there now. So CheckStyle is on SourceForge. And then there is a plugin for JetBrains that I couldn't get to work. But assuming that you can get it to work, it will actually set it up so that if somebody has put in their project, the check style file that basically tells it what types of endpoints you're allowed to do, how much space you have to have between braces. It goes super

granular before you actually go try and commit and compile your code. It will actually throw errors for you in the compiler so that you can go back and fix that stuff before you go in and put it in pull request and do all that other kind of stuff. So this is, this is one of the things in the Java world that is the equivalent of what outlaw was talking about. And there's all kinds of things out there for different languages, but this is one that I found and it's a, you know, it's pretty neat. It seems to do a really good job of things. Like it can get as granular as the order of your imports in your Java. You have to put these certain types of imports up above these other ones and all that.

Pretty neat stuff. Yeah, it's nice to be able to have that in for a tool. When I was talking about being able to do this inside of a pull request, I was thinking of the one that came to mind specifically was the Python Black library for the code format, the uncompromising code formatter, as it's called. Interesting. Yeah, I know this one is

built into the Maven build that was used on the open source project, so it worked both locally and when you put in your merge request up on GitHub, it would also run the build up there and fail it if any of those check styles were failing. So, yeah, I mean, again, there's tons of these things out there,

but it's worth knowing about and possibly getting them in your pipeline because then instead of developers arguing about tabs versus spaces, you can let the tools tell you that you did it wrong. Yeah, that it was supposed to be spaces. And then. Right. Whatever. Tabs.

No. Spaces. Okay. All right. Well, so hope you enjoyed this episode. And in case that a friend happened to share a link to this episode or whatever, if you haven't already subscribed to us, we'd appreciate it if you would. You can find links at the top of our page for

iTunes, Spotify, Stitcher, all your favorite podcast apps. And if you have a favorite that's not, that we aren't aware of, that you couldn't find us on, let us know and we will make sure to rectify that issue. And as oh wait, did we do the beg before?

I don't know. I think I stepped away. No, Joe did. Yeah, when you guys were in the bathroom together. Yeah, we weren't here. You guys were in the bathroom. That was weird. That was awkward. So yeah, so you can find some...

If you haven't already left us a review, we would greatly appreciate it. You can find some helpful links at www.codingblocks.net slash review. My lawyers say I'm illegally allowed to still say that. And while you're up there, do definitely check out our incredible show notes, our discussions, and get involved. And Joe? Hey, follow Twitter at Twitter. CodingBlock's head up to the page that has the top at the feedback questions,

rants, and Slack. I don't know if he had a stroke, if I had a stroke. I'm not sure what happened. So I'm playing competitive Overwatch right now. Oh, all right. I got like 10 seconds. What's up? What do you want?

No, no. Go back. Go back. Give me a review of something. I'll stamp it. No problem.