Let's review some practical refactoring considerations using real-life code. So here I have a class called NotifyGiftCertificateRecipients. It's an artisan command. And from the description, we can see, deliver all certificates that are scheduled for today. Yeah, so you can imagine an artisan command that runs once a day, automatically. It'll fetch all gift certificates where the deliverAt field is set to today. And then it will loop over them. For each certificate, fire off a notification to the recipient. Then mark the certificate as delivered and update the database. Okay, so already I can see a couple things I might change here.

Then mark the certificate as delivered and update the database. Okay, so already I can see a couple things I might change here. And we'll come back to it in a minute. For now, just pay attention to this section here. All right. Next I have a class called PurchaseGiftCertificate. Now I think of these classes as use cases. And those are classes that represent a particular action a user can take in your system. In this case, purchasing a gift certificate. And one thing I like about this is how it contains all of the steps for purchasing a

In this case, purchasing a gift certificate. And one thing I like about this is how it contains all of the steps for purchasing a gift certificate. Or in other words, they're not dispersed throughout your system with eight different event listeners. Sometimes you need to do that. But then also sometimes you didn't necessarily improve the code. You just moved functions from here into this other file that you won't remember six months from now. Anyways, depending on the context, this is one approach you might consider. So you'll see in our handle method, we break up the steps into their own methods.

Anyways, depending on the context, this is one approach you might consider. So you'll see in our handle method, we break up the steps into their own methods. So if I want to purchase a gift certificate, we charge a card. Then we generate the certificate. Then we notify the purchaser. And then finally, we check, well, did the purchaser set the certificate to be delivered today? If so, notify the recipient immediately. And then we have what is effectively a callback. We announce that the purchase is complete.

And then we have what is effectively a callback. We announce that the purchase is complete. And that's it. So not much going on here. Process the charge, create a certificate using Eloquent. And then once again, right down here, I'm seeing some repetition. So this encapsulates notifying a recipient, right? Fire off a notification, then mark the certificate as delivered, then save it. Notice how similar that is to what we had here. Fire off the notification, mark the certificate as delivered, and then update the database.

necessarily their responsibility. It's sort of a weird tangent, admittedly, but that's how I think of it. When I see code like this, I see an artisan command that is fully in control. I'm going to send the notification, and then I'm going to update your internals, and then I'm going to persist. So how do we get around this? Well, if we want more autonomy, we want the certificate to be in charge rather than the artisan command or the use case. Well, what's going on here? Well, it's written right there on the 10.

Well, what's going on here? Well, it's written right there on the 10. We are delivering the gift certificate. So that's a verb. Could we work that into a method like certificateDeliver? And what if we could make that the code? Hmm. All right, let's give it a shot. I'm going to start by cutting all of this out. I'll switch to my GiftCertificate class.

I'm going to start by cutting all of this out. I'll switch to my GiftCertificate class. Mostly here, you can ignore just about everything. It's pretty standard fare. But I will add a new method called deliver, and I'll paste in everything from that artisan command. Now here, notice before, the artisan command was getting its clause into the internals of certificates. But instead, let's just say this recipientEmail. Now that we are in a GiftCertificate class, we have an instance there.

But instead, let's just say this recipientEmail. Now that we are in a GiftCertificate class, we have an instance there. Next, the same thing here. We already have a certificate, so I will reference the instance. And then finally, this can be removed, like so. Next, what else? Here that's fine. But if you have your guarding set up correctly, I could just say this updateDelivered is true. And then I can bring that down to a single line.

It should not be responsible for it. That sort of code should either go on a controller or a service class or within, as many might say, an event listener. And you know what? That's a valid argument, and many, many people would agree with you. However, I do have one note here. Which approach will make the code more clear, more easier to come back to? Is certificate deliver better, or would storing it on a controller or service class be better? And be honest here. If you're doing on the service class or you're taking an event approach, that would mean,

And be honest here. If you're doing on the service class or you're taking an event approach, that would mean, All right, well, when it's time to deliver the certificate, we create a new event class using php artisan make:event. We would next create an event listener class. Next, depending upon whether you have auto-registration turned on or not, you might have to register the binding between the event and the listener. And then finally, in the event listener class, you would have what is effectively these four lines of code. So when considering refactors, always opt for, in most cases, unless you have a really

lines of code. So when considering refactors, always opt for, in most cases, unless you have a really good argument, but always opt for the approach that makes the code more clear. Rather than referencing requirements and rules, well, this violates SRP, or this should not be responsible for that, or you're not supposed to do this over here, we often want to reach back to those rules to limit us from writing the thing that will make the code most clear. That's what I want. And further, now that we've reduced what we had before, all of that, down to this, now because I have a Collection class here, I could use the higher order proxy and just say scheduled for each one, deliver it.

because I have a Collection class here, I could use the higher order proxy and just say scheduled for each one, deliver it. And now we're starting to get a bit more clarity here. You could further do things like this, where you extract this whole thing to a method, and we could say certificatesScheduledForToday, get rid of the doc blocks there, and then inline that, yeah. Next we have a variable that's only used once, so let's inline that further. And now our handle method looks like this. Let's read it out together. This certificateScheduledForToday, for each one, deliver it.

Let's read it out together. This certificate scheduled for today, for each one, deliver it. A little better, don't you think? Next though, we have this Eloquent query here, and it's not horrible, but again, think of it sort of like the artisan command knowing the internals of what makes a gift certificate a gift certificate. For example, the delivered_at timestamp, the delivered Boolean, and things like that. Sometimes there's no way around it, but it's a consideration. So let's see if we can move this, once again, to gift certificates, and what would the method be called?

