No Compromises

Some shy away from code review, not wanting to be critiqued. Aaron and Joel share some tips on how to get the most out of code reviews, and things to look for as a code reviewer.
  • 00:00 The value of a code review process
  • 02:53 Look at it as a learning opportunity
  • 04:06 Approaching a pull request to review it
  • 06:05 Should we make this better?
  • 07:00 Did the programmer have a reason for what they did?
  • 09:05 Everyone on the team should get their code reviewed
  • 10:25 Silly bit

Creators & Guests

Host
Aaron Saray
Host
Joel Clermont

What is No Compromises?

Two seasoned salty programming veterans talk best practices based on years of working with Laravel SaaS teams.

Joel Clermont (00:00):
Welcome to No Compromises, a peek into the mind of two old web devs who have seen some things. This is Joel.

Aaron Saray (00:08):
And this is Aaron.

Joel Clermont (00:16):
Nobody likes criticism, but when you're working on a team and you're all contributing code and building the product together it's inevitable that someone else is going to look at what you wrote and have an opinion on it. And many teams even have a formal process whereby one person always reviews another person's code. And we've been on teams like that, we've set up our own code review process. But, Aaron, I know you have opinions on this and I'd like to maybe get a high-level take from you on what you see as the value of code review and how to do so constructively.

Aaron Saray (00:53):
Yeah. I think that's the keyword, constructive criticism. I think there is value in conflict. Conflict doesn't always have to be such a bad word. When you have conflicting opinions and you can take a look at something and start to pull it apart, you're not doing it to bring a person down. You're doing it to kind of pull apart the code and look at it and understand, "What are we actually getting here after we've created something?" I think the important thing for me when I think about code review, is first of all, it's super important for a couple of reasons.
First, I want to make sure I can disseminate the information of this code with at least one other person, especially if I'm on a team. When you solve a problem with code, the best way to understand how that was solved is if someone else is also reviewing that code. So it's kind of a way of thinking of sharing the duties or spreading out the knowledge on a team, so it's a little bit of a knowledge share. But the second more important thing I kind of think about is, you're doing quality review, you're doing accuracy review and by doing some code review, you're looking at the end product and saying, "Is this up to my standards? Is this going to solve the problem?" et cetera.
It's like if you go and buy a new car or something like that, you want to take a look at it usually, and see if it looks nice, if the trim is nice, there's no scratches or anything on there. And then maybe you'll even get in and drive it around. Just like you might run some code from someone else's to see that it does in fact do what you think it does.

Joel Clermont (02:30):
Okay. Yeah, that's a fair way of looking at it. What I heard you say is a lot of times when you're doing a code review, it's sharing knowledge, yes, but it's also preventing defects and maybe enforcing some team standards that may have been missed in the heat of development. It could also be, putting myself in the shoes of a person submitting code to be reviewed, you could even look at it as a learning opportunity.

Aaron Saray (02:58):
Sure.

Joel Clermont (02:58):
If somebody comes back and says, "Hey, why didn't you do this?" Now, maybe I thought of what you suggested and I can tell you why and then we both learned something, but maybe I didn't think of it. So don't get defensive but, hey, you just learned something too. Easier said than done, but I have benefited from constructive code review in the past where it's like, "Oh, yeah, that is a better way of doing it."

Aaron Saray (03:19):
Right. Well, I think too, we need to be a little honest with ourselves and say, whenever you put your work up for review, there is going to be some sort of pride that you have in that work.

Joel Clermont (03:30):
For sure.

Aaron Saray (03:30):
When someone comes by and pokes on it and says, "This is good enough." I don't think it's wrong to have those little spikes of emotion and be like, "Hey, you're poking at my baby." But understand that as a team or as another person that you've contracted to look at the work, that that person just wants what's best and they're not there to attack you or your work. They're just looking to say, "How can we make this better?" And if you are really proud of your baby, you'd want it to be the best code baby you possibly could have.

Joel Clermont (04:03):
Fair enough, yeah. Now, okay, let's put our code reviewer hats on. We just got a pull request, it came in. Do you have a mental process you've kind of worked through in terms of...? Let's just say this is a larger than desirable pull request. It's like 30 files changed or something, how do you dissect that? How do you attack that as a code reviewer without blocking out a whole data to read every line of code?

