Best Practices - Writing Readable PHP tips and advice

Add breathing space

Take a look at this piece of code, what would you say is wrong with it?


// Everything's cramped together.
public function getPage($url)
{
    $page = $this->pages()->where('slug', $url)->first();
    if (! $page) {
        return null;
    }
    if ($page['private'] && ! Auth::check()) {
        return null;
    }
    return $page;
}

This code is missing — what we like to call — "breathing space". Just like reading text, grouping code in paragraphs can be helpful to improve its readability. Have a look:


public function getPage($url)
{
    $page = $this->pages()->where('slug', $url)->first();

    if (! $page) {
        return null;
    }

    if ($page['private'] && ! Auth::check()) {
        return null;
    }

    return $page;
}

Optimise readability for happy path

Here's an example where the happy path is handled first. Try to read it: you'll notice that, besides some code overhead, it's also more confusing to understand what this code does from a first read.


// core functionality comes first, special cases handled at the end
public function sendMail(User $user, Mail $mail)
{
    if ($user->hasSubscription() && $mail->isValid()) {
        $mail->send();
    }

    if (! $user->hasSubscription()) {
        // throw exception
    }

    if (! $mail->isValid()) {
        // throw exception
    }
}

So instead, let's first check and handle all edge cases followed by the happy path as the last step of the function. Conveniently, it doesn't need to be wrapped in a condition anymore if all edge cases have already been handled.


// special cases handled first, core functionality comes later
public function sendMail(User $user, Mail $mail)
{
    if (! $user->hasSubscription()) {
        // throw exception
    }

    if (! $mail->isValid()) {
        // throw exception
    }

    $mail->send();
}

Remove blocks of commented / unreachable / dead code

Everybody probably has at some point added some code that was only temporarily needed. Maybe you were trying to activate a feature or experiment with a variation. You might have commented out the code, with the sole purpose of uncommenting it should it be needed again.

However, keeping commented code around hurts readability.


$basePrice = $this->calculateBasePrice();

$tax = $this->calculateTax($basePrice);

$price = $basePrice + $tax;

/**
  * Reactivate this when we run a promo;
  * 
  * $discountPercentage = $this->getDiscountPercentage();
  * 
  * $discountAmount = $price * $discountPercentage;
  * 
  * $price = $price - $discountAmount;
*/

Another approach that people sometimes use is to put code in a statement that is never true, which makes the code unreachable.


$basePrice = $this->calculateBasePrice();

$tax = $this->calculateTax($basePrice);

$price = $basePrice + $tax;

if (false) {
    $discountPercentage = $this->getDiscountPercentage();
    $discountAmount = $price * $discountPercentage;
    $price = $price - $discountAmount;
}

If the unused code is needed, but not at all times, you could opt to uncomment it and put it behind a feature flag, so you can toggle it on when desired.


function calculatePrice()
{
    $basePrice = $this->calculateBasePrice();
    $tax = $this->calculateTax($basePrice);
    $price = $basePrice + $tax;

    if ($discountIsActive ?? false) {
        $discountPercentage = $this->getDiscountPercentage();
        $discountAmount = $price * $discountPercentage;
        $price = $price - $discountAmount;
    }

    return $price;
}

If you're not sure that the code will ever be needed again, you could delete the code. Commit the deletion with a good commit message like "Remove discount calculation". This way you can always find the code again in the history, should you need it.


$basePrice = $this->calculateBasePrice();
$tax = $this->calculateTax($basePrice);

$price = $basePrice + $tax;

A good rule of thumb is to think about comments as fundamental parts of your code: only add them where they add real value or information that's not immediately obvious from the surrounding context. If there are any other ways of achieving the same result, prefer that other approach:

Adding a feature flag
Use versioning with a clear commit message

Grouped related variables in class

Classes often have properties. The order of these properties isn't really important for the PHP interpreter, but for humans it can matter.

Here's a simple example where we are going to implement a Period class without making use of a date class such as Carbon.


// these properties are in a weird order

class Period
{
    protected int $startDay;
    protected int $startYear;
    protected int $endDay;
    protected int $startMonth;
    protected int $endYear;
    protected int $endMonth;

    // ...
}

You can improve this by putting properties in a logical order.


