LogicException getRandom() / getRandomKey()?

[th3l0g4n]

This is not quite what you expect having an array with just one value, compared to the build-in array methods.

[bocharsky-bw]

Yes, I think exception is more OOP approach in this case instead default error.

What do you suggest here?

[th3l0g4n]

Well, maybe the same check as it currently exists could be made, but instead of checking if the array has a size of 1 it could be checked if it has size 0. At least when using getRandom.

When calling getRandomKey with an empty array the standard php notice will be raised, which is ok in my opinion.

[bocharsky-bw]

Really, you're right here, for getRandomKey() method we should change if statement to:

        if (0 === $this->count()) {
            throw new \RangeException(sprintf(
                'The number of elements in the array "%d" should be greater than "0".', $this->count()
            ));
        }

and use a RangeException here.

Also, for getRandomKeys() method we should simplify a bit an if statement for LogicException throwing with:

        if ($number === 1) {
            throw new \LogicException(sprintf(
                'The given argument "%d" should not be equal to "1" or "%d" (the number of elements in the array).',
                $number,
                $count
            ));
        }

but we should keep $number === 1 check because we have getRandomKey in order to get single key.

P.S. I think if any built in function throws a standard php notice - we should throw an exception in our case. Notices are also bad and they mean you're doing something incorrectly, IMHO.

@th3l0g4n What do you think? And thank you that pointed this!

[th3l0g4n]

Maybe it is a good idea to use getRandomKey() and getRandomKeys() in conjunction, as first one is actually just the latter one with argument 1.

So it could look like:

public function getRandom() {
  return $this->offsetGet($this->getRandomKey());
}

public function getRandomKey() {
  return $this->getRandomKeys(1);
}

public function getRandomKeys($number) {
  $number = (int) $number;
  $count = $this->count();
  if ($number > $count) {
            throw new \LogicException(sprintf(
               // message that you cannot obtain more keys than elements in array
            ));
   }
   return array_rand($this->elements, $number);
}

[bocharsky-bw]

My only point is that getRandomKeys() should return a several items based on its name getRandomKeys. If you want to get only one key - then use getRandomKey.

Do you think it'll be not confusing when getRandomKeys will return a single value?

[th3l0g4n]

Of cause thats true, but maybe the implementation shouldn't be too restrictive. Implementing a switch everytime you need some random keys and calling either getRandomKey() or getRandomKeys() just because getRanomKeys() only accepts values greater 1 is from my POV not really developer friendly and it would actually introduce the same behavior which because this ticket was opened.

[bocharsky-bw]

OK, I agree with you. We could allow to get 1 random key with getRandomKeys() method. Lets also get rid of:

    if ($number === 1) {
            throw new \LogicException(sprintf(
                'The given argument "%d" should not be equal to "1" or "%d" (the number of elements in the array).',
                $number,
                $count
            ));
        }

Do you want to do it and open PR for this?

[th3l0g4n]

Sure, I can do it.

[bocharsky-bw]

👍

[bocharsky-bw]

Fixed in #30