DevShop Stories #9 - The Story of Why We Do Code Reviews

Listen on Apple

Listen on Spotify

Listen on YouTube

‍

[00:00:00] Josh: welcome to another episode of Dev Shop Stories. My name is Josh, and I have Tanner here with me and Spencer. And today we're gonna share a story about code reviews and why they're important. So the story begins like this. We had a small dev shop and we were developing a mobile application. And of course in this mobile application when certain things happened, we wanted to have text messages being sent out.

And so this developer was working with a software solution called Twilio that can actually send text messages out. And what he did was he did some code changes and some modifications, and he just pushed it right into production code. And when he was testing it in, in the. Production environment, which is obviously, there's a couple things wrong with that sentence.

I say right there, he was testing in production and he didn't do code reviews at all. What happened was he introduced a bug that created an infinite loop, and he wanted to test it with his own cell phone. And so what happened was, is he received. Text messages out the wazoo. Just, yeah, massive amounts. You know, he, in, in one minute, he received over 13,000 text messages in a minute.

So much so I wasn't there. Tanner was there, but so much so that his phone just basically, Bricked. I mean, it just like completely froze up. Couldn't interact with it at all.

[00:01:19] Tanner: Yeah, he couldn't do anything. It totally locked up.

[00:01:21] Josh: Yep. And, and so just that, but here's the thing on as a business owner's side of things, each one of those text messages cost a penny.

And so within one minute, he had spent $1,300 in text messages because of non coder review testing and production and, and bugs that were introduced. So, you know, , that's a story to begin with the coder reviews here. So

[00:01:44] Tanner: yeah, lack of them definitely can cost real money.

[00:01:47] Josh: Yeah. And the sad thing was, is he, he never told me, I only saw it on a, you know, invoice.

I was looking at bank accounts. I'm like, why, why is Twilio charging me, like, you know, 1300 bucks or whatever. And I went and talked to Tanner here, and he is like, oh yeah. And

[00:02:01] Tanner: like, yeah, this bug came up. I thought you were aware of it. I don't know.

[00:02:05] Josh: Yep. Yeah, so,

[00:02:07] Spencer: I, I'm surprised there, there wasn't like spam. Filters in place that would prevent like his phone bricking or you getting charged?

[00:02:14] Tanner: Nope. But he had that much at the time. It was like the latest iPhone and it locked that bad Boy right up.

[00:02:19] Josh: Yep. That's great. So we're gonna spend kind of the rest of this episode talking about code reviews, what they are, what are the benefits, and, you know, tools that could help you with to code reviews.

And so code reviews, what are they, Spencer?

[00:02:33] Spencer: All what it comes down to is code reviews. It's, it's being peer reviewed of having your code checked over by another person. that's, that's really all it comes down to. And you, you can have various processes in place where it's, Do you just need one person to review to go over code.

Do you need two people? Does it have to be someone more superior to you? It's, you can come up with all sorts of rules, but that's really what comes down to it's peer reviews.

[00:02:54] Josh: Yeah. Tanner. What do you think?

[00:02:55] Tanner: Yeah. So yeah, I mean, for those that don't know, it's the, the way that it works when you do development is you have this repository that lives in the cloud.

you pull that repository down to your local machine and then make a branch off of it. So you kind of fork whatever this is, for lack of a better term, and work on your code. When you're done, you commit it into the repository and then try to merge it back into that main branch and that repository living in the cloud.

And in that, that merge process, you can put in some restrictions in one of those as the code review. And, like Spencer said, it's, it's getting a second set of eyes to go through and just sanity check. know, a whole bunch of it. It's just that peer review process to make sure that people aren't making large architectural changes or whatever.

Right.

[00:03:40] Josh: yeah. Yep.

[00:03:41] Spencer: Or, or if they are doing something where they, they can justify it. Yeah. You can have a conversation.

[00:03:45] Tanner: Yeah, exactly. It's like a stop gap to let you have that dialogue on those changes that are gonna take place. So it would've prevented, you know, theoretically prevented, you know, this $1,300 bill or whatever that, that we got hit with.

You know, assuming that the other developer, me in that case would've caught it.

[00:04:03] Josh: you would've caught it for sure.

[00:04:04] Tanner: Oh, I absolutely would've caught it. Definitely.

[00:04:06] Josh: So things that we're looking at in a, in a code review, let's talk about that for a second. So we're obviously looking for those bugs, you know, those infinite loops that could happen or you know, just in invalid stuff, but there's.

