Is the traditional code review workflow holding you back?

Joel: Welcome to No Compromises. A peek into the mind of two old web devs who have seen some things. This is Joel.

Aaron: And this is Aaron.

We've done other podcasts where we talk about the importance of code review. I really like it too because it's how I can kind of say,

like to Joel or someone else on my team, "This is the work I've done. Can you take a look at it and see if there's any issues to fix, or did I make any bugs?

Jerk any code standards? Stuff like that. And I think we talked about that, and we made a good point that that's important and that's something I don't

ever want to get rid of. However, I was working with another client the other day. I thought this was a one-off, and turns out it might be a pattern.

So it's something I want to talk to you about, Joel.

Joel: Okay.

Aaron: So I was working with this other client, and I put a pull request out there, and they took a look at it. And mind you, this particular work was a while coming,

so it was like maybe two or three weeks. The PR had been there for a while too, and it was like 110 files. So it was sort of a weird sort of scenario too,

where I was starting to forget what I'd done on it, and it was sort of languishing. Well, the other developer was the one who was going to review it.

Took it, reviewed it, made five or six changes, five or six commits to it, and then just merged it in.

Joel: Like, without talking to you?

Aaron: Yeah, without talking to me. Yeah.

Joel: Okay.

Aaron: And I was like, I don't think that's the point of the code review, right? I thought the code review was to tell me, "Hey, you know, here's an issue,

and you can fix it and learn from it. But before you agree with that, let me put a pin in it because I want to talk about something that

happened today with me and Joel. So with me and you, I guess.

Joel: I'm like, "You have another Joel in your life. What's going on here?"

Aaron: So it was another situation where I put in a pull request, and you were reviewing it.

Joel: Yes.

Aaron: And then you said, "Oh, there's two little nitpicky things. Do you want to fix them now, or do you want to do them in the future PR?" or whatever.

And I sort of disagree that they were nitpicky. I thought they were actually more important than they made it sound.

Joel: So I did a good job?

Aaron: Yeah, you did a great job.

Joel: Okay.

Aaron: And we had a time pressure a little bit. So, normally I would have been like, "Okay, write it up and I'll fix it."

And it'll be done in a couple hours, and you can accept and merge it.

Joel: Yeah.

Aaron: But this time you gave me the choice. You're like, "Should we do that or should I just write them in Slack and you can handle it?

Because I know you're working on them again." And I said, "No. Why don't you just go and fix them?" Because they're pretty simple, and they're right there,

and then we have time pressure and whatnot.

Joel: Yeah.

Aaron: And so, that was kind of a little bit different than our normal workflow.

Joel: True.

Aaron: And so, then when you stated the bugs to me, I kind of asked repeatedly. Like, "What was the fix for that?"

And I even went and I looked at the commits that you had done.

Joel: Yeah.

Aaron: And I was clicking through them and looking, and that's why I asked you another question. It was like a locked property in Livewire,

I said, "Hey, why was one locked and the other one not locked?" So you knew I was taking the time to see what you had done, what you had changed,

what you had found, and I was still learning from it, right?

Joel: Yes.

Aaron: So, I could have done it, I guess, with the other project too, I just didn't. I didn't go back and look at that.

I mean, generally know what the changes were, but I didn't look at specifically what I could have done better or if I made a mistake or whatever.

And they didn't tell me either. So the reason I bring all this up, though, is because the time pressure was real, but the world is also changing.

And I kind of feel like with some of the things that we've been seeing in the industry with AI and people pushing to go faster and faster,

we can still have the same quality of work, but we have to change our mechanisms. And so, I wanted to bring it up to you and say,

"Is the way that we did our code review, me and you, actually the right way to move forward now? And how do we make sure we don't lose that

back and forth where we learn from each other, though?"

Joel: So, are you proposing, like, going forward, if somebody finds something, like they just fix it? Is that what you're saying?

Is like a change to the workflow or something else?

Aaron: Perhaps. Like, I'm not sure if there's like a rule there. If it's like small fixes, or if it's something majorly you got to send it back.

I think it comes with, like, you know, you have to kind of figure it out. But, yeah, in the past it was always like it goes back to the original developer

and they fix it. You know, whereas this time we tried something a little bit different. I can see where the hole is because I could have not looked at those diffs,

you know. Maybe I didn't get the emails, you know, and so I didn't necessarily learn. And maybe you didn't even tell me. I mean, you did,

but what if you didn't even tell me? You just saw the issues, and you fixed them, you know, kind of like the other project had went?

Joel: Well, yeah, I was kind of thinking about it. Because from a purely subjective... no, objective stance, the other person fixing your bugs and

just merging it is better for you because they're doing... it's less work for you, right? Like, they're just doing it and doesn't

have to come back to you and this whole thing. And so, in my case, I think the difference was like, we talked about it first,