// these properties are in a logical order
class Period
{
    protected int $startYear;
    protected int $startMonth;
    protected int $startDay;
    protected int $endYear;
    protected int $endMonth;
    protected int $endDay;

    // ...
}

Notice that it might still be difficult to quickly scan these variables. Grouping them by adding a newline improves readability.


// these properties are in a logic order and properly grouped

class Period
{
    protected int $startYear;
    protected int $startMonth;
    protected int $startDay;

    protected int $endYear;
    protected int $endMonth;
    protected int $endDay;

    // ...
}

However, don't go overboard with this technique. If you end up with too many groups of a single property, that may hurt readability.


class Period
{
    protected int $startYear;

    protected int $startMonth;

    protected int $startDay;

    protected int $endYear;

    protected int $endMonth;

    protected int $endDay;

    // ...
}

Be consistent

Naming files consistently, will help you find them faster through a search. Also, it will be easier to look through your app's structure.

Consider this file structure. It can be confusing that not all files have the Action suffix. Is there are reason for that?


Actions/ImportYouTubeVideos.php
Actions/GetTwitchStreamsAction.php

If there is no specific reason, use the same suffix for the same kind of files. (Or vice versa - remove the suffix - just be consistent!)


Actions/ImportYouTubeStreamsAction.php
Actions/ImportTwitchStreamsAction.php

Avoid using else

As code tends to branch to different states, using else might seem inevitable. However the opposite is true. Using if, some boolean logic and "early returns" we can rewrite almost any if-else statement with only a simple if statements. This way we're avoiding a needlessly complex conditionals that would make our code harder to understand at a quick glance.

In this piece of code, it takes some brainpower to determine when each of those conditions will be reached.


if ($conditionA) {
   if ($conditionB) {
      // condition A and B passed
   }
   else {
     // condition A passed, B failed
   }
}
else {
   // condition A failed
}

Using early returns this becomes much more readable, because there aren't as many nested paths to follow. Our code has become more linear.


if (! $conditionA) {
   // condition A failed
    return;
}

if (! $conditionB) {
    // condition A passed, B failed
    return;
}

// condition A and B passed

Should you end up with to many if statement, then consider refactoring to a lookup table or structure. If the conditions themselves are becoming too complex, you could refactor them to simpler functions.

Group boolean return values

When a function has multiple paths that return a boolean value, it can be worthwhile to group the paths with the same return values together. This allows us to mentally draw a line where the function's false code path ends and the true path starts.

In the following example, we're returning false, then true and then false again.


public function someFunction()
{
   if ($this->someCondition()) {
       return false;
   }

    if ($user->hasSubscription()) {
        return false;
    }

    if ($this->anotherCondition()) {
        return true; // not the same boolean as branches above
    }

    return false;
}

In the refactored code, we made sure to always return false first and only return true at the end.


public function someFunction()
{
    if ($this->someCondition()) {
        return false;
    }

    if ($user->hasSubscription()) {
       return false;
    }   

    if (! $this->anotherCondition()) { // reverse the condition
        return false; // same boolean
    }   

    return true;
}

Order functions in a logical manner

Multiple rules and guidelines exist to help you order the methods in a class. The most common approach is to start with the __construct() method and any static methods followed by the other methods ordered by visibility (public followed by protected and finally private).

Whether you choose to follow this convention or not, it's often helpful to order the methods in order of being called.

Methods are ordered randomly


class BlogPost()
{
    public function publish(): self
    {
        $this
            ->makePublic()
            ->clearCache()
            ->sendTweet();

        return $this;
    }

    protected function sendTweet(): self
    {
        // ...
    }

    protected function makePublic(): self
    {
        // ...
    }

    protected function clearCache(): self
    {
        // ...
    }
}

Methods are order in the order that they are being used


// methods are order in the order that they are being used

class BlogPost()
{
    public function publish(): self
    {
        $this
        ->makePublic()
        ->clearCache()
        ->sendTweet();

        return $this;
    }

    protected function makePublic(): self
    {
        // ...
    }

    protected function clearCache(): self
    {
        // ...
    }

    protected function sendTweet(): self
    {
        // ...
    }
}

When an object has a certain chronological lifecycle, it might be worthwhile to order the methods according to the lifecycle too. This is a pattern that you'll often find in models and controllers.

Methods are ordered randomly


// methods are ordered randomly

class MyObject()
{
    public function delete();

    public function create();

