Refactoring ‍‍Arr::random code

[alirezasalehizadeh]

Hello there,
I was reviewing the code of the ‍Arr::random method when I noticed an issue.
The current code of this method is as follows:

/**
     * Get one or a specified number of random values from an array.
     *
     * @param  array  $array
     * @param  int|null  $number
     * @param  bool  $preserveKeys
     * @return mixed
     *
     * @throws \InvalidArgumentException
     */
    public static function random($array, $number = null, $preserveKeys = false)
    {
        $requested = is_null($number) ? 1 : $number;

        $count = count($array);

        if ($requested > $count) {
            throw new InvalidArgumentException(
                "You requested {$requested} items, but there are only {$count} items available."
            );
        }

        if (empty($array) || (! is_null($number) && $number <= 0)) {
            return is_null($number) ? null : [];
        }

        $keys = (new Randomizer)->pickArrayKeys($array, $requested);

        if (is_null($number)) {
            return $array[$keys[0]];
        }

        $results = [];

        if ($preserveKeys) {
            foreach ($keys as $key) {
                $results[$key] = $array[$key];
            }
        } else {
            foreach ($keys as $key) {
                $results[] = $array[$key];
            }
        }

        return $results;
    }

‍‍‍The issue is, why should the value of $number be set to null? Moreover, why should we call this method without specifying how many random values we need?

This has resulted in repeatedly checking whether the value of $number is equal to null or not.(Four times)

Is there any reason other than determining whether we can retrieve the number of random values we need from the input array?

And as mentioned in the documentation of the pickArrayKeys method, the value of $number must be at least equal to 1.

So, if we set the default value of $number to 1, there will no longer be a need to check it in the first line.

And similarly, I observed in the tests of this method:

public function testRandom()
    {
        //...
        $random = Arr::random(['foo', 'bar', 'baz'], 0);
        $this->assertIsArray($random);
        $this->assertCount(0, $random);
       //...
    }

Why should we call this method and request 0 random values?

All of these have led to the existence of unnecessary ‍‍if conditions.

Therefore, I will refactor this method in the following way:

/**
     * Get array of one or a specified number of random values from an array.
     *
     * @param  array  $array
     * @param  int  $number
     * @param  bool  $preserveKeys
     * @return array
     *
     * @throws \InvalidArgumentException
     */
    public static function random($array, $number = 1, $preserveKeys = false)
    {

        $count = count($array);

        if ($number > $count) {
            throw new InvalidArgumentException(
                "You requested {$number} items, but there are only {$count} items available."
            );
        }

        $keys = (new Randomizer)->pickArrayKeys($array, $number);

        $results = [];

        if ($preserveKeys) {
            foreach ($keys as $key) {
                $results[$key] = $array[$key];
            }
        } else {
            foreach ($keys as $key) {
                $results[] = $array[$key];
            }
        }

        return $results;
    }

Here, with just one condition, we can determine that the array is not empty and we can retrieve random values equal to the ‍$number parameter.

Note: If it is important to you that when $number equals 1, the value itself is returned rather than an array, this can be handled with a single if condition.

What is your opinion?
Thank you.