When weird code needs to explain itself

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

And this is Aaron. So, I was doing a code review on some of Joel's code the other day, which-

Beautiful code.

Of course, it's just amazing.

Of course.

And we're using Mary UI on one of our projects. It's an older version because of some of the requirements of the project.

And we're doing some sort of successful thing, and there's this toast system that you can issue a success message. And there's built-in ways of doing that.

You call this success or something like that, and it just handles all that. And I was doing the code review on Joel's code, and I saw him not using that and then flashing maybe a message into success or something like that.

And then later on in another, what seemed to be unrelated component maybe, it was checking in mount for that success message.

Yes.

And then issuing a toast message after that. And I was really confused and then I remembered something.

I remembered because of the nature of this old system and whatever, we would sometimes see a duplicate success message in certain scenarios.

Yes.

And I vaguely remember Joel talking about that, but I can't remember. And at the time I'm doing this code review too, or like, well, maybe a week later, I'm just like, "This seems stupid."

"Why are you doing all this work, Joel?"

Yeah. And I'm like, "What are you doing? Why can't you just call success?" I'm like, "I seem to remember something, but why is this over here though? That doesn't seem like what we talked about."

I'm forgetting and so I kind of marked it off. I'm like, "This feels inconsistent. I don't know what you're doing here." And the fact that I'm asking you to explain it to me means that there's something invalid...

not invalid, but there's something unclear about the code. Whether it's... If anyone has to stop and ask a question during code review and be like, "What is this even for?"

You know that there's some sort of problem there. So, what did you feel like when I stopped and been like, "What are you talking about?" Because you know that I even agreed with the solution.

Yes.

But then later on, when it came to code review, I'm like, "This is horrible. What are you doing?"

Well, what was fun to me because I think this was not a huge PR, but there was like maybe four or five different Livewire components that each kind of shared this pattern.

And it was like the first time you encountered the PR you're like, "Oh, this is weird. Isn't there a different way to do this?" And the second time you're like, "Joel, why are you doing this again?"

I think then the final comment on the PR is like, "What is going on?" So, I could sense the increase in concern over, "What is going on here?"

So, just as a side point, I think adding to all of this, this was a PR where I finished it late at night and I didn't even write up that great of a description. And I think that just added to your frustration.

So, I might have headed this off if I would've put a reminder note in there, but I think it served as a good opportunity for us to talk about here and the code got better. So, let me just clarify one thing.

Hold on there a second. I just want to like... You are right. I appreciate you saying that you could have wrote a better note.

Yes.

But also, if the explanation is only in the note, and without the note, the code is confusing, it's interesting. Because we should have done it better with the description.

But also, like you said, this kind of showed that there was a hole in what we were even looking at.

Yeah. We're not the only two people on this team. So, if somebody else didn't read that PR note, they would've likely had the same questions. So, the pattern was we have one component for create, one for edit, and then we had an index view.

And anytime you would make a create or an edit, it would show a toast message and redirect you back to the index view.

But what was happening is, it would start to show it and then mid redirect, it would disappear and then it would come back. So, it was like, we're in spa mode for Livewire.

And like you said, those old versions these bugs have all been fixed. But the version we were on this little UX flicker was there, and that's what we were working around.

So, instead of using the simple, this success helper, which does the redirect and the toast in the create component, I would put the message in flash, I would do the redirect, and then in the index component, I would look for the flash and make the toast appear.

And you're like, "Why are you doing this?" So, anyways-

Well, and it's funny because even as I was getting irritated with it, I remember something vaguely in the back of my mind being like, "I know we had to do this for a reason, but this is stupid."

"Yes, Joel is not this bad at programming. He would know this other method exists." So, anyways, you raised the issue and then we talked through it.

And it's like, well, what are we going to do about this? How? Because it's a pattern throughout the app, right? It's not like this is in one place, but it was in multiple places.

And even within a single component area, you had the index and the create. So, the code wasn't all in even one spot. So, should we talk about the solution?

