Refactoring BandTracker

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 <?php tags 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!

Laravel: Validate Fields Only When One Is Present

Sometimes, when setting up validation for Laravel forms, you need to be able to set up validation rules for two fields, but allow for one of them to be ignored if the other is filled. This is how you can set up your validation rules to make that happen:

'email' => 'nullable|required_without:login',
'login' => 'nullable|required_without:email'

You first have to set nullable to allow both fields to have null values, otherwise Laravel rejects them. After that, you set each field to require_without:[otherField]

A potential use case for this (and the one I needed it for) is when your application allows a user to login using an email, a login, or both. It turns out sometimes didn’t work at all for this because it only applies when the field in question sometimes isn’t sent, not cases where it is sent but the value is null. The solution above accounts for this.

Laravel and PHPUnit Error: Supported Ciphers

While setting up unit testing for a PHP project, I ran into this error when I first tried to run the simple example tests to prove the system was set up correctly:

RuntimeException: The only supported ciphers are AES-128-CBC and AES-256-CBC with the correct key lengths.

Searching for this error revealed that, by far, this issue most commonly happened when you had an improper app key, and the solution was simply to regenerate the key — but this was not my problem, because I had a perfectly valid app key. I even regenerated it for good measure, and still I got the error. Nothing else on the first two pages of Google provided any help for my situation, and once I started getting the foreign-language pages I knew I was on my own.

Fortunately, in this case, the solution proved to be simple. In the phpunit.xml file Laravel automatically generates, I found this in the section for setting up environment variables:

<env name="APP_ENV" value="testing"/>

Aha! It was set to look for the testing environment, aka .env.testing! Sure enough, my environment file had the application key set to, literally, the word “key”. I simply copied over my main .env file into .env.testing, ensuring the correct key value was provided, and this solved the issue.

Laravel Redirect Status Code

Laravel’s documentation doesn’t make this obvious, but when you are using a redirect you can change what the status code of the redirect is.

Route::get('some/route', function(){
    return Redirect::to("another/route", 301);
});
Route::get('some/route', function(){
    return Redirect::route("another.route", null, 301);
});

The tricky part is knowing which parameter is the status code. To check which parameter it is, refer to the Redirector Class.

Wildcards in Laravel Routes

Laravel doesn’t have a way to use wildcards in routes by default. However, you can add one in by using route parameters and regex constraints. Don’t worry: it’s super-easy to set up!

Start by picking a term to use as the wildcard (for these examples, I chose “whatever”). Insert that term as a route parameter wherever you need a wildcard. At the end of your route, chain on the where method to tell Laravel you want to parse a route parameter with a regex. The first argument is the term you’ve chosen; the second argument is .+, which will select everything in the route parameter.

Route::get('some/route/{whatever}', function(){
    // do your stuff here
})->where('whatever', '.+');

Voila! You’ve just created a route wildcard!

There are a variety of uses for this. One is if you have a bunch of old links which start with the same prefix, but don’t end in an identifiable pattern. Another is if, instead of returning a 404 error for non-existing routes, you want to take some other action (like a redirect back to the home page).

You aren’t limited to just catching whatever’s on the end of your url, either. You can stick the wildcard in the middle of the url and it will catch everything in between!

// Catch anything starting with /some/route and ending with /after

Route::get('some/route/{whatever}/after', function(){
    // do your stuff here
})->where('whatever', '.+');

There are a couple of catches you should be aware of. One is that you can’t use a wildcard and another route parameter right next to each other in the same slash block.

// This won't work.

Route::get('some/route/{param}{whatever}', function(){
    // do your stuff here
})->where('whatever', '.+');

Laravel will pick up on {param} and add everything in {whatever} as part of it. To get around this, if you are able, you can add a delimiting character in between the two.

// This works.

Route::get('some/route/{param}.{whatever}', function(){
    // do your stuff here
})->where('whatever', '.+');

Another catch happens when your wildcard parameter doesn’t always show up in the url. For instance, if you want to redirect /some/route and /some/route/, using /some/route{whatever} won’t work because Laravel is expecting something to follow /some/route (the slash is ignored). To compensate, just make the wildcard parameter optional by using a question mark.

// Use a question mark when the wildcard won't always show up in the route.

Route::get('some/route{whatever?}', function(){
    // do your stuff here
})->where('whatever', '.+');

Hope this makes your life a little bit easier!