Other things that we're looking for. We're looking for consistency of coding. So if you've created a sort of a style guide at your company that your code has to be formatted in a certain way, you're looking for that. You're also looking as kind of educational moments too. I know I've commented on people and I said, Hey, you don't have to change this, but just so you know, You could do this a little bit better if you did it this way, you know?

And, and try not to be overbearing in it and say like, Nope, you have to do it my way, but here, here's one way an alternative. You could think of it. And then you're, like I said, you're looking for those coding standards to, the right capitalization, the right indentation, which if you use things like prettier, you know, you don't have to look at that as much.

Right?

[00:04:58] Spencer: Yeah, with code reviews, it's part of our development process.

We have clean coding where. As, as we code, we want our code to be read like a book so that you can jump in in the code. And there's certain things make sense as you read it. You know, you're, Josh kinda hit on like variable names. Do they make sense? and it helps maintain that code hygiene for lack of better words, where it's just.

[00:05:16] Tanner: Yeah, it's a, it's a good opportunity to, to target and identify some things that otherwise could be technical debt, right? Yeah. And clean up some of those items. It's like, Hey, does, does this method make sense? Are you putting in guards on your code so you fail early and it's more readable? logically, does this make sense?

Can I look at this function without having any greater context of what this project is and know what this is doing, or you know, is it, is it ambiguous? Is there something else?

[00:05:42] Josh: And one, one valid check that I think that that happens is the person that's code reviewing is not as intimately familiar as you are with that section of code that you obviously wrote.

Right. And so, does that make sense to the code reviewer? Was he able to read it and parse it and understand it? If not, you know, should things be named differently? Should you extract some of the code into. Helper functions that, that helps describe and, and self document it because we, we are not big on adding comments just to help describe the code.

Only comments where you're, you're kind of deviating from the path of Norman. You're explaining why you deviated kind of thing. Right?

[00:06:17] Spencer: Yeah. And, kind of a point I wanted to bring up too with, code reviews. It's a great time too to find these kind of issues cuz longer, poorly organized code existing code.

Architecture kind of. Oh, yeah, yeah. The, the longer bad architecture exists in code. The longer bugs exist in code, the more expensive those problems become to either fix or change later. And it could really start to slow down progress on a, on a project as it gets later. And now you've got these. Huge architectural problems that could have been caught earlier on, but

[00:06:45] Josh: Right. And so some of the other benefits of it is it kind of keeps everybody aware of the changes that are going on. So you're gonna make some change and you put it out to the group of the people that are working in that, in that software, that subsystem and keeps them aware of what's going on. I remember specifically, we were starting a new project and Spencer kept hounding me on like, Hey, You know, you don't have any code reviews, see your code review.

I haven't done it yet, you know, kind of thing. And I, I get Now what he's saying I was like, no, I'm not done yet. I don't want to submit it. I don't wanna have a code review yet. But, but he wanted to be aware of, of kind of my thought, train my process so that if, there was maybe some improvements we could do on it, he could actually comment back on that. Right?

[00:07:25] Spencer: Yeah. it's not just about finding bugs and fixing, you know, your code or getting somebody smarter to look at it. It's a training tool too, to get other people understanding. what your thought was. So,

[00:07:34] Tanner: yeah. And it is nice though. I mean, if you do, if you build something like it's hard to stay consistent on that when you start off a project cause it changes so quickly.

building on what you know, theoretically could be a rocky foundation. Those bugs and those issues propagate very quickly. And kind of what you had pointed out, Spencer, they're pain in the butt to pull out

[00:07:53] Josh: so some people might say, well, in coder reviews, that's just gonna take more time. It's gonna cost money cuz you have developers doing coder reviews. What do you, what do you have to say to that, Tanner?

[00:08:02] Tanner: Yeah, people, I mean, that's always gonna be the initial argument. It's like, oh man, well I need to spend my time coding. It's like, well, A, it's a learning tool, right? It doesn't matter who it is.

You're always gonna learn from somebody. it helps with the quality, the cleanliness absolutely necessary. There's not an excuse not to do them.

[00:08:18] Spencer: Yeah. I've worked on teams or companies where code reviewers aren't enforced or aren't valued, and trying to introduce that, it's, you get a lot of kickback from project managers, developers, maybe even senior developers who've never done them. Yeah.

[00:08:31] Josh: now in your specific case, you told me a story of how coder reviews only went one way. Explain that.

[00:08:37] Spencer: Yeah. I was on one team where it was basically, you know, somebody who had ownership of the project. Very, tight controls. It was so any code reviews just went one way and then, so then it was never a learning opportunity for me of, okay, now you're changing, code too, and I should have some insight of what's changing or offer some maybe my own suggestions too, and not just six months later, like, when did this get written?

