phplaravelscrutinizer

Worst rated PHP Operations declared by Scrutinizer


I use scrutinizer to analyse my code, and I get a function declared:

Worst rated PHP Operations

This is the function:

/**
 * Insert Empty Fighters in an homogeneous way.
 *
 * @param Collection $fighters
 * @param Collection $byeGroup
 *
 * @return Collection
 */
private function insertByes(Collection $fighters, Collection $byeGroup)
{
    $bye = count($byeGroup) > 0 ? $byeGroup[0] : [];
    $sizeFighters = count($fighters);
    $sizeByeGroup = count($byeGroup);

    $frequency = $sizeByeGroup != 0
        ? (int)floor($sizeFighters / $sizeByeGroup)
        : -1;

    // Create Copy of $competitors
    $newFighters = new Collection();
    $count = 0;
    $byeCount = 0;
    foreach ($fighters as $fighter) {
        if ($frequency != -1 && $count % $frequency == 0 && $byeCount < $sizeByeGroup) {
            $newFighters->push($bye);
            $byeCount++;
        }
        $newFighters->push($fighter);
        $count++;
    }

    return $newFighters;
}

What this function is doing is trying to insert Empty Fighters in a regular / homogeneous way

But for me, this method seems quite OK, what am I not seeing?

Any better way to achieve it???


Solution

  • Misleading name (probably not picked up by Scrutinizer). At no point the actual $byeGroup collection is necessary

    private function insertByes(Collection $fighters, Collection $byeGroup)
    

    An if statement, that is only used to pull out something, that should have been a method's parameter.

        $bye = count($byeGroup) > 0 ? $byeGroup[0] : [];
        $sizeFighters = count($fighters);
        $sizeByeGroup = count($byeGroup);
    

    Another if statement that adds to complexity. Also uses weak comparison.

        $frequency = $sizeByeGroup != 0
            ? (int)floor($sizeFighters / $sizeByeGroup)
            : -1;
    
        // Create Copy of $competitors
        $newFighters = new Collection();
        $count = 0;
        $byeCount = 0;
    

    Content of this foreach should most likely go in a separate method.

        foreach ($fighters as $fighter) {
    

    And that complex condition in yet another if statement (which also contains weak comparison), should also be better in a well named private method.

            if ($frequency != -1 && $count % $frequency == 0 && $byeCount < $sizeByeGroup) {
    

    Since $bye can be an empty array, this kinda makes no sense.

                $newFighters->push($bye); 
                $byeCount++;
            }
            $newFighters->push($fighter);
            $count++;
        }
    
        return $newFighters;
    }
    

    TBH, I have no idea what this method does, and it would also be really hard to write any unit test for it.