slevomat / coding-standard

Slevomat Coding Standard for PHP_CodeSniffer provides many useful sniffs
MIT License
1.39k stars 171 forks source link

UnnecessaryConditionWithReturnSniff #424

Closed kukulich closed 6 years ago

kukulich commented 6 years ago
if (cond()) {
  return true;
} else {
  return false;
}
if (cond()) {
  return true;
}

return false;

should be fixed to:

return cond();
cond() ? true : false

should be fixed to

cond()
Levivb commented 6 years ago

Note: this might cause problems with type hinting.

For example:

function getSpecials(array $array): array {
    return array_filter($array, static function($item){ return $item instanceof Special; });
}

function hasSpecialItems(array $array): bool
{
    return getSpecials($array) ? true : false;
}

as it might be changed to:

function hasSpecialItems(array $array): bool
{
    // getSpecials returns an array while the typehint of this func states this function should return a boolean
    return getSpecials($array); 
}

Perhaps not the best of examples, but I hope it shows the need for a typehint check :P

And since phpcs can only check per file, whatever type the cond() function returns can often not be checked

ondrejmirtes commented 6 years ago

@Levivb I don't think you're right - in your case, both getSpecials($array) ? true : false and getSpecials($array) result type is boolean.

PHPCS is not the right tool for typehint checking, that's what tools like PHPStan are for. They can infer this better.

Levivb commented 6 years ago

Agree with the PHPStan comment.

But getSpecials actually returns an array. So return getSpecials($array) will also return an array and therefor error on the return typehint from hasSpecialItems

I've tinkered the following in artisan tinker (laravel):

>>> function getSpecials(array $array): array {
...     return array_filter($array, static function($item){ return $item instanceof Special; });
... }
>>> function hasSpecialItems(array $array): bool
... {
... return getSpecials($array);
... }
>>> hasSpecialItems([])
TypeError: Return value of hasSpecialItems() must be of the type boolean, array returned
>>>

A possible solution would be to cast to boolean:

>>> function getSpecials(array $array): array {
...     return array_filter($array, static function($item){ return $item instanceof Special; });
... }
>>>
>>> function hasSpecialItems(array $array): bool
... {
... return (bool)getSpecials($array);
... }
>>> hasSpecialItems([])
=> false
ondrejmirtes commented 6 years ago

Yeah, it's correct that the value in condition (both in if and in ternary) might be just truthy/falsey (like an empty array or an empty/non-empty string) so having this automatically fixed might be problematic. I'm so used to writing your situation like this:

return count(getSpecials($array)) > 0 ? true : false;

That this didn't occur to me.

kukulich commented 6 years ago

Implemented in https://github.com/slevomat/coding-standard/commit/d043592d4f611f788661152c59d1af8da11bce84

github-actions[bot] commented 4 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.