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
Comments
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 ;) |
|
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. |
I have a repeating code in my codebase that I decided to extract to another class.
This class has 8 cyclomatic complexity.
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.
The text was updated successfully, but these errors were encountered: