Worst rated PHP Operations declared by Scrutinizer

Question

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???

I'm voting to close this question as off-topic because it belongs on code review — David Brossard, May 21 '17 at 22:30
Can you expand on what might be meant by "worst rated"? Does Scrutinizer offer any help on that point? — halfer, May 21 '17 at 22:31
Scrutinizer will have an "issue list". Look at what it says about that piece of code. — tereško, May 21 '17 at 22:32
@halfer Scrutinizer says nothing, that's why I look for help :( — Juliatzin, May 21 '17 at 22:37
Ah, just found [these "worst rated" operations](https://scrutinizer-ci.com/g/dschoenbauer/exception/code-structure/develop/hot-spots). I think it means they are the worst rated according to all the other metrics it uses, not because "worst rating" is a measure in itself. In this example they're all "A" scores anyway, so there can be very little wrong with them. — halfer, May 21 '17 at 22:44
So, the solution presumably is to look at how your operations are scored specifically, which will change the worst rated classes/operations. Once the worst ones are all A or B, your code is good. — halfer, May 21 '17 at 22:45
@halfer, he's testing Laravel code ... I would be surprised if there are D and E rated stuff there .. especially if he accidentally also added the framework to the "analysed code". — tereško, May 21 '17 at 22:47
@tereško I use laravel, but I only analyse my own code ( my plugin code ) — Juliatzin, May 21 '17 at 22:48

score 1 · Accepted Answer · answered May 21 '17 at 23:16

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.

I'm building a tournament tree, so basically, if I have 5 fighters, I must add 3 empty fighters equally separated in the 8 positions. I will check all your comments! Tx for apportation — Juliatzin, May 22 '17 at 06:03

Worst rated PHP Operations declared by Scrutinizer

1 Answers1