It’s always a good idea to take time to review old code you’ve written and see how you can refactor it to be cleaner, modern, and easier to understand. This weekend, I’ve been doing just that with one of my old demo projects, BandTracker. It’s a Laravel 5.4 app I wrote about a year ago as a demo project for a company I was applying for. I didn’t get that job, but the project did end up helping me land my current job.
Since then, I’ve grown significantly in my programming knowledge, and I decided it was time to revisit this old project and refactor it. Many things were updated during this process: I upgraded from Laravel 5.4 to Laravel 5.6, I added unit tests for the existing features, and I refactored the code to follow Laravel best practices, including PSR-2 conventions.
This article is about one part of the refactor: the AlbumController. Here is a link to what the file used to look like: OldAlbumController. Take a look. It’s…ok. It works, but it looks cluttered and slightly complicated. Not to mention this isn’t PSR-2 compliant at all, or friendly to Laravel best practices. Let’s fix that!
You’ll see me include
<?phptags at the beginning of each gist. That’s to get the syntax highlighting.
First, we start with the index method.
Right away, you can see a typo in the first inline comment. That if statement also looks a little ugly, even though it’s doing its job well enough. Also, 2-space indentation. It’s what I liked at the time, but now we’re trying to follow PSR-2 (and I’ve used 4-space indentation long enough that 2-space indentation now hurts my eyes to look at).
Let’s improve on these things.
That looks cleaner, doesn’t it? Instead of an if statement, we’re now using a ternary operator, which takes up less room and is easier on the eyes. I also added eager loading (the ::with('band') part) so we don’t end up with N+1 queries when our album index view calls $album->band->name or any other property it needs from the parent band. I run my unit tests to make sure my changes didn’t break anything…and nothing broke. Yay!
You can assume from this point on that I’ll keep running unit tests after I make a change.
Next up, the sorting functionality. If you look at it, you’ll notice that it calls $this->sort(), and if you further inspect the old controller you’ll see that there’s no sort method to be found. Where is it? Well…it’s in the parent class. I added it there so both the album controller and the band controller could make use of it. That’s not cool, and it needs to be dealt with. I also want to remove the for loop attaching band_name to the album. That’s what $album->band->name is for!
Let’s make it happen!
Much better. I already changed the band controller to use this same code, so we no longer need to use the crappy old sort method parasitically attached to the parent Controller class. I also used a switch statement instead of an if statement, because in cases where I’m expecting specific values I find this cleaner and easier to read. It also allows me to get rid of the redundant $sortdirection ternary. I normally love ternaries, but that one wasn’t a pretty sight.
Why didn’t I abstract the sorting logic into a service? It’s a valid option, and arguably a “more correct” choice from a DRY standpoint…but at this point, I felt it was more complexity than I needed to deal with. I’d need to set up a service provider, bind it to the app, include another facade, find a new, clean way to change $sortdirection…right now, I just need basic sorting, and the code written is easy to understand. For now, I’m okay with being a little WET.
I could also have used a trait to graft a sorting function to my controllers, but, again, that is more complexity than is needed right now.
When I initially made this change, my unit tests reported an error. Now that band_name, is no longer an actual attribute on the Album model (because we took out the for loop), there’s no longer anything to sort. I changed the parameter being passed in to band.name to allow Eloquent relationship magic to work, and that fixed the issue.
On to the next part. The band grab is alright, but we don’t need every single parameter. Let’s narrow the scope of our grab.
Nothing fancy, just adding a select to only grab the band’s name and id.
The last part doesn’t need much work, either. I’ve taken to prefer the compact() way of passing variables into the view, so I’ll update my return to use that.
Notice that I’m now directly using the variables I assigned earlier, instead of calling $request->variable on some of them, which allows me to compact everything and avoid the ugly-looking ['variable' => $variable].
And that’s the index method! Let’s see the changes in full.
It’s about five lines longer (thanks to the syntax of switch/case), but despite that it’s a lot easier to read. Plus, now we’re PSR-2 compliant! We also optimized our queries and cleaned up the return statement. Overall, a good start to the refactoring process!
I used a linter on my editor — Visual Studio Code — to help me identify where my code was violating PSR-2 standards. I highly recommend using a linter whenever you need to enforce any kind of style guide; it’s much easier than just eyeballing it!
Time to deal with the create method:
We can improve it slightly to make it easier to read.
I added breathing room between the lines so it’s less dense, and thus less of a cognitive burden to read. I also compacted the variables being passed into the view, as I did in the index method.
You can assume I’ll keep adding
compact()to my returns from this point forward.
Next up, the store method:
Yeah, it looks gross, doesn’t it? I’m validating directly in the controller, I directly created a new Album model, I’m individually storing every parameter to the model…yuck. Let’s fix this, stat!
First, I enter php artisan make:request AlbumRequest in my terminal to make a Request object, and then move all of my validation rules to it (after making sure the authorize method of the request always returns true — otherwise none of our form submissions will be authorized). A quick update to the typehint for the $request variable (and the corresponding @param in the doc block) and that part’s good to go.
Now, to deal with saving the data. There’s ten lines of steamy code just begging to be put out of their misery. Let’s oblige.
Bam! All that ugliness reduced to a clean one-liner — that never gets old. Any attribute we need to store will get passed into the create method by $request->(), and if we happened to not pass a parameter in, Model::create() ignores it.
However, we have to be careful that we aren’t passing in any parameters which don’t exist on the model; Eloquent doesn’t like that.
Coming off the one-liner high, let’s deal with the flash message. There’s a better way to write that:
We no longer call flash() statically off the Session facade — we’re using the session() helper, instead. We’re also using a localization string for our message and passing in the name of our album. This will result in the same flash message as before, but with the message string in the app language file, which will not only make it easier to find and change the string in the future, should we want to do so, it will allow us to reuse the string in other locations of the application.
The final result for the store method:
Whew! Just extracting the validation and squashing the model creation makes this method much better already! Combined with the session flash improvements, we’ve done good work here!
On with the show (method):
Another simple method. The main things that we need to change here are adding dependency injection (and subsequently removing the find() call), adding eager loading, and adding some spacing to improve readability. Make it so!
Quick and easy. Time for the edit method!
The changes we need to make here are nearly identical to the ones we made for the create method; the only differences are the additions of eager loading and dependency injection. Let’s do it!
It’s a similar story for the update method; we just need to add dependency injection and the changes from the create method. (I’ll spare you the torture of looking at all that bloated code from the old method — if you really feel like seeing it, it’s in the link to the old album controller at the beginning.)
Here’s the result of making those changes:
At last, we’ve come to the final method — the delete method. Here’s the old code:
Not much needed here — just add dependency injection, reduce the deletion to a simple $album->delete(), update how we’re setting the flash message, and reformat for readability. Easy peasy!
We’re almost done! There’s just one thing left to take care of — the use statements before our class declaration.
The obvious one is the newline between Illuminate\Http\Request and the rest of the use statements. We’ve also refactored all of our session calls to use the session helper, so we no longer need to include Session directly. The as statements are redundant, so we can get rid of them. Finally, we’ll add a newline between the opening tag and the namespace declaration.
Whew! This took a fair bit of work, but the end result is not only a lot easier to read, it more efficient and it’s compliant with PSR-2. You can look at the final result here.
Feel free to look over the repo to see what other changes I made!