    public function update();
}

Methods are ordered according to lifecycle


class MyObject()
{
    public function create();

    public function update();

    public function delete();
}

Refactoring complex conditionals

Code containing if statements that combine multiple conditionals using && or || can quickly become cumbersome to read and parse mentally. Add some nested conditionals on top of that and you've got yourself a spaghetti code recipe.

Readability for these if statements can be vastly increased by separating those complex conditionals to their most basic boolean form and moving any nested if statements to their own methods.

Another practical example is this piece of code, copied straight from a real legacy project.


// negations and || makes this code unreadable

if (! $this->shipping_country === 'GB' || $this->status !== "Valid") {
    return true;
}

return false;

Conditional has been split


if ($this->shipping_country === 'GB') {
    return false;
}

if ($this->status !== 'Valid') {
    return false;
}

return true;

Sometimes, the comparison itself can be hard to read especially when negated using !. In those scenarios introducing a new method might feel like it's overkill, but it will greatly improve readability.


if (! in_array($this->item->address->country, $listOfCountries)) {
    return true;
}

Extracted to a method!


if ($this->isItemCountryOutsideOfEurope()) {
    return true;
}

If you have multiple nested if blocks, you could consider refactoring it to a dedicated function with early returns.

Loads of nesting...


function foo()
{
    if ($someCondition) {
        if ($anotherCondition) {
            if ($yetAnotherCondition) {
                // do something
            }
        }
    }
}

No nesting!


// no nesting

function foo()
{
    if (! $someCondition) {
        return;
    }

    if (! $anotherCondition) {
        return;
    }

    if (! $yetAnotherCondition) {
        return;
    }

    // do something
}

Put variables close to where they are used

Variables should be introduced close to where they are being used. This way related code is grouped together, and will be easier to refactor.

Variables are far away from where they are being used


$owners = $this->getOwners()
$admins = $this->getAdmins();
$guests = $this->getGuests();

$this->process($owners);
$this->process($admins);
$this->process($guests);

Variables are close to where they are used


$owners = $this->getOwners()
$this->process($owners);

$admins = $this->getAdmins();
$this->process($admins);

$guest = $this->getGuests();
$this->process($guests);

Now imagine that support for guests users should be removed. It's easier to do so when the two related lines are grouped together. As an added bonus the git commit will look a lot cleaner with just one deletion in the diff.

Make sure boolean parameters are readable

Passing booleans to a method can be a code smell, as it is not clear what a boolean does by reading the calling code.

It is not clear how this boolean will affect behavior


$this->getPrice(true);

A common way to improve on this is by assigning the boolean to a temporary variable with a descriptive name.


$this->getPrice($includingTaxes = true);

Use ensure and guard methods

Another commonly occurring pattern in functions and methods is to check if all requirements to execute the function are met before actually executing any real logic. These requirement checks can quickly overtake the function and drown out the code and logic that's actually important.

Bad!


public function writeToDisk(Disk $disk, string $path, string $content): void
{
    if (! $disk->isConnected()) { // not writing to disk
        $disk->connect();
    }

    $directory = pathinfo($path, PATHINFO_DIRNAME); // no disk writing here

    if (! $disk->hasDirectory($directory)) { // still not writing to disk
        $this->createDirectory();
    }

    $disk->write($path, $content); // finally, we're writing to disk
}

These requirement checking code can be moved to one or more separate functions. In this case we'll call it ensureWriteable. This also helps with making it self-evident that writing to disk will only happen when the path is actually writable.

Good!


public function writeToDisk(Disk $disk, string $path, string $content): void
{
    $this->ensureWritable($disk, $path);

    $disk->write($path, $content);
}

protected function ensureWriteable(Disk $disk, string $path): void
{
    if (! $disk->isConnected()) {
        $disk->connect();
    }

    $directory = pathinfo($path, PATHINFO_DIRNAME);

    if (! $disk->hasDirectory($directory)) {
        $this->createDirectory();
    }
}

If you've just read about being expressive in naming methods, you might feel like makeSureThatDiskIsWritable is a more appropriate name. However, having the name start with ensure is better as the prefix is used by convention for these type of methods. When you come across a method that starts with the ensure prefix you can be certain that a requirement will be checked or a preparation is being made.

