Loading, please wait...

picture of me
Joe Rushton Fullstack Web Developer

Eliminating Smell From Your Codebase (#1)

24th January 2019 Reading Time: 10 mins
Tags: PHP Code Refactoring

I've recently acquired a passion for writing beautiful code - not just in a visual sense, but also in terms of readibility, extensibility, performance and overall well-roundedness. My intentions are to share the secret sauce via a series of [insert optimistic frequency] blog posts. I aim to keep them short, concise and easy to absorb so lets get to it.

 

Always Be Explicit

The first thing I'm going to talk about is the concept of making the implicit explicit. This may be one of the simplest refactors that you can start using in your day to day code for noticable results. Looking past the fancy phrasing, all it really means is to give meaningful names to things. What things?

Basically everything and anything in your codebase that might make someone (including yourself) have to think just a little. We want to reduce thinking time as much as possible and have the code do all the thinking for you.

 

Example 1: Compound Conditionals

Here are some trivial examples but hopefully they portray the message. Take the following conditional:

if ($order->status == 3 && $order->paid === true) {
	//..
}

It's likely you'll have a multitude of compound conditionals like the above to represent some sort of state of a domain object. In the above example, you might be able to guess that we are checking if an order is complete - but we are by no means at an optimal level of clarity here.

 

The first smell is the magic number 3 which means nothing to anyone. This would be much better defined as a constant somewhere in your system:

class Order
{
    const COMPLETED = 3;
}

if ($order->status == Order::COMPLETED && $order->paid === true) {

}

Using a sensibly named constant, static class property or configuration value will remove any element of doubt. The next step would be to extract the conditional into a simple expressive method, which, in this case can happily sit within the Order class as well:

class Order
{
	const COMPLETED = 3;

	public function isComplete()
	{
		return static::COMPLETED && $this->paid === true;
	}
}

if ($order->isComplete()) {
	//
}

 

Example 2: Simple Operations

I'm using the word 'simple' here because you're probably sensible enough to already be encapsulating most of your application logic into a series of functions or methods with readable names - however, often this process is overlooked when it comes to simple operations. Take the following example:

$path = Storage::putFile('avatars', $request->file('photo'));
$user->update(['profile_picture' => $path]);

Here we are uploading a file and then updating a field on the user model with the new filepath. It's 2 lines that you could probably get away with leaving to linger in your controllers... but things like this add up, especially if it's just dumped within a controller action alongside a sea of other "oh it's fine to leave here, it's just a couple of lines".

class User
{
	public function changeProfile($file)
	{
		$path = Storage::upload($file);
		$this->update(['profile_picture' => $path]);

		return $path;
	}
}

$user->changeProfile($request->file('photo'));

The outside-in view is much cleaner - just a simple changeProfile call directly on the user passing a reference to the file.

 

Example 3: Database Queries

Another place where you can apply this refactor is one-off, long or arbritary database queries that may not sit nicely within one of your existing models. The first example that comes to mind is reports - which usually involve querying multiple models with a series of joins and aggregate functions and maybe some post-query processing.

 

The following code is a simplified example of a real report query that fetches the number of sales for a given product set within a specified month. It uses a few aggregate functions in the SELECT clause to support adding n amount of product sets within the one query.

class ProductSetSalesReport
{
    public static function get($name, $month, $year)
    {
        return OrderItem::query()
            ->selectRaw(sprintf('COUNT(IF(product_set_types.name = "%s", 1, 0)) * AVG(IF(product_set_types.name = "%s", order_items.quantity, 0))', $name, $name)
            ->join('order_item_data', function ($join) {
                $join->on('order_item_data.order_item_id', '=', 'order_items.id');
            })
            ->join('product_set_types', function ($join) {
                $join->on('product_set_types.id', '=', 'order_item_data.item_data')
                    ->on('order_item_data.name', '=', 'product_set_type_id');
            })
            ->where(MONTH(order_items.created), $month)
            ->where(YEAR(order_items.created), $year);
    }
}

The first place I thought to put this code was on the OrderItem model, which was okay if this is where the reporting ended - however this query quickly grew to support multiple products that were filterable by category and various other conditions.

 

The next refactoring involved moving it over to a generic `Report` class which worked until we introduced profit reports, complete/incomplete sales, product addon sales etc. The Report model at this point became far too bloated, hard to read and understand.. The main queries were exposed as public methods where each had their own private helpers for both building the query and post-processing:

class Report
{
    public function productSetSales()
    {
        // The main query goes here, which uses a handful of private helper methods
    }

    private function productSetSalesFields() { }
    private function productSetSalesJoins() { }
    private function productSetSalesConditions() { }
    private function processProductSetSalesReport() { }

    // Rinse and repeat for each of the different reports
    // We quickly have a class that has got out of hand.
}

Enter dedicated query objects. There's absolutely no reason to cram it all into one Report class. We can still achieve the same logical grouping by delegating to other classes within our report model!

class Report
{
    public function productSetSales(...$args)
    {
        return \Reports\Queries\ProductSetSales::get($args);
    }

    public function addonSales(...$args)
    {
        return \Reports\Queries\AddonSales::get($args);
    }

    public function profitBetween(...$args)
    {
        return \Reports\Queries\ProfitBetween::get($args);
    }
}

With this approach, all of those query specific private helper functions can sit inside their respective query objects. If you need to tweak a report, you just hit up the reports directory and find the class with a name that resembles the name of your JIRA ticket.

 


For realtime dev tips, checkout dailywebdev.co.uk - a side project of mine which shares a brand new handcrafted programming tip via twitter, email and the website every single day.

Back to Homepage

Find me on social media