[00:08:57] Josh: So basically the, the person over that would code review your code, tell you what you did wrong, but when he did code changes, you never got to see it.

No. Those, you never got straight in chance. Yeah, they just went straight in because, yeah.

[00:09:08] Tanner: Well, and then you get that it, the nice thing too is that it, it enforces. That checks and balances where you don't test in production like we've talked about previously. You know, like people do. We have a, couple projects that we've worked on and, there isn't anything Right.

We enforce it on our side, and if you can't enforce it on the repository, well then, like you're, you're still leaving that, that open. Gaping issue.

[00:09:31] Spencer: Yeah. You definitely have to take advantage of the tools that enforce it cuz it's very, even I have moments where I, it's like I just need this to get in. I don't, I don't wanna get somebody else to look at this right now. the tools that enforce it, then you have to.

[00:09:41] Josh: Yep. So speaking of tools with. GitLab and GitHub, just for those listening, you have kind of different ways of doing code reviews or, or initiating them. Right. With GitLab, they call it merge requests, and so you're requesting a merge code into it, and GitHub, they call it poll requests because usually Their methodologies, you kind of fork from a repo that's open source and you're asking the, the maintainer to pull your code into theirs. And so same thing, but it , it initiates a code review. And the, the tools that are built into these web platforms are amazing. They, they show the, the code directly right next to each other.

They show what's. What has changed, what was added, what was deleted, and then they allow a very simple UI to be able to click on a line and write a message and say, Hey, what are you doing here? Right?

[00:10:27] Tanner: Yep. Yeah. It, it makes the dialogue very, very targeted and explicit. it's really easy to see. You have all of the file changes.

You get, you know what, change it. It really gives you a lot of context on that thing. Now, one thing I think as well, Really beneficial is you can actually put some restrictions in there on both tools, where it has to be approved by certain people explicitly, or certain quantity of people or whatever you want to do.

And then it's per branch too, so, You can have these different rules that take place, depending on what environment you're working in.

[00:10:58] Josh: Yep, yep. GitLab for example, has kind of different classification of users. They have reporters and those are people that can just create issues and tickets and stuff.

They have developers and those are the people obviously that can code and they can do stuff. But you can put Lockdowns on. The next level up is maintainers. Maintainers are the ones that I can actually. Influence protected branches. So you can mark whatever branches you want. Usually it's like a master main and a dev branch, and you can say, Hey, only the maintainers are allowed to accept merger requests into those branches.

Right? And what I like about GitLab is that it shows you can create all these comments and you can submit 'em all as one, coder view. And just kind of have your overarching message on there. You submit that the developer, they just get one email saying, Hey, so-and-so is just code review. Go and clear it up.

And then the, developer will either go and address every single comment either with a code fix or if they, they believe strongly, or if it's just a, a general question, they can just comment right back, right there on GitLab and be able to kind of counter or, you know, approve, you know?

[00:12:06] Tanner: Yeah. But it forces that resolution and it forces that conversation about whatever ambiguity or question or whatever came up.

[00:12:13] Josh: And we, we say that, all of those comments have to be resolved before we allow the code to actually get merged in. And the resolving, this is one problem that we have with some people that come in, is the person that's going through it, they think that. Okay, I fixed it. I'm just gonna click the button and say This is resolved.

I think it should be the person that originally created the comment. That is the one that that actually says it's resolved. Cuz maybe that code change that they did that to fix it. Still wasn't Right. Yeah.

[00:12:40] Tanner: And I've had several of those where we've gone back and forth cuz it's like, Hey, what you did isn't satisfactory. Like it's an improvement, but it's still not good enough.

[00:12:49] Josh: Yep. Yep. And so, other tools that I've seen people use as Code Stream yeah,

[00:12:53] Tanner: we tried that one for a little bit.

[00:12:55] Josh: Yep. And that's a plugin that you can put into VS code or into PHP Storm and it kind of allows you to, to share snippets of code and do all this kind of stuff.

Eventually I removed the plugin. I just felt it was, Very taxing on the system. It just really didn't provide the benefit there. We didn't get a, a full buy-in from all the users, so not everybody was using it. And so just overall just it, it wasn't a tool that we used.

[00:13:18] Tanner: Yeah. Now there were some benefits to it, right?

Like it keeps you in your IDE and not takes you out to the, to the cloud platform and stuff, but.

[00:13:26] Spencer: Well, and you could share without, you could share code snippets without it actually being a formal code review too. Yeah. But, but yeah, like I said, it was kind of a cumbersome tool too.