and it wasn't just kind of like done out from under you. I think that's maybe the piece that if I put myself in your shoes, that would kind of annoy me.

Is like, "What if I don't even agree with what you changed?" or like, "What if you didn't understand what I was doing and you made it worse?"

Like, I did that for a very specific reason. I mean, that's part of it, right?

Aaron: Yeah. Well, I think so. Yeah, I think the reason for a code review is to have a discussion.

Joel: Yeah.

Aaron: But at the same time, I'm sort of suggesting maybe that's an archaic way of looking at it sometimes.

Joel: Yeah, I don't know if I would go that far. I do think it is worth re-evaluating workflows in the light of AI and tooling and the things that weren't

prevalent even three years ago. But if we did that- so I'm not saying hard no, but if we did that, how would it be different from what happened to you

on the other project? Which, I'm taking it, you perceived as a negative. So, like, what would the difference be? Just that it's an agreement upfront, or what?

Aaron: Well... and just to clarify. The reason that other project was negative for me is not because I had some sort of pride issue. I mean, we all have pride, right?

So, my initial thing was like, "What are you doing?" But in all reality, it was like, "I don't know. Did I actually make a mistake?

Because the developer reviewing that's been with that company for four years, and I'm just a contractor."

Joel: Okay.

Aaron: So, maybe I would make the same mistake in my next PR and the one after and the one after if I didn't know what I did wrong, you know?

Or, was it just stylistic? Or was it, "I would do it this way?" You know, was it a... I didn't even have a chance to determine,

like, is this a real issue, or is it just changing it a different way, you know?

Yeah.

Joel: Okay, all right. So, yeah, I guess that's one difference. Like, you and I, the projects we work on, we I think, worked on them equally

or for the same amount of time. Like, we have the same amount of experience, roughly speaking, with the project. So there's less of that mismatch

where maybe it's like, "Oh, I did that wrong." I want to know what I did wrong. Versus like, "I just made a mistake," or, "I missed this one thing

and Joel caught it. And it could have just as easily gone the other way, where I made that same mistake, and you caught it.

And it wasn't like we didn't know to do it, it was just a mistake, right?

Aaron: Right.

Joel: So, I do see that as a difference here. And with the workflow we did today, you did get an email saying, like, "Oh, Joel pushed three more commits."

and then you chose to look at them, and they were all relatively small, so it wasn't a big deal.

Aaron: Yeah.

Joel: But that's part of it too, right?

Aaron: Well, that workflow was frustrating to me too because in my mind, I'm like, "Okay. Well, Joel told me about three bugs, and I'm going to mix this up.

I don't know what's happened. And there was five commits. I'm like, "Ah." Now I have to look at all five commits. You know, I've got email or whatever.

Like, you know, I don't like... Didn't you just, like? Because I gave you one PR, I don't expect... I mean, you may look through them by commits,

but I don't expect you look through them by commits. I don't look through PRs by commits. And so, like your changes coming into my branch,

I then had to surf through a bunch of different commits to try to figure out if it was just one-

You could have just diffed the tip of what I did versus what you did.

Well, so that's a workflow thing. I don't know how to do that off the top of my head. I had an email, it's the only reason I kind of knew that

there was five commits. So then I have to, like, go in and say like, "Well, what is the tip, and what's what was the commit that I made?"

And then I have to figure all that out again...

Joel: Yeah.

Aaron: And so that thing is like, yeah, I need to, but also that's friction. And so as a developer, I'm just going to stop doing that.

Joel: Yeah, I think that the difference is the mechanism. Like, when you open a PR, sort of this project was in GitLab, doesn't matter.

Like, GitLab, GitHub, whatever. Like, by default, it shows you a diff of the whole thing, right? You kind of have to do a little more work to go look

at each individual commit. So, like, your PR was more than one commit. I'm just putting that on the record.

Aaron: Oh, yeah. I know, it was many, many commits. But to me, those are my work stamps.

Joel: Yeah.

Aaron: You know, you don't need to look at them.

Joel: I could have done it all as one. But let's just put that aside for a minute. Like, I think the difference is I pushed commits to your PR,

which is your branch. And now there's not a clear workflow in these UI tools for version control to like, be like, "Oh, I just want to see the difference."

Like there are, I could show you how to go in and pick the different things from the dropdown list, but it's not as obvious as, like,

"Oh, I just clicked diff and see what changed."

Aaron: Right.

Joel: Yeah.

Aaron: And I know I could figure that out, but I'm at this point, I just then don't care because it was a locked property. Assume I trust Joel,

but maybe you did something. You know, again, it's that friction. So the other mechanism I thought of, and I don't think that this is the right answer either.

Joel: Okay.

Aaron: Is like a pull request to the pull request.

Joel: Right.

