Refactoring #7: Upping the coding style game in PHP using Rector

Continuing from the last post, we're now exploring other Rector configurations that help us improve our code.

It's funny actually, Rector is a refactoring tool, but I'd be lying if I didn't use it to learn a good practice or two. Really, they have a catalog of refactorings ready for us to consume and expand upon.

Anyway, proceeding with the SetList::CODING_STYLE configuration sets.

    $rectorConfig->sets([
        LevelSetList::UP_TO_PHP_74,
        SetList::CODE_QUALITY,
        SetList::CODING_STYLE,
    ]);

Upgrades I absolutely love

We'll start with some top-notch improvements that I think we should apply to every PHP project.

New line after statement

 class Invoice extends Model
 {
-    protected $appends = ['order_subtotal', 'total'];
-    protected $guarded = [];

+    protected $appends = ['order_subtotal', 'total'];
+
+    protected $guarded = [];

This might seem a bit intrusive, in the sense that there might be cases where you want to pack lines together, but I don't think you'll have problems with that. This refactoring only touches the code that you really want to have some breathing room for, like class methods and properties, conditionals, loops, etc. Actually here's the full list as of Aug 2022.

Separate multiple use statements

 class Delivered
 {
-    use Dispatchable, InteractsWithSockets, SerializesModels;

+    use Dispatchable;
+    use InteractsWithSockets;
+    use SerializesModels;

I love this one. Putting the use statements one under the other lets me find them more easily. Also, hidden gems, if you add/remove one of them they'll appear as additions/deletions when viewing the git diff, instead of a one-line change that will take you a second to figure out.

Notice that the rule with the new lines above does not affect the use statements, they're still packed together nicely.

Simplify quote escaping

-    return ['message' => 'You\'re already verified.'];

+    return ['message' => "You're already verified."];

Oh, I am guilty of this. It's pretty common actually to start a string with single quotes when it fits in pleasantly with neighboring strings, only to find out you need to use 'you\'re' with an awkward backslash. Same with using double quotes. What a lovely refactor.

Wrap variables in curly braces

-    return "Service $service does not exist!";

+    return "Service {$service} does not exist!";

Do I really need this refactoring? Quite some time ago PHPStorm suggested that you use the curly braces on all occasions. Lately, I'm seeing that it advocates removing unnecessary braces. My mind is split right now, but I guess I liked having them in my strings in the first place. So I'm keeping the braces, and telling PHPStorm to stop complaining for now.

Convert switch to if-else

-    switch ($event->type) {
-        case 'payment_intent.succeeded':
-            $this->handlePaymentIntentSucceeded($event);
-            break;
-        default:
-            abort(404);
-    }

+    if ($event->type == 'payment_intent.succeeded') {
+        $this->handlePaymentIntentSucceeded($event);
+    } else {
+        abort(404);
+    }

This is a perfect example of a developer thinking of far-future use cases when writing code, making things harder to read. If your switch only contains one case, you're better off with the good old if. I'd never think of doing this refactoring manually, by the way, this tool rocks.

Match catch exceptions with their types

-    } catch (Card $e) {
-        $body = $e->getJsonBody();

+    } catch (Card $card) {
+        $body = $card->getJsonBody();

At first, I had some doubts about this rule. I liked the short $e version, it was nice and clean, but I guess a proper exception name provides more context after all. Alright, alright, I'm keeping this one.

Inherited method same visibility as parent

     use RefreshDatabase;

-    public function setUp():void

+    protected function setUp():void
     {
         parent::setUp();

I didn't notice this before but I had a lot of test classes where I'd set the setUp function as public. I remember doing it once in one test, and then I've copied that method ever since 😂.

Don't do that mistake, generate your tests with php artisan make:test ExampleTest, and modify the stubs however you like with php artisan stub:publish.

Upgrades I have mixed feelings for

Static arrow functions and closures

    $schedule->command('some:dummy-command')
-        ->when(fn() => Carbon::now()->endOfWeek()->isToday());

+        ->when(static fn() => Carbon::now()->endOfWeek()->isToday());

There are these two rules, which actually occurred quite frequently, that prefix the arrow functions and closures with static whenever possible. That is, if a closure does not need the $this reference, it can be made static. I'm dropping these rules, however, since the RFC notes that I probably don't need them.

Static closures are rarely used: They're mainly used to prevent $this cycles, which make GC behavior less predictable. Most code need not concern itself with this.

You can skip rules by simply adding them to your rector configuration, like this:

    $rectorConfig->skip([
        // ...
        StaticArrowFunctionRector::class,
        StaticClosureRector::class,
    ]);

Convert post increments to pre increments

-    $count++;

+    ++$count;

This one really made me hmmm big time. Why do I need this? I check in with the issue and PR that added the rule and I find out that this is a mirror rule from PHPStan. I know there are some edge cases when developers will ++ everywhere they can, in if statements and array keys, but damn this rule made a lot of code a bit suspicious. Or I'm just not used to this and it's actually pretty common. I'm skipping it right now, maybe we cross paths again in the future (PostIncDecToPreIncDecRector).

Convert inline variables to sprintfs

-    "Order canceled with reason {$reason}."

+    sprintf('Order canceled with reason %s', $reason)

I'm not sure I understand how the sprintf version is more readable than the encapsed one. Maybe if I wanted to do some formatting, but in an existing project, the formatting has already been done in some other way. This feels a lot like C, ignoring this one as well (EncapsedStringsToSprintfRector).

We're pausing here for now. This list is of course, incomplete, it's impossible to cover all the Code Style refactorings it can do. We'll continue exploring other refactorings like dead code and early returns in another post. See ya 🏠.