[00:13:34] Tanner: Yep. And, and if you can't get the whole team on it, there's not there's not a point to use it.

And then, yeah, it happened to be a little heavy.

[00:13:40] Spencer: And it wasn't enforcing it at the repository level, which is where, where you really need it is.

[00:13:44] Josh: So let's go a little bit deeper into kind of what is looked at in a coder review. So the first thing I can think of is it allows you to read and understand without a lot of context.

What it's trying to do is, is kind of what, what I'm looking for. So that change there, I should be able to kind of just look at the little snippet and not have to expand the entire file just to understand what's going on. And is it you? Basically you do that through clean coding techniques.

[00:14:09] Tanner: Yep. Yeah. And for us for me, the, the very first thing that I, I want to look at is we use type script.

So I check types. Are you using your types? I don't care about what it's doing. If you're not using your types, I'm kicking it back. I don't care.

[00:14:22] Spencer: And if it's an any, you're probably gonna,

[00:14:24] Tanner: no, I'm kicking a back. Kicking a back. Unless you have a very good reason and even, and then it's not valid. Use a generic , you know?

[00:14:30] Josh: Yeah. Yeah. Alex is. Developer that we have here. And he remembers a quote that I said to him, and I said, if you, if you write a, any in your code, the first thing you should do is slap yourself and then, then write yours, you know, any in your code. So, yeah. yeah. What, what else do you look for, Spencer?

[00:14:47] Spencer: Yeah, I like when I first get, look at a code review, I like to look at. What they're trying to do, there's a, when they make the merge request, they have a message of what they're trying to do or description. And before I even look at the code, I like to imagine like, okay, how would I do this if I was trying to solve the same issue?

And I'm trying to look for that. Okay, does this, how does this compare to my thoughts? Sometimes it's very close and sometimes it's wildly different. and if it's wildly different, then it's like, okay, is this better or does it need to be reworked? And so,

[00:15:14] Tanner: yeah. Now with that, I will say that what they are doing does need to be cohesive.

with the, the ecosystem, with the environment that they're working in. Yeah.  now there may be better ways to do it, right, but if it's totally outta left field and it doesn't follow the same kind of coding conventions that the regular code base is, I'm much more of a proponent that it follows the standard of the code versus something way outta left field.

[00:15:38] Spencer: Well, and like somebody doing a big refactor and also trying to fix bugs in the same merge request. It is so hard to read cuz you lose that cohesiveness. It's like, okay, what's really going on here? Or what are you actually changing?

[00:15:49] Tanner: Well, and that's a really good point is your merger views need to be small.

You should do one thing, right? Like what is this one ticket that you're working on? just do that and keep it small and concise. The worst thing you can do is have these huge. You know, thousand plus line changes.

[00:16:07] Spencer: Well then the description on those is fixed bugs.

[00:16:09] Tanner: Yeah, exactly. They suck to review.

[00:16:12] Josh: one, one thing that I personally look for is my, kind of my position in the company, I get to oversee a lot of different projects and I've worked on our core framework that we use in a lot of these projects.

And so I've usually have a better feel for things that we've done in the past or other groups have done. And so, What I'll do is I'll, I'll kind of look at some code and sometimes see, hey, this code that you did here where you're changing a date, timestamp to, you know, an hours, minute, seconds kind of thing.

We actually already have a utility for that. You should use this utilities class.

[00:16:42] Tanner: Yeah, yeah. That, that comes up quite a bit. a big thing is code duplication. You don't repeat yourself. super core concept. So looking for those items and calling them out even if it's, there's repetition, you know, not necessarily a, a function that already does this thing, but repetition inside of that same request.

identifying those and calling them out, Hey, you could pull this out into a function and simplify this a whole bunch and make it a lot more maintained.

[00:17:07] Spencer: I, I like it when they think they've come up with a great solution. They've, I've done this before. You know, write some great new code and then you get that code comment.

We already have this in the utilities package. You should have just called that. Mm-hmm. . Like, we've already solved this .

[00:17:20] Josh: Other things we're looking for is, you know, are there any speed concerns? you know, did they do something that's that's gonna take a hundred times longer than it should?

You know.

[00:17:28] Tanner: Oh yeah. Could you parallelize your synchronous request to, you know, whatever.

[00:17:33] Spencer: Or do you have nested loops where you're you've increased your, big O Big complexity. Big, yeah. unnecessarily. Cause sometimes people, sometimes people will loop over to a raise. Then array and you know, it's like you've just, you've just gotten an ncore complexity when you didn't have to, right?