Aaron: But it's merged in right away, I guess. Or, I don't know how that would work either.

Joel: Yeah, that feels like more ceremony. Like, I feel like it would be easier at that point either to just say, like, "Hey, if you're going to fix somebody

else's PR before you merge it, just do one commit." Because that would also imply if it was so big, like, "Oh, this has to be multiple commits."

Well, then maybe that's the "rule", like, don't just do it. Like, maybe talk about it first? In my case, I think it was just my normal workflow to, like,

"Oh, I'm fixing three things. I'm going to make that three commits." And then, I don't know if you noticed this, but like one of the three ended up

reverting and that's why it actually looked that way, because I rewrote history, so it was actually more commits than ended up being there.

Aaron: Right. Yeah, and I was like, "What is going on here?" you know? I'm like, "And great, do whatever you want. I don't care what happens in Git.

I just care what the final code was." But again, that's all that friction where I was like, "I don't even know if I should try to even learn what he did.

I mean, I know I should, but what's this one PR if I just miss it? You know.

Joel: Right. Yeah, I get what you're saying. It's like you trust me, and like we already talked about in this case too,

so you're even less inclined to have to go digging through it.

Aaron: Right.

Joel: But if you could remove that point of friction, then in the future, if this is a workflow we adopt, then you'd be more likely to actually look at it,

and then maybe some of the essence of the old days is still preserved. Where we're like, there is a back-and-forth, even though it's not

the same exact back and forth as we used to do.

Aaron: So, I don't know where that kind of leaves this all. Is like, I think there's the path that we used to do. Which was like you could send something back

and in wait because someone else was working on something else, and vice versa. Though I think the way and the order in which I work now with automated

tooling and all this assistance is faster. There's a lot less downtime between tasks to reabsorb updates to the previous one.

Joel: Okay. Yeah, because it's context switching for you to have to go back to it.

Aaron: Right.

Joel: And in my case, having just done the review, it's like all warm and fresh context in my head, right? And I'm going to use those same agents,

in this case, I didn't but I could have, to go and make the fix. Yeah, it's an interesting change, so I'm glad you brought it up.

But I feel like I don't know what the answer is yet either.

Aaron: I think you had something a little bit when you talked about, like, "Can your changes reasonably fit in one commit?"

Joel: Yeah.

Aaron: Now, I know that you and I have different commit strategies, and that's perfectly fine. I tend to commit work-in-progress blocks all over the...

Like, to me it's like hitting Ctrl+S save.

Joel: Oh, sure.

Aaron: Not necessary. But, I mean, I know you work differently. But if I was working with a particular person, you know, if I'm doing a merger,

I still would have my own little temporary branch where I just merge or commit it all the time, and then merge it into like one commit or something.

So, I do know how to use Git, it's just we use them differently.

Joel: Yeah.

Aaron: So I'm a little reluctant to say if it's only one commit, then the person can do it, because one commit can mean different things for different people.

Joel: 4,000 lines, one commit, yeah. So, we didn't come to firm conclusion on this. But I guess my takeaway, Aaron, is just the value of taking time

to reevaluate some of these well-worn patterns in workflows. Just in light of the new tools that we have available to us, it's, you know,

don't be stuck in a rut.

I don't even know how to tell you this, Aaron. But there's this thing my brain does.

Aaron: Okay.

Joel: Like I could be driving down the road and I see a billboard. I see maybe even a license plate, something that has letters on it.

And this is not a conscious thing I'm doing, but I'll be like, "Oh, what's an anagram of that?" Like, I'll try to, like, find another word, all right?

I see the look on your face. Other people can't see it, I see it. And you're like, "What is wrong with you?" Right? Say it.

Aaron: No, that's not what that look is. That look was, "I can't believe I didn't know this until now, because I watch a television show that I think you would love."

Joel: Oh, is it Wheel of Fortune?

Aaron: No, it's called Countdown, and it's in UK. Honestly, I don't actually watch that version of it. I watch one called Cats Does Countdown,

which is comedians doing math. But the show is the show is a math problem and a word problem, and the words basically are anagrams.

And they give you a bunch of letters, and it's like whoever gets the most letters out of this word, or the largest word of these letters, wins,

I mean, nothing. But it's a hilarious show. Oh, you got to watch this now.

Joel: Okay, I'm going to. But so the reason I even told you this is because the other day I had one for my own name, Claremont. And it's LemonCRT.

And so, I just wanted to share that with you, and I didn't know how else to share it without telling you the rest of it. But I was pretty proud of that one.

Aaron: Nice.

So, do you want us to come into your project and question everything with no absolute solutions whatsoever?

Joel: We won't do that, Aaron. But we will come in, and if we see something that needs improvement, we can make your project better.

Head over to nocompromises.io and see how you can work with us.