phpinsights icon indicating copy to clipboard operation
phpinsights copied to clipboard

Having `classes` with more than * cyclomatic complexity is prohibited. How to reduce?

Open xxRockOnxx opened this issue 1 year ago • 3 comments

Q A
Bug report? yes?
Feature request? no
Library version 2.8.0

I have a repeating code in my codebase that I decided to extract to another class.

This class has 8 cyclomatic complexity.

class ListingService
{
    /**
     * @var Array<string>
     */
    private array $sortableColumns = [];

    private Model $model;

    private string $defaultSortBy;

    /**
     * @param Array<string> $sortableColumns
     */
    public function __construct(Model $model, array $sortableColumns = [])
    {
        $this->model = $model;
        $this->sortableColumns = $sortableColumns;
        $this->defaultSortBy = $this->getPossibleDefaultSortBy();
    }

    public function getListings(Request $request): LengthAwarePaginator
    {
        $sortBy = $request->input('sortBy');

        // Invalid sortBy is given, fallback to default.
        if (! $sortBy || ! in_array($sortBy, $this->sortableColumns)) {
            $sortBy = $this->defaultSortBy;
        }

        $direction = $request->boolean('desc', false) ? 'desc' : 'asc';
        $limit = $request->input('limit', 10);
        $page = $request->input('page', 1);

        // Less than or equal to 0 limit or page is given, fallback to 1.
        $limit = max(1, $limit);
        $page = max(1, $page);

        return $this->model
            ->orderBy($sortBy, $direction)
            ->paginate($limit, ['*'], 'page', $page);
    }

    private function getPossibleDefaultSortBy(): string
    {
        if (!$this->model->usesTimestamps() && empty($this->sortableColumns)) {
            throw new \InvalidArgumentException('You must provide at least one sortable column when the model does not use timestamps.');
        }

        if ($this->model->getCreatedAtColumn()) {
            return $this->model->getCreatedAtColumn();
        }

        if ($this->model->getUpdatedAtColumn()) {
            return $this->model->getUpdatedAtColumn();
        }

        return $this->sortableColumns[0];
    }
}

I'm not sure what else to do here to reduce that number.

Sure I could probably use Eloquent scopes which might help, but the question still stands.

It kinda feels like a dead-end and so I have no idea what to do.

xxRockOnxx avatar Mar 29 '23 10:03 xxRockOnxx

5 is really quite low, and as you can see this class isn't unreasonable. Adjust to 10 ?

$this->defaultSortBy = $this->getPossibleDefaultSortBy();

I'd only call this if I need it as it's only used in one place

$sortBy = $this->defaultSortBy;

would change and removing defaultSortBy property.

$sortBy = $this->getPossibleDefaultSortBy();

Here you are checking 2 conditions, if sortBy isn't false and then if it doesn't exist in the array

if (! $sortBy || ! in_array($sortBy, $this->sortableColumns)) {
            $sortBy = $this->defaultSortBy;
        }

Whilst not quite as efficient - probably has the same effect, I'm assuming here sortableColumns are strings

if (! in_array($sortBy, $this->sortableColumns, true)) {
            $sortBy = $this->defaultSortBy;
        }

You can probably remove

        if ($this->model->getUpdatedAtColumn()) {
            return $this->model->getUpdatedAtColumn();
        }

I think from what I'm reading, if the model has timestamps, then it would always match created_at first as both would exist in laravel.

Which means you can probably rewrite the code reducing the if's

    private function getPossibleDefaultSortBy(): string
    {
        if ($this->model->usesTimestamps()) {
            return $this->model->getCreatedAtColumn();
        }

        if (! empty($this->sortableColumns)) {
            return $this->sortableColumns[0];
        }

        throw new \InvalidArgumentException('You must provide at least one sortable column when the model does not use timestamps.');
    }

So final file which has less than 5

class ListingService
{
    /**
     * @var Array<string>
     */
    private array $sortableColumns = [];

    private Model $model;

    /**
     * @param Array<string> $sortableColumns
     */
    public function __construct(Model $model, array $sortableColumns = [])
    {
        $this->model = $model;
        $this->sortableColumns = $sortableColumns;
    }

    public function getListings(Request $request): LengthAwarePaginator
    {
        $sortBy = $request->input('sortBy');

        // Invalid sortBy is given, fallback to default.
        if (! in_array($sortBy, $this->sortableColumns, true)) {
            $sortBy = $this->getPossibleDefaultSortBy();
        }

        $direction = $request->boolean('desc', false) ? 'desc' : 'asc';
        $limit = $request->input('limit', 10);
        $page = $request->input('page', 1);

        // Less than or equal to 0 limit or page is given, fallback to 1.
        $limit = max(1, $limit);
        $page = max(1, $page);

        return $this->model
            ->orderBy($sortBy, $direction)
            ->paginate($limit, ['*'], 'page', $page);
    }

    private function getPossibleDefaultSortBy(): string
    {
        if ($this->model->usesTimestamps()) {
            return $this->model->getCreatedAtColumn();
        }

        if (! empty($this->sortableColumns)) {
            return $this->sortableColumns[0];
        }

        throw new \InvalidArgumentException('You must provide at least one sortable column when the model does not use timestamps.');
    }
}

I didn't write any tests to check this - hopefully your's will show any errors ;)

EPGDigital-MW avatar Apr 18 '23 13:04 EPGDigital-MW

 'config' => [
        CyclomaticComplexityIsHigh::class=>[
            'maxComplexity' => 8,
        ],
      //....
    ],

ivanomatteo avatar Jun 07 '23 09:06 ivanomatteo

I'm not sure if I really can consider this "solved"? changing the config seems to be more of a workaround unless this is really the lowest it can get.

At the time this was written, the code being written is part of the job application which has a given config we cannot change as the code will be checked via automation.

xxRockOnxx avatar Jun 07 '23 10:06 xxRockOnxx