Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
xxRockOnxx opened this issue Mar 29, 2023 · 3 comments

Comments

@xxRockOnxx
Copy link

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.

@EPGDigital-MW
Copy link

EPGDigital-MW commented Apr 18, 2023

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 ;)

@ivanomatteo
Copy link

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

@xxRockOnxx
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants