Refactoring Out Bloat From A Laravel Controller Enter a brief summary for this post

Refactoring Out Bloat from a Laravel Controller

One of the joys I find with web development is refactoring. There is always the urge to get something working and quickly to show visible results, because few have patience to wait.

It's still the case with any projects of your own and I'm no different. Usually, the second iteration are the unit tests introduced along with the encapsulation of critical logic. The separation or decoupling of code and I've the perfect example for you.

The following is from a Project controller but has since been refactored out (thankfully).

$projectsQuery = Project::filterByCompany($user->company_id);

        /**
         * @note    all $rows are available unfiltered for users with a 
         *          Project leader privilege (or superior), otherwise, user only 
         *          has those $rows their team belongs to a project
         */

        if(! ($user->isProjectLeader())
        {
            $projectsQuery->filterByTeamWithUser($user->id, $user->isProjectLeader());
        }

$projectAging = $projectsQuery
            ->filterByDaysRemaining()
            ->get();
        
        $projectAgingDistribution = $this->agingDistributionService->calculateAgingPercentages(
            $projectAging->filter(function($project) 
            { 
                return $project->daysRemaining >= 0 && $project->daysRemaining <= 7; 
            }),
            $projectAging->filter(function($project) 
            { 
                return $project->daysRemaining >= 8 && $project->daysRemaining <= 14; 
            }), 
            $projectAging->filter(function($project) 
            { 
                return $project->daysRemaining >= 15 && $project->daysRemaining <= 30; 
            }), 
            $projectAging->filter(function($project) 
            { 
                return $project->daysRemaining > 30; 
            })
        );

This part is basically utilizing a service to calculate the percentage of days remaining (on each project) that are grouped by a given period, say seven days and so forth.

I threw this together rather hastily to get the charts finished on the Inertia front end and recently revisited once more for the refactoring.

Refactoring in Laravel, Removing Bad Code

I plumbed in a service directly through the controller's constructor, knowing that perhaps wasn't the wisest move. But it got things working at the time and there are always improvements to be found anyway, through iteration.

I just left it as it was at the time, to come later. Later is now and below is the refactoring I arrived at and one I'm happy with. I began with a Factory and took it from there.

namespace App\Services;

use App\Services\Factories\CompletionServiceFactory;
use App\Services\Factories\DistributionServiceFactory;

class ServiceFactory
{
	private ?CompletionServiceFactory $completionFactory = null;
	private ?DistributionServiceFactory $distributionFactory = null;

	public function __construct() {}

	public function getCompletionFactory($dataService)
	{
		if(is_null($this->completionFactory))
		{
			$this->completionFactory = new CompletionServiceFactory($dataService);
		}

		return $this->completionFactory;
	}

	public function getDistributionFactory($dataService)
	{
		if(is_null($this->distributionFactory))
		{
			$this->distributionFactory = new DistributionServiceFactory($dataService);
		}

		return $this->distributionFactory;
	}
}

There are many services required to put together the project dashboard. So rather than move the bloat from the controller to a single factory I created many factories, such as:

  • the Distribution factory
  • the Completion factory

The refactoring continues of course but for this post, suffice to say we're only interested in one service implementation.

Following this, there is the actual service factory itself. In this particular case it is the AgingDistributionService and it's created in the following way, below.

namespace App\Services\Factories;

use App\Models\User;
use App\Services\Factories\Distributions\AgingDistributionService;
use App\Services\Factories\Distributions\DurationDistributionService;
use App\Services\Factories\Distributions\PriorityDistributionService;
use App\Services\Factories\Distributions\StatusDistributionService;
use App\Services\Factories\Distributions\OverdueDistributionService;

class DistributionServiceFactory {

	private $dataService;

	public function __construct($dataService)
	{
		$this->dataService = $dataService;
	}

	public function getDurationDistributionService(User $auth)
	{
		return new DurationDistributionService($auth, $this->dataService);
	}

	public function getAgingDistribution(User $auth)
	{
		return new AgingDistributionService($auth, $this->dataService);
	}

	public function getPriorityDistributionService(User $auth)
	{
		return new PriorityDistributionService($auth, $this->dataService);
	}

	public function getStatusDistributionService(User $auth)
	{
		return new StatusDistributionService($auth, $this->dataService);
	}

	public function getOverdueDistributionService(User $auth)
	{
		return new OverdueDistributionService($auth, $this->dataService);
	}
}

Each and every service requires the auth()->guard('web')->user() and a data service (such as the ProjectDataService for example) to ensure loose coupling of code. I do believe in the "one class, one responsibility" rule in object-oriented programming.

The data service is further down the page but next up is the aging distribution service. Reflecting on what went before the refactoring and looking now at what's now put in place, the difference is startling (I feel good anyway).

namespace App\Services\Factories\Distributions;

use App\Models\User;

class AgingDistributionService
{	
	private ?User $auth;
	private $dataService;

	public function __construct(User $auth, $dataService)
	{
		$this->auth = $auth;
		$this->dataService = $dataService;
	}
	
	/**
	 * @note 	calculate a percentage of tasks for their remaining duration 
	 * 			before their end date, 0-7 days, 8-14 days, 15-30 days, 31+ days
	 */

	public function calculate() : array
	{	
		$sevenDays = $this->sevenDays();
		$fourteenDays = $this->fourteenDays();
		$thirtyDays = $this-> thirtyDays();
		$overThirtyDays = $this->overThirtyDays();

		$totalDays = ($sevenDays + $fourteenDays + $thirtyDays + $overThirtyDays);

		if($totalDays == 0) 
		{
			/**
			 * @note 	this avoids the notorious "division by zero" error
			 */
			
			return [
				'sevenDays' => 0,
				'fourteenDays' => 0,
				'thirtyDays' => 0,
				'overThirtyDays' => 0,
			];
		}

        return [
        	'sevenDays' => ($sevenDays / $totalDays) * 100,
        	'fourteenDays' => ($fourteenDays / $totalDays) * 100,
        	'thirtyDays' => ($thirtyDays / $totalDays) * 100,
        	'overThirtyDays' => ($overThirtyDays / $totalDays) * 100,
        ];
	}

	private function sevenDays() : int
	{
		$projects = $this->dataService->queryDaysRemaining($this->auth);

		return $projects->filter(function($project) 
		{ 
			return $project->daysRemaining >= 0 && $project->daysRemaining <= 7; 
		})
		->count();
	}

	private function fourteenDays() : int
	{
		$projects = $this->dataService->queryDaysRemaining($this->auth);

		return $projects->filter(function($project) 
		{ 
			return $project->daysRemaining >= 8 && $project->daysRemaining <= 14;
		})
		->count();
	}

	private function thirtyDays() : int
	{
		$projects = $this->dataService->queryDaysRemaining($this->auth);

		return $projects->filter(function($project) 
		{ 
			return $project->daysRemaining >= 15 && $project->daysRemaining <= 30; 
		})
		->count();
	}

	private function overThirtyDays() : int
	{
		$projects = $this->dataService->queryDaysRemaining($this->auth);

		return $projects->filter(function($project) 
		{ 
			return $project->daysRemaining > 30;
		})
		->count();
	}
}

Still room for improvement sure but for now it's cleaned up the controller hugely, especially with the other services being refactored too. The data service for the Project model is below. For some strange reason I can't explain why I didn't implement the Repository pattern opting instead for using scopes on the model and "this", below.

namespace App\Services\Data;

use App\Models\User;
use App\Models\Project;
use Illuminate\Contracts\Pagination\Paginator;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\Builder;

class ProjectDataService 
{
	protected ?Project $project = null; 

	public function __construct(Project $project) 
	{ 
		$this->project = $project;
	}

	public function loadIndexDashboardBeginningData(User $auth, $from, $to) : Paginator
	{
		return $this->project
			->notCompleted()
            ->filterByTeamWithUser($auth->id, $auth->isProjectLeader())
            ->filterByCompany($auth->company_id)
            ->beginning($from, $to)
            ->select([
				'id', 'name', 'completed', 'start_date as start', 'end_date as end',
			])
			->orderByDesc('updated_at')
            ->simplePaginate();
	}

	public function loadIndexDashboardConcludingData(User $auth, $from, $to) : Paginator
	{
		return $this->project
			->notCompleted()
			->filterByTeamWithUser($auth->id, $auth->isProjectLeader())
            ->filterByCompany($auth->company_id)
            ->concluding($from, $to)
            ->select([
				'id', 'name', 'completed', 'start_date as start', 'end_date as end',
			])
			->orderByDesc('updated_at')
            ->simplePaginate();
	}

	/**
	 * @note 	via the model scope, return projects that are pending, and 
	 * 			only those belonging to company of $auth
	 */

	public function pending(User $auth) : Collection
	{
		return $this->project
			->filterByCompany($auth->company_id)
			->pending()
			->get();
	}

	/**
	 * @note 	via the model scope, return projects that are already in progress, and 
	 * 			only those belonging to company of $auth
	 */

	public function inProgress(User $auth) : Collection
	{
		return $this->project
			->filterByCompany($auth->company_id)
			->inProgress()
			->get();
	}

	/**
	 * @note 	via the model scope, return projects overdue (current date is beyond 
	 * 			a project end date), and belonging to company of $auth
	 */

	public function overdue(User $auth) : Collection
	{
		return $this->project
			->filterByCompany($auth->company_id)
			->overdue()
			->get();
	}

	/**
	 * @note 	via the model scope, those projects completed, on time or otherwise, 
	 * 			for the company $auth belongs to
	 */

	public function completed(User $auth) : Collection
	{
		return $this->project
			->filterByCompany($auth->company_id)
			->completed()
			->get();
	}

	/**
	 * @note 	implement the Visitor pattern, via a Decorator, to access the rate 
	 * 			completion of a collection
	 */

	public function accept($visitor, User $auth)
	{
		return $this->project->accept($visitor, $auth);
	}

	/**
	 * @note 	return the query builder set up with filter for 
	 * 			company belonging to $auth user
	 */

	private function queryBuilder(User $auth) : Builder
	{
		return $this->project->filterByCompany($auth->company_id);
	}

	/**
	 * @note 	use the query builder, load all projects found for company 
	 * 			belonging to $auth user
	 */

	public function queryWhenNotProjectLeader(User $auth) : Collection
	{
		return $this->queryBuilder($auth)
			->when(!$auth->isProjectLeader(), function($query) use($auth) 
			{ 	
				/**
				 * @note 	filter out projects that do not belong to, or associated 
				 * 			with authenticated user, because they're not a project 
				 * 			leader or superior role
				 */

				return $query->filterByTeamWithUser($auth->id, $auth->isProjectLeader()); 
			})
			->get();
	}

	/**
	 * @note 	use the query builder, load all projects found for company 
	 * 			belonging to $auth user, but with days remaining of each proejct
	 */

	public function queryDaysRemaining(User $auth) : Collection
	{
		return $this->queryBuilder($auth)
			->filterByDaysRemaining()
            ->get();
	}
}

On the controller it runs into a few lines to get the data calculated and put over to the client.

$projectAgingDistribution = $this->getFactory()
            ->getDistributionFactory($this->dataService)
            ->getAgingDistribution($user)
            ->calculate();

A day spent refactoring I figure lifts up the mood too because you know you've took another step in the right direction with something. Improving quality of code is critical when in years to come another is to care for the application's development, and not you.

One itch I did have afterwards and it's a common distraction or error and it's this. I was considering the changing of the method names, to something a little clearer and more expressive.

It's always a bad idea really unless it can be justified because it uses up valuable time for next to nothing in return. Here is what I was thinking but didn't do.

$projectAgingDistribution = $this->getFactory()
            ->getDistributionServices($this->dataService)
            ->useAgingService($user)
            ->calculate();

Isn't hindsight wonderful? You need to be strong and not second guess or doubt yourself. As the saying goes, the grass is never greener on the other side.