Skip to content
This repository has been archived by the owner on Dec 11, 2020. It is now read-only.

allow randomElements to accept a Traversable object #1389

Merged
merged 8 commits into from
Jan 8, 2018
Merged

allow randomElements to accept a Traversable object #1389

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cff4c18
allow `randomElements` to accept a Traversable object
browner12 Dec 18, 2017
26b11af
formatting
browner12 Dec 18, 2017
0a00054
revert my accidental autoformat
browner12 Dec 18, 2017
8d2b01b
do not use short array syntax
browner12 Dec 18, 2017
e177693
use ternary to set array
browner12 Dec 18, 2017
8f337a3
use new variable
browner12 Jan 4, 2018
5762ea5
initialize `$traversables` variable
browner12 Jan 5, 2018
657df1b
use long array syntax
browner12 Jan 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/Faker/Provider/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,17 @@ public static function randomAscii()
*
* @return array New array with $count elements from $array
*/
public static function randomElements(array $array = array('a', 'b', 'c'), $count = 1, $allowDuplicates = false)
public static function randomElements($array = array('a', 'b', 'c'), $count = 1, $allowDuplicates = false)
Copy link
Owner

@fzaninotto fzaninotto Jan 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you re-add the type hint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know what you mean by this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just revert your changes to this line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole point of this PR is to accept things other than arrays, so if I add the type hint back, that negates all of the new code.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, you're right, my bad

{
$allKeys = array_keys($array);
if ($array instanceof \Traversable) {
foreach ($array as $element) {
$traversables[] = $element;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to set traversables to an empty array first I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i could, but this is why i use the isset below. it's 2 ways to accomplish the same thing.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then use only 1 intermediate variable if you want, but pushing to a non-existing array will cause a warning. If it doesn't, it's a bad practice anyway.

}
}

$arr = isset($traversables) ? $traversables : $array;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't check isset but zero length


$allKeys = array_keys($arr);
$numKeys = count($allKeys);

if (!$allowDuplicates && $numKeys < $count) {
Expand All @@ -191,7 +199,7 @@ public static function randomElements(array $array = array('a', 'b', 'c'), $coun
$keys[$num] = true;
}

$elements[] = $array[$allKeys[$num]];
$elements[] = $arr[$allKeys[$num]];
$numElements++;
}

Expand All @@ -206,7 +214,7 @@ public static function randomElements(array $array = array('a', 'b', 'c'), $coun
*/
public static function randomElement($array = array('a', 'b', 'c'))
{
if (!$array) {
if (!$array || ($array instanceof \Traversable && !count($array))) {
return null;
}
$elements = static::randomElements($array, 1);
Expand Down
16 changes: 16 additions & 0 deletions test/Faker/Provider/BaseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Faker\Provider\Base as BaseProvider;
use PHPUnit\Framework\TestCase;
use Traversable;

class BaseTest extends TestCase
{
Expand Down Expand Up @@ -134,6 +135,11 @@ public function testRandomElementReturnsNullWhenArrayEmpty()
$this->assertNull(BaseProvider::randomElement(array()));
}

public function testRandomElementReturnsNullWhenCollectionEmpty()
{
$this->assertNull(BaseProvider::randomElement(new Collection(array())));
}

public function testRandomElementReturnsElementFromArray()
{
$elements = array('23', 'e', 32, '#');
Expand All @@ -146,6 +152,12 @@ public function testRandomElementReturnsElementFromAssociativeArray()
$this->assertContains(BaseProvider::randomElement($elements), $elements);
}

public function testRandomElementReturnsElementFromCollection()
{
$collection = new Collection(array('one', 'two', 'three'));
$this->assertContains(BaseProvider::randomElement($collection), $collection);
}

public function testShuffleReturnsStringWhenPassedAStringArgument()
{
$this->assertInternalType('string', BaseProvider::shuffle('foo'));
Expand Down Expand Up @@ -554,3 +566,7 @@ public function testRandomElements()
$this->assertContainsOnly('string', $allowDuplicates);
}
}

class Collection extends \ArrayObject
{
}