So let's see if we can move this, once again, to gift certificates, and what would the method be called? Give me the giftCertificate. Let's paste it in like that. All right, give me the giftCertificates that are scheduled for today. Scheduled for today, be very clear, and should it be a static function that performs a full query? Should it just be a query scope? I'm not sure. It would depend on if I would ever need to do anything different with this portion of

I'm not sure. It would depend on if I would ever need to do anything different with this portion of the query. And if the answer is yes, I think a query scope would be the way to go. But if the answer is no, then I'm inclined to leave it like this. Okay, so let's go back. Let's just see if we made the code better. We're going to return giftCertificateScheduledForToday. But now notice something. The method is called certificateScheduledForToday, and the code says giftCertificateScheduledForToday.

But now notice something. The method is called certificateScheduledForToday, and the code says gift certificateScheduledForToday. At this point, I don't think the extracted method is needed anymore. So let's replace it, like so, and get rid of the method, and here we go. Gift certificates that are scheduled for today, for each one, deliver it. So have a look. If I run git diff on this file, all of this could be replaced with a one-liner. I think that's an improvement. So now, if we go back to purchase gift certificate, we're back to our use case. Let's see if we can make use of some of those changes, as well as a couple more.

So now, if we go back to purchase giftCertificate, we're back to our use case. Let's see if we can make use of some of those changes, as well as a couple more. Okay, so let's have a look. We're going to scroll down here to where we notify the recipient. And yeah, we've already refactored this, which means I can change this to thisCertificate->deliver. And even better, because the certificate already knows who the recipient is, there's no reason to pass in the email there. We're giving more autonomy to the certificate. Okay, so now that means notifyRecipient delegates to thisCertificate->deliver.

We're giving more autonomy to the certificate. Okay, so now that means notify recipient delegates to this certificate deliver. I'm not sure I need an extracted method there. So I could bring this up here, remove the method, and I'm now shortening the file a bit. Yeah, so little refactors, and I promise they really do add up. That is the key. Don't think of big refactors. Think of multiple little ways to clean up the code. Can we remove this conditional?

Think of multiple little ways to clean up the code. Can we remove this conditional? Can we extract this method or inline this method? Can we delegate and let this class be responsible for that logic rather than that class? And then when you review those 20 little refactors, they add up to a big difference. All right, so now at this point, I'm just going to do the squint test. And that's where you go over the code and you sort of scan it, and you squint your eyes and you wait for something to pop out at you. In this case, the things that pop out at me are often the more weighty, dense sections of the code, or even where they take up the most space.

In this case, the things that pop out at me are often the more weighty, dense sections of the code, or even where they take up the most space. So that sort of jumps out at me. If I scroll down, that doesn't necessarily jump out at me. We'll talk about that, but this section does. Here you can see I imported the Notifications top-level namespace, but then we went one level further. I don't see any real reason to do that. So let's pull it in fully, and that will allow me to just reduce the line length a little bit.

So let's pull it in fully, and that will allow me to just reduce the line length a little bit. Next, it's still kind of wordy. And I'm not against class names that are wordy, but if you can think of something more succinct, then why wouldn't you? Even if it's as simple as removing the notification suffix, I'm not sure there's a good argument for that to be there. So now if we notify a new GiftCertificate purchased, I think that ends up being a little cleaner. I'm going to keep it at notification, but yeah, that is a refactor you might consider.

own judgment. Well, I can't do this because this rule says SRP, or this rule says I have to do that. But the reality of coding is many times it does come down to your instinct and your preference and your personality. So why would I put one notification on an Eloquent model, but another notification here? And the answer is my judgment tells me that delivering a gift certificate to the recipient, that is a core thing it does. That is a very important thing that it does. Notifying the purchaser, this is effectively the receipt. Thank you for your purchase.

Now, I like that we're delegating deliver, which we weren't doing before, but could we replace this by just asking the Certificate if it's scheduled for today? Because it turns out that's an important thing, finding out, is the Certificate scheduled for not tomorrow or next week, but today? We keep checking for that. So why can't the Certificate be responsible for that check? All right, well, if we take that approach, I could go to GiftCertificate, and we'll say right here, isScheduledForToday. And I'm just going to move all of that code we had earlier, parse, this deliver isToday.

And I'm just going to move all of that code we had earlier, parse, this deliverAt is today. Now, I'll be honest with you, I think deliverAt, that field should be set as a timestamp. And when you do that with Laravel in the casts property, it will automatically be a Carbon instance. I'm not sure why that wasn't done earlier in the code, if there was a reason for it. But yeah, if it's a refactor we can do, that would allow us to replace this with this deliverAt is today, which is even cleaner. But yeah, like I said, I'm not entirely sure right now why that wasn't done. So we'll stick with the exact code we had earlier, but that's something to consider.

But yeah, like I said, I'm not entirely sure right now why that wasn't done. So we'll stick with the exact code we had earlier, but that's something to consider. Okay, so now if I come back, it reads a little bit better. It's still a conditional, but we're letting the certificate let us know if it's scheduled for today. Now, on that note, I think this is probably okay, but sometimes you might even want to take it further, where you say certificate->deliverIfToday(). You almost sneak in the conditional through the back door, and you include it with the method call. It might be something you consider.

back, it does clean it up a little bit maybe. So now the steps required to purchase a gift certificate are charge the card, generate the certificate, notify the purchaser, conditionally deliver the certificate today, and then trigger our callback. And at least for this lesson, that's the only refactoring we're going to do, but it does clean up the code. And then further, if we go to the ArtisanCommand class, that drastically cleaned up the code. So I would call this a successful series of refactoring. What do you think?