Aaron Saray (04:29):
The first thing I do is I berate the other programmer for making a 30-file pull request.

Joel Clermont (04:34):
Very good, smart.

Aaron Saray (04:35):
I make sure that they understand that that's not okay. And now I just delete the pull request.

Joel Clermont (04:41):
I have a policy where I wait one day for every additional file that was included.

Aaron Saray (04:49):
No, I think obviously there's a limit on those sort of things. But sometimes life happens and you have to do what you have to do. I'm a huge fan of patterns and I think we all follow patterns in our daily lives. Patterns are something that I used when I review larger blocks of code, to look for areas I might want to poke at or look more in detail at. If I'm going to have 30-something, 40-something files, I'm going to start scrolling through those. I'm looking for patterns. I'm looking for patterns in the shape of how the code looks, I'm looking for patterns as in different words that may be used.
So I start to see if there's indenting problems or if there is spelling mistakes, or if there's random sort of spacing, or extra words, or short variables. Or all these different things that are just little cues for me, that the pattern doesn't look the same as everywhere else. When I can see that pattern is a little bit different, then I've kind of found an area where maybe I should spend a little bit more time to review.

Joel Clermont (05:54):
Yeah. Another one that I know we've talked about is those four magic letters, to-do. You see that in the code and you're like, hmm. To-do, but why didn't you? You're just like, "That's a red flag right there." Not saying it's never good, I write to-do tests sometimes. But it definitely is something that picks your eye when you're doing a code review. Let's say we're working in a less than pristine code base, it's seen some things, right? This code base has been around a while, and as a team we agree-

Aaron Saray (06:30):
It�s seen some things.

Joel Clermont (06:30):
It's seen some things. We agree things could be better. I'm working in a 400 line controller method and I add 20 more lines to it. If you're a code reviewer, would you push back on that? Or would you ask somebody like, "Hey, could we maybe clean this up a little bit?"

Aaron Saray (06:52):
I think that has to do with time available and just understanding it too. I might push back on it, but I'm more pushing back to understand why that was done, versus say that we shouldn't do that. What's super important to me as a coder viewer too is when I ask a question, that the answer had a reason. Now it could be a wrong decision, but the worst thing you can do as a programmer is have no reason for the thing you did which you did.

Joel Clermont (07:21):
I don't know.

Aaron Saray (07:22):
That's kind of, "Yeah, whoa." But I would ask a little bit on that. I'd be like, "Well, we're making this a little bit longer. Is that the best solution?" Then that's time for a conversation where you as a programmer can be like, "Yeah, I know, but we have slated to rewrite this method in a couple weeks, so it's fine." Or, "I don't like it either, but I couldn't think of another way. Do you have any feedback for me of how we could do that?" I think that's actually another good point, is don't assume that everyone knows the answer. Sometimes you can point out and say, "Why is this this way?" The person might be like, "Hey, I didn't know a better way. Do you know a better way?" With the wrong tone, that can sound like a challenge.

Joel Clermont (08:09):
Sure.

Aaron Saray (08:09):
Like, "Well, I don't know. How about you know?" But I think it's a legitimate question during the process if you ask it respectfully.

Joel Clermont (08:16):
Yeah, and just the light bulb went on for me when you're talking about as a developer, hopefully you're making a conscious decision. If you're making a decision, especially if it's one you're thinking a little hard about, that might even be an opportunity for a comment or a note in the pull request to head off that question. Like, I know this isn't great, but here's why I did it. And just put that right in the pull request. Or, if you're doing something a little non-standard but you have a valid reason, that probably belongs as a comment in the code so the next person doesn't just copy it and propagate this thing that doesn't belong in other places of the code base.
Code reviews are valuable, I think we've established that. They might be a little painful if not handled properly or if coming at it with the right attitude. Do you ever think it's valuable for maybe the team lead to have their code reviewed by somebody else on the team? Maybe even somebody that's been there less time or has less experience?

Aaron Saray (09:16):
Yeah, I think that's a great idea for a number of reasons. But one of the most important, comes back to that initial sort of reason. Is, especially if it's someone who hasn't been there for a long time, they'll probably get to learn something. They'll get to learn how this process and what's the level of code, because hopefully the team leader is setting the standards as well. It also is a thing where you kind of join the team.
You say, "No one's above code review. It has nothing to do with your stature as a programmer, it has everything to do about the product." While I might have a lead developer who makes better, or makes all the decisions and whatnot, it doesn't mean that they're better than any other person when it comes to actually coding and the responsibilities that we all have to review each other's work. The idea of code review is to make the product better, it doesn't matter who the author is of the code. It's to add that extra value.