Another prefix that is often used for these type of methods is guard. Often these guard methods will throw an exception when something is not in order as opposed to a ensure method that might try to fix the problem.

In the following example you'll quickly see that five of the six lines are related to making sure that we're not saving an unexpected $mimeType.

Bad!


public function saveFile(string $path, string $content): void
{
    $mimeType = mime_content_type($path);

    if (! in_array($mimeType, $allowedMimeTypes)) {
        $allowedMimeTypesString = implode(', ', $allowedMimeTypes);

        $exceptionMessage = "The file `{$path}` has an invalid type `{$mimeType}`. These are the allowed mime types: `{$allowedMimeTypesString}`";

        throw new Exception($exceptionMessage);
    }

    file_put_contents($path, $content);
}

By refactoring the code to extract some of that logic to a guard method it becomes more clear what is actually happening when saveFile is called.

Good!


public function saveFile(string $path, string $content): void 
{
    $this->guardAgainstInvalidMimeType($path, $content);

    file_put_contents($path, $content);
}

protected function guardAgainstInvalidMimeType(string $path, array $allowedMimeTypes): void
{
    $mimeType = mime_content_type($path);

    if (! in_array($mimType, $allowedMimeTypes)) {
        $allowedMimeTypesString = implode(', ', $allowedMimeTypes);

        $exceptionMessage = "The file `{$path}` has an invalid type `{$mimeType}`. These are the allowed mime types: `{$allowedMimeTypesString}`";

        throw new Exception($exceptionMessage);
    }
}

By reversing the in_array condition, we can improve the guardAgainstInvalidMimeType even further. In the code above, you'll see that most of the code is indented because it lives in the if statement. By reversing the condition and returning early, we can "flatten" the code and make it easier to read.


protected function guardAgainstInvalidMimeType(string $path, array $allowedMimeTypes): void
{
    $mimeType = mime_content_type($path);

    if (in_array($mimeType, $allowedMimeTypes)) {
        return;
    }

    $allowedMimeTypesString = implode(', ', $allowedMimeTypes);

    $exceptionMessage = "The file `{$path}` has an invalid type `{$mimeType}`. These are the allowed mime types: `{$allowedMimeTypesString}`";

    throw new Exception($exceptionMessage);
}

Use custom exceptions

When things go wrong in your application, it's important to collect as much information as possible in the thrown exception. The most important piece of information being what went wrong and how to solve it. Most of this information can be contained in an expressive exception message but creating a custom, aptly named exception class is equally important. Finally, exceptions are what they say they are: an exception to the regular flow of your code. That's why, once again, it makes sense to extract them to a separate guard method.

Here's a guard method you might recognise from the previous chapter. It throws a generic Exception if an unexpected mime type is used. Let's improve on that.

Bad!


protected function guardAgainstInvalidMimeType(string $path, array $allowedMimeTypes): void
{
    $mimeType = mime_content_type($path);

    if (in_array($mimeType, $allowedMimeTypes)) {
        return;
    }

    $allowedMimeTypesString = implode(', ', $allowedMimeTypes);

    $exceptionMessage = "The file `{$path}` has an invalid type `{$mimeType}`. These are the allowed mime types: `{$allowedMimeTypesString}`";

    throw new Exception($exceptionMessage);
}

Instead of creating the exception and crafting a good exception message inline, you could opt for a custom exception class. This custom exception class can be used to build up the exception message without cluttering the original method.

Good!


class MimeTypeNotAllowed extends Exception
{
    public static function make(string $file, array $allowedMimeTypes): self
    {
        $mimeType = mime_content_type($file);

        $allowedMimeTypes = implode(', ', $allowedMimeTypes);

        return new self("File has a mime type of {$mimeType}, while only {$allowedMimeTypes} are allowed");
    }
}

With that exception class in place, you can rewrite the original code.


protected function guardAgainstInvalidMimeType(string $file, array $allowedMimeTypes)
{
    $mimeType = mime_content_type($file);

    if (in_array($mimeType, $allowedMimeTypes)){    
        return;
    }   

    throw MimeTypeNotAllowed::make($file, $allowedMimeTypes);
}

Additionally, preparing custom exception classes makes it possible to catch the MimeTypeNotAllowed exception at a higher level to implement user friendly exception handling or validation errors.

Writing Readable PHP - general tips and advice

Writing Readable PHP - general tips and advice