[00:17:50] Tanner: Yep. Yeah. A lot of it too is just thinking through what they're doing. Right. Given the context of what you have, their commit message, the ticket. and their code, are they covering the edge cases that you can think about? cuz you know, it's always nice to have that, that sounding board somebody else to look at something and think of it a little bit differently.

Kinda like you pointed out before Spencer. and it's, are they covering these edge cases in a, you know, at all? And if so, does it actually cover the edge case?

[00:18:18] Josh: also one of the things that we have is kind of our clean coding convention, and we actually believe in clean coding. So really quick clean coding was a book written by Uncle Bob Martin, or just Bob Martin.

He likes to be called Uncle Bob, but he actually has some really good ideas and concepts in there of how people should code as far as, you know. Long function names and long variable names, and being able to explain what you're doing so you don't have to have tons of comments of what's going on. So one of the things I look for is we actually have put our developers through clean coding class.

We, we have a udemi course that we, we kind of basically force on the watch and so we can all speak kind of the same language and, and have the same ways of representing code. And so I look to see are they following those clean coding standards as one of my code review checks.

[00:19:06] Tanner: Yep. Yeah. And that encompasses everything, right?

From, is your function huge? Are you encompassing a whole lot of lines? Is your merge commit massive? If so, like, well, I'm sorry, you should go refactor that. Break it out more simple, readable, understandable things.

[00:19:22] Spencer: Yeah. and going back to error handling, clean code has system suggestions on how you handle error, how you handle errors, and do.

How you you do guards in your functions and you know, are, are they actually doing that? Are they. you know, just, yeah, just our conventions we've established that we like.

[00:19:35] Tanner: Yep. And for us it works great. Right? It's, it's, it's this the methodology that we prescribe too.

[00:19:42] Josh: So I just want to kind of go through a list that we've created of best practices, and I know we've covered these and so I'm just gonna read 'em out, add any comments to where we need to.

So best practices with coder reviews are keep them as short as possible. The, the actual code commits that you're doing. That way somebody doesn't get buried and it's a lot of work if it's, if it's too much. Right. Next one is have a good comment or definition of what you did. So,

[00:20:07] Tanner: yeah, it's frustrating when you get that, that merger quest, even if it's small and it's like, oh, fixed bugs.

Mm-hmm. like, cool. What bug did you fix? Yeah. What context changed? Right. Like, tell me what's going on and preferably relate it to the ticket.

[00:20:21] Josh: Right? The code review. The merge request should accomplish one goal and one goal only. we talked about how we have some auto unit testing and prettier running.

So prettier it has to pass the auto code formatting and ly have to pass. So that way when the code reviewer goes to it, they're not trying to say, oh, your curly brackets on the wrong line, or, or whatnot, or You have too many spaces between functions.

[00:20:43] Tanner: Well, it removes that opinion, right?

[00:20:44] Spencer: It can be automated, so you should, it's like why spend your brain cells thinking. White space and curly brackets

[00:20:49] Josh: and Yep. Yep. coder reviews should start right away, even at the initiation of a project so that you can get helpful feedback from coworkers on, on how you're doing stuff and how you're architecting it.

coder reviews should go bidirectional so that you have juniors can coder review seniors and seniors can code review juniors.

[00:21:08] Tanner: Yeah. Like I pointed out before, it's a, it's always a good learning experience you know, some junior might be doing some weird cool thing that they just learned about in school or whatever.

and then vice versa, they get the experience of seeing what the seniors do.

[00:21:21] Spencer: Well, speaking of junior devs, a lot of times they, they have the time to really dig into some library technology and they may actually. Sometimes, you know, I'll code review a junior and be like, I didn't know you could do that.

It could be a language teacher. I'm like, like, wow, that's a great idea.

[00:21:34] Josh: Yep. and then finally, you know, did the ci cd tests, did they pass or they break? You know, is there functionality that that just isn't there? You kind of try to catch that with coder reviews. Overall, I think coder reviews are a great educational process, and so we highly encourage them at Red Sky.

Uh, I would encourage anybody out there to, to not look at it as a, a cost sync, but as a, a improvement and overall. If you actually look at it, I'm sure there's been studies done as, I don't have any right on my hand, but I guarantee that, that the cost savings are there in the final product. When you look through all the QA and bugs that are caught and captured,

[00:22:12] Tanner: absolutely.

The, the technical debt associated and just everything after you're gonna pay for it either way. Right. You might as well do it up upfront.

[00:22:20] Josh: Well, thank you for listening to another story. We will be back next week with more stories, personal experiences, and advice on running a dev shop. Thank you.

[00:22:27] Tanner: Thanks guys.

[00:22:28] Spencer: Thank you.

‍