Evaluating Refactor Quality0:00
A key technique for any developer's tool belt is learning how to measure whether or not a particular change or a refactor actually made the code better, or if you simply added some complexity, and it feels good because you've made more files. But again, this key phrase, is it better, should be something you ask yourself constantly. So I'm going to show you an example of this using the Council codebase, which is an open source forum we've built here at Laracast. Recently a PR came in that refactored some of the model hooks. If we take a look at this, don't worry about the code too much, only focus on what I'm showing you here. So you'll see a Thread, this represents a single thread for a forum, and we do have
showing you here. So you'll see a Thread, this represents a single Thread for a forum, and we do have a couple model events here. So we're saying deleting, well when you are deleting a Thread, we need to make sure that we delete the replies associated with it. We could also do that on the MySQL end, but at the moment that's done on the php side. And then we also want to make sure that if you delete a Thread, the User's reputation, it's like experience points if you think of a video game, then we will decrease that. And then we have another one, when you create a Thread, well we're going to dynamically set the slug, we'll fire an event, and then we will add reputation to the User.
And then we have another one, when you create a Thread, well we're going to dynamically set the slug, we'll fire an event, and then we will add reputation to the User. Now we also have another one on a Reply. So if you have a Thread, well a Thread consists of Replies. And if we take a look at that, once again we have two more. When a Reply is created, well increment the replies count, we're actually hard coding that on the thread table, just for convenience, it's helpful. And then of course the User gets some experience points. Next when you delete a Reply, well decrement the replies count, remove the reputation, and then if this Reply happens to be the best one, well we have to remove that best Reply
Refactoring to Observers1:38
Next when you delete a reply, well decrement the replies count, remove the reputation, and then if this reply happens to be the best one, well we have to remove that best reply from the thread. Okay, so this is a basic example of model hooks. Now a PR that recently came in refactored this. So I'm not going to touch the PR, but we are going to go through the process of refactoring, and then we're going to look at the new code and ask ourselves, is this necessarily better? Let's get started. Now to begin I have a full test suite here, so let's give it a run. And everything is returning green.
Now to begin I have a full test suite here, so let's give it a run. And everything is returning green. Let's begin on the Thread class. We'll come to those model hooks, and I'm going to comment out this entire boot method. Now if we give it a run, you're going to see a bunch of things break. There we go. You can see, what do we have, one, two, three, four, five different breaks. Now what we could do is visit our EventServiceProvider, and we could register an observer. For example right here we could say app()->make(Thread::class)->observe(), and we'll build up a new observers folder here, and the class name will be StartThreadObserver.
For example right here we could say App\ThreadObserver, and we'll build up a new observers folder here, and the class name will be ThreadObserver. So as you know, for any model you can declare them here, or you can also create a dedicated class, which we would refer to in Laravel as an observer. So here we're just going to apply that. All right, let's create that App, a new observers folder, and ThreadObserver. So I'll build up my class here. And yeah, as a quick recap, you can hook into any of Eloquent's model events, like created or deleted or updated or deleting. Anyways, in this case we'll say called, just to show you that it is working.
or deleted or updated or deleting. Anyways, in this case we'll say called, just to show you that it is working. Oh, and of course, let's set the namespace. All right, let's give it a shot. php artisan tinker, and we will create a Thread, and if everything was done correctly, sure enough we did hit that method. All right, so our observer is in effect, which means right here I could take this bit of code and paste it in here, uncomment, and reformat. Next let's make sure Thread was published, was hooked in properly. Finally, I will accept the Thread that was created, the model.
Next let's make sure thread was published, was hooked in properly. Finally, I will accept the thread that was created, the model. All right, next we also have one for deleting, so let's hook into that event. And then here we can grab this section there, paste it in, accept the thread, and reformat. Yeah, oh, sorry, been doing a lot of JavaScript work lately. So now I think we should be back to green if we give this a run. We've extracted both of those to a dedicated ThreadObserver class. Uh-oh, we have two failures. So that's odd, because we have successfully, we've moved all of this logic to a thread observer.
Fixing Test Failures4:24
So that's odd, because we have successfully, we've moved all of this logic to a ThreadObserver. But now they're being triggered, so it should still be working. It looks like we have an issue related to the activity test. Hmm, let's see. It records activity when a Thread is created. So it's going to be related, I'm pretty sure, to this trait I have here called RecordsActivity, where we boot it. So I'm thinking when we register the model observer, it's going to run these as well. Hmm.
So I'm thinking when we register the model observer, it's going to run these as well. Hmm. Let's give this a shot. I'm going to do a phpunit filter on that single test. Is it not showing us as logged in? Let's see, auth check. Oh, that's returning false, even though the test assumes it's true. So yeah, I guess when we register the observer, this is being triggered sooner than it was before. Honestly, I'm not entirely sure, but maybe we can just push this further down the rabbit.
But anyways, I think at this point we should be at green. And we are. Okay, great. So that's one refactor we could make. And yes, we could get rid of all of this entirely. Well then, we could do the exact same thing for reply, right? So let's give that a shot. We'll go to EventServiceProvider, and we will register a reply observer. You can also do this as a class, by the way. You could do that number if you want.
You can also do this as a class, by the way. You could do that number if you want. It will accept both. Anyways, so now we're going to create a new one, App\Observers\ReplyObserver. And let's see, let's steal a little of that, like so. And then clean these both out. Okay. And then I guess these would be reply. So let's go over to reply, and we can see when a reply is created, we're going to trigger this.
So let's go over to Reply, and we can see when a Reply is created, we're going to trigger this. So paste that in. And when a Reply is in the process of deleting, then we'll do that. And refactor. Okay, so now, let's see, let's try it again. Can we get rid of that entirely? Let's run our test suite, and we'll check if we're still at green. And clearly, no, we made some big mistakes here. So let's see what we did wrong.
And clearly, no, we made some big mistakes here. So let's see what we did wrong. In EventServiceProvider, we did register a ReplyObserver. There it is. Whoops. We registered ReplyObserver on Thread. So of course, that's not what we want. Anyways, if we give it a run now, I think we should be at green. And we are. All right, so yeah, that's another refactor we could do.
And we are. All right, so yeah, that's another refactor we could do. We're not saying it's any better. We're just going through the steps right now, and then we will re-evaluate. Let's go to GitHub. Here's the changes we've made so far. So within Thread, we got rid of this section right here, just one method. In Reply, we got rid of that section. This is an unrelated fix, so you can ignore that. But instead, we registered two observer classes, and we have one file for that, and another
Splitting Observers by Responsibility7:54
Is this better than what we had before here, where we just registered it on the model? Not too much going on there. Just a couple quick eloquent calls. So is this better? Let's take it a step further. Some people may say on our ThreadObserver, well, you're doing too many things here. So if we want to better follow a single responsibility principle, it'd be better if you had maybe an Observer for each thing that needs to take place, almost like a handler. So for example, here and here represent reputation. All right, well, maybe we can have a dedicated Observer for that.
So for example, here and here represent reputation. All right, well, maybe we can have a dedicated observer for that. We'll call it ThreadReputationObserver, say namespace App\Observers class ThreadReputationObserver. Okay. So now, this section here, and move it. So when you have created a Thread, then we can move it in here. That can now leave there. Next, losing a reputation. So when we are deleting a Thread, we need to make sure that we lose reputation. Okay.
So when we are deleting a thread, we need to make sure that we lose reputation. Okay. So now, I'll get rid of that. And we've extracted that specific functionality into its own observer. But it hasn't yet been registered. So if we give this a run, I can't remember what class is responsible for it, maybe ReputationTest. But anyways, at some point, yeah, there we go. We can see things are starting to blow up in ReputationTest. Let's quickly do a filter down to ReputationTest, so you can see these tests specifically.
We can see things are starting to blow up in reputationTest. Let's quickly do a filter down to reputationTest, so you can see these tests specifically are failing. Okay. We'll go back to EventServiceProvider. Now we're going to register specific ones. So yes, we have a ThreadObserver, but we're also going to have a ThreadReputationObserver. So now, if we give that a run, everything is back to green. And if we do our full test suite, magically fast, and we're at green. All right.
And if we do our full test suite, magically fast, and we're at green. All right. Let's take it a step further. Here, this kind of represents its own thing, right? So we're updating the slug. All right. Well, maybe we're going to give that its own one. UpdateThreadSlugObserver. I'll paste in a snippet here from earlier, and we'll update this. Okay.
I'll paste in a snippet here from earlier, and we'll update this. Okay. Now we can see when it's created. This is a very common refactor you'll see, so we're going to talk about this in just a minute. All right. That means I could get rid of this entirely. If we give it a run, it's going to fail once again. And it did. You can see a Thread requires a unique slug.
And it did. You can see a Thread requires a unique slug. So that specific piece is failing. All right. Let's now add it. And this would be what? Generate ThreadSlugObserver. And give that a run. And now that part is back to green. Okay.
And now that part is back to green. Okay. So we're just going to keep taking this step by step. This one here, well, that's responsible for deleting the thread replies. All right. App\Observers\DeleteThreadRepliesObserver. Once again, we'll paste this in. DeleteThreadRepliesObserver. And this one takes place when deleting.
Delete thread replies observer. And this one takes place when deleting. So that can be removed entirely. And we'll leave this one here just for firing the event. All right. So we run the suite just like before. It'll fail. And it does. So we'll exit out. Come back to EventServiceProvider.
So we'll exit out. Come back to EventServiceProvider. Register another one. And this is DeleteThreadRepliesObserver. And we're back to green. So let's close all of this out. Take a look. app/Observers. So now we have all of these files. And it is true, each one does a specific thing.
Centralizing Observer Registration11:40
So now we have all of these files. And it is true, each one does a specific thing. So we have one observer that handles this, one for this, one for this. And we haven't even gotten to reply yet. Now we could, let's keep going just a little bit further. Now we could clean things up a little bit right here by extracting a property here. So let's say protected $observers. And then we could say, well, for a Thread, here are the observers we have. So we have one for, well, to start, ThreadObserver. I'll import that.
So we have one for, well, to start, ThreadObserver. I'll import that. We have another one. What else? For ThreadReputationObserver. Import that. And then one for GenerateThreadSlugObserver. And then finally, we have DeleteThreadRepliesObserver. All right, so do note at the very top, I have imported all of those. That means if we scroll down, I could get rid of these four.
All right, so do note at the very top, I have imported all of those. That means if we scroll down, I could get rid of these four. And then we could do the exact same thing for the Reply. And at the moment, we do just have that ReplyObserver, don't we? Yeah. Oh, and I'm sorry. I'm sorry. Of course, these do need to be strings. But yeah, you get the idea. That does clean things up a bit.
But yeah, you get the idea. That does clean things up a bit. And then down here, we could say, well, for each $observers as $observer, and that will be the model. And then we'll have an array of $observers. So as model observers. Then we need to filter through the $observers. So once again, for each $observers as $observer, we'll do the same as before. So take the model. So we basically want to say $reply->observe($observer), like so.
So take the model. So we basically want to say Reply, observe this Observer, like so. And yeah, I think that would do the trick. So if we give this a run quickly, and yes, we are back at green. So it cleans things up a little bit. The right Observer now just has a single one that you could even put somewhere else if you wanted to. Behind the scenes now, I'm going to go ahead and do reply as well, since you get the basic gist. And then we're going to re-evaluate.
gist. And then we're going to re-evaluate. Okay, flash forward. You can now see I've removed the ReplyObserver entirely and replaced it with three targeted ones. One for ReplyReputationObserver. So when a Reply is created, we add and lose reputation. This follows single responsibility principle, right? Thread repliesCount. This handles incrementing and decrementing the repliesCount.
Thread replies count. This handles incrementing and decrementing the replies count. And then we have a best reply observer that determines if we need to remove the best reply from the associated thread. Okay, so now if I run phpunit here, everything's going to return green. And it does. And if we take a look at GitHub, here's everything we've done. So once again, let's take a look at what we removed. Here is some functionality on thread and reply. And what we've replaced it with is an event service provider that lists all of our observers.
Reassessing Tradeoffs and Principles14:35
Here is some functionality on Thread and Reply. And what we've replaced it with is an EventServiceProvider that lists all of our observers. It filters through. And for each one, it registers the observer. And then we think we're following single responsibility principle. So we created a class for that, and that, and that, and that, and that, and that, and that. And it goes on and on. So for every new thing that needs to take place, well, we would create yet another file. And again, this is a situation where, to reiterate, you could say it does adhere to the single.
So for every new thing that needs to take place, well, we would create yet another file. And again, this is a situation where, to reiterate, you could say it does adhere to the single responsibility principle more than what we had before. It better adheres to the open close principle, which says a class should be open to extension but closed to modification. And in this case, that would fit that. Because if you want to hook into another model event and do something else, you would create a brand new observer and simply update your EventServiceProvider. You wouldn't have to touch Thread at all like we would with the old implementation. But again, here's the question that I proposed at the beginning of the video.