Joel Clermont (10:10):
Yeah, I agree. I look forward to reviewing all your code in the future and being-

Aaron Saray (10:17):
It's perfect, so you won't have anything to review. I don't make many pull requests, but when I do their a hundred files large.
Joel, we have a lot of talented people in this world, or some are just lucky or whatever. Can you think of any weird things that people do, maybe amazing things that people do, that when they do them it works perfectly fine, but when you try it's just a dumpster fire? I'll give you an example.

Joel Clermont (10:51):
Okay, please.

Aaron Saray (10:52):
This is such a weird example, but I thought of this the other day. Is we would go to church and my grandma would eat a hard candy, like a Jolly Rancher or something like that. She would eat half of it and then she'd spit it back into the wrapper and wrap it up for later.

Joel Clermont (11:12):
Was it just too much for one sitting?

Aaron Saray (11:13):
I guess.

Joel Clermont (11:14):
Okay.

Aaron Saray (11:14):
Great Depression, you know got to make those last. So as we're driving home, she would unwrap it and eat the rest of it. Well, I was really young and I thought, "I want to be like grandma." But no matter what, whenever I would do that, I would eat some of the Jolly Rancher and put it back in the little wrapper, I could never get out of the wrapper then again. It just shredded basically, it was somehow just stuck. I've never to this day understood how she was able to do that.

Joel Clermont (11:42):
First of all, I'm pretty sure it was not a Jolly Rancher. If it was a grandma, it was almost certainly one of those Werther's hard caramel candies.

Aaron Saray (11:50):
Well, I mean it wasn't Jolly Rancher. It was the off-brand one from Fleet Farm. It was like Ranch Jollies or something like that.

Joel Clermont (12:00):
Okay. Jolly Cowboy instead of Rancher?

Aaron Saray (12:03):
Yeah. Happy Cowboy.

Joel Clermont (12:07):
This is a total nerd thing, but the first thing when you were posing the question. I thought of, yeah, my VS code never lints code properly and everybody else it works fine for it. So there's a ghost on my machine. I don't know if this is in the same category, but I cannot roll my tongue and it seemed like everybody I grew up with and everybody in my family to this day can do it and I'm just like, "I think I'm missing a muscle or something in my mouth to make that happen."

Aaron Saray (12:36):
For listeners, what you may not know is we actually can see each other during our podcast, so I'm going to ask. Joel, can you try to roll your tongue right now?

Joel Clermont (12:46):
I can certainly can't do it while I'm smiling. I can feel it in my mouth rolling, but as soon as I try to like open my mouth, it goes away.

Aaron Saray (12:55):
All right.

Joel Clermont (12:56):
So, no, I cannot.

Aaron Saray (12:58):
No, I saw your attempt. It was very blob-like.

Joel Clermont (13:02):
Yeah, no good. Nothing else, I guess...

Aaron Saray (13:08):
Oh, those people that can whistle really loud... I can whistle, you know.

Joel Clermont (13:13):
Sure.

Aaron Saray (13:14):
But those people that can attract someone in the crowd and they're like, "Oh, just stick your fingers in your mouth."

Joel Clermont (13:19):
Yeah, the fingers in the mouth whistling, no.

Aaron Saray (13:21):
I don't know how you do that, it's never worked for me.

Joel Clermont (13:25):
I'm with you on that one. Yeah, I know people that can whistle super loud like that. I'm like, "I don't know what you're doing and I'm not going to take time to figure it out."

Aaron Saray (13:32):
A lot of mouth stuff we're talking about today, but I also can't blow a bubble in chewing gum. I can certainly shoot my gum at you, but I try to... I just never could.

Joel Clermont (13:46):
Yeah, I could do that. I feel like I got one thing going for me here, blowing bubbles.

Aaron Saray (13:53):
Is it time to set up a code review process for your team?

Joel Clermont (13:56):
We can help. Contact us for a free consultation on our website, nocompromises.io.