phpinsights
phpinsights copied to clipboard
Having `classes` with more than * cyclomatic complexity is prohibited. How to reduce?
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.
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 ;)
'config' => [
CyclomaticComplexityIsHigh::class=>[
'maxComplexity' => 8,
],
//....
],
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.