And then I think this kind of highlights maybe a general pattern for doing weird things like this and documenting it in a more self-evident way.

Yeah, this is going to be one of those solutions where it's like, "That's the perfect solution, I think, for this. But that's not always the perfect solution for everything."

Correct. Yes, this is not a general pattern. But for this particular case, we decided to use a trait. And the thing I liked about... So, the trait does two things.

It just has two methods in it. There's the one method, which is sort of on the create or the edit side where it puts the message into the flash and it does the redirect.

And then there's a second method that the index uses to look for this special success message and then show a toast. So, the code ended up being identical.

But first of all, moving it to a trait now we could co-locate these two weird things. And at the top of that trait, I think there's a big paragraph or even two-paragraph doc block explaining what the bug is.

It only happens because we're on some old version. We can get rid of this when we move to the next version.

I think we even linked to a GitHub issue or something, but there's lots of explanation as to why we were doing this. And I think the trait was even called something really on the nose like, FixesToastFlicker, or something like that.

So, it was very obvious, like, "Oh, this is not a built-in trait that comes with Mary UI. This is something weird that's happening. I should go look at that."

And then even the two methods had the same thing. So, instead of this success, it was successWithFlickerFix. And the mount was mountFixesMerryToastFlicker.

So, all these things combined, when you would see this on either side of the component, it was very obvious this is something special.

And as soon as you clicked into it, then all the documentation was there as like why this pattern exists in the first place, and when we can get rid of it in the future, hopefully.

Well, and the interesting thing about that too, is that the solution that Joel came up with, I approved of. I thought it was a great solution, coding-wise.

And during code review, I rejected it because I didn't understand it while it was still a great solution. So, this kind of shows you the importance of... I know we come back to this as a joke, but naming things.

First of all, naming things is important. Second, understanding architectural choices are more than just where the code belongs. Sometimes it's about putting the code in a specific bucket, it demonstrates something else.

So, that's the reason why I brought up like, this was a solution for this problem but it's not always a solution for everything.

Because I've seen it too many times when people will reach for traits just to make the code smaller in their one thing. And that's not what we were doing here.

It legitimately was a thing that's reused. And plus with Livewire, we got a couple bonuses of having those named mount methods after the trade automatically wires it in.

Yeah.

So, it was very specific solution. But what's interesting is, like I said, the underlying code, the way it was having to handle this bug all still the same, just moved to a different location, some documentation, and then it's amazingly clear.

It's documented and it's clear enough that it deserves the respect of the good code.

Thank you. And I also think too, it's not just about me and you, Aaron, we work on teams with other people and in this particular project, we're just working on a small slice of it.

We didn't want to introduce this pattern and all of a sudden the team starts copying it without even knowing why it exists. Or, maybe our coding buddy, you know, our agent sees these patterns too.

And just, all these things to add the documentation, makes it clear for us in the moment to get pass code review, but then broader just to make the team more informed as well.

Picture this, Joel. You're at an amazing, nice restaurant. You have a great meal, it's delicious. The chef comes out the table, you thank them.

You say, "That was a great meal." And then right after that you immediately tell them, "I wish you'd cook more for me.

I wish you'd make even more meals. Why doesn't this meal come to my table faster? It was a good meal, but it was a little bit expensive. And come on-"

What?

"I need many more meals right now. Let's go."

You had me up until you started asking all these questions.

I was just thinking, as a programmer wouldn't it be just nice to be a chef one time? Where after you did a project, they just kind of said, "Hey, good job," and just stopped.

Like, have you ever noticed that? And we do it to each other too. It's like, "Oh, that's great. That's awesome. Can you do it more?" It's like, come on, just give me a moment. Let me just enjoy it. Treat me like a chef.

Yeah.

Joel, I noticed you had some internet problems lately, and you couldn't get online. Good thing you have all the documentation for programming in your brain, right?

Oh, if only that were true. No, I did have local resources, but one of those I think everybody would benefit from. Which is our Mastering Laravel Validation Rules book. Head over to masteringlaravel.io and take a look at that.