3
\$\begingroup\$

I'm new to Doctrine and had a hard time deciding how to get random rows within it without creating custom DQL functions or using a native query. The solution I have come up with is below. This is within a custom repository for an Entity. I'm hoping for some constructive criticism in regards to it, however it currently is working.

class MyRepository extends EntityRepository
{

/**
 * @param int $numberOfResults
 * @return array
 */
public function getRandomResults($numberOfResults = 1)
{
    $maxID = $this->getMax();
    $count = 0;
    $randomNumberCounter = 0;
    $randomNumbers = array();
    while ($randomNumberCounter < $numberOfResults) {
        $randomNumberCounter++;
        $randomNumbers[] = $this->getRandom(0, $maxID);
    }

    $qb = $this->createQueryBuilder('r');
    while ($count < $numberOfResults) {
        $qb
            ->andWhere(
                $qb->expr()->orX(
                    // the greater than is to account for holes within the primary key
                    $qb->expr()->gte("r.id", "?".$count),
                    /* the less than is in case we have a beginning database with a large disparity
                    between starting and ending ID */
                    $qb->expr()->lte("r.id", "?".$count)
                )
            );
        $count++;
    }
    $result = $qb
        ->setParameters($randomNumbers)
        ->setMaxResults($numberOfResults)
        // ensure we have no duplicates
        ->groupBy('r.id')
        ->getQuery();

    return $result->getResult();
}


/**
 * get the maximum ID that the table has
 * @return mixed
 * TODO: Create a Cron job to set this every 6 hours
 */
public function getMax()
{
    $maxID = $this->createQueryBuilder('r')
        ->select('(MAX(r.id))')
        ->getQuery()
        ->getSingleScalarResult();
    return $maxID;
}

/**
 * @param $min
 * @param $max
 * @return int
 */
public function getRandom($min, $max)
{
    return mt_rand($min, $max);
}
}
\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! Good thing your code works, it's a prerequisite for code that's ready for a peer review ;-) \$\endgroup\$ Sep 17, 2015 at 3:04
  • \$\begingroup\$ The problem with your code is that the ID is supposed to begin from 0 and will not work if you delete some rows in the database. \$\endgroup\$
    – talibe
    Sep 26, 2016 at 22:29
  • \$\begingroup\$ @talibe, where do you see this starting from 0? I also believe the < and > filters allow for gaps. Either way, I have a more performant method that I will update this with. \$\endgroup\$ Oct 12, 2016 at 2:30
  • \$\begingroup\$ If in the table you delete some rows the database will not reorder the IDs. So if you delete the row where id=1, there will be no row corresponding for id=1 any more. Your code will try to find that row and it won't work. \$\endgroup\$
    – talibe
    Oct 18, 2016 at 16:08

1 Answer 1

2
\$\begingroup\$

Increment operator ➕➕ can be moved up after usage in while loop

In the first while loop to add numbers to $randomNumbers the value of $randomNumberCounter is incremented on a separate line:

$randomNumberCounter++;

That operation could be combined in the condition of the while loop:

while ($randomNumberCounter++ < $numberOfResults) {
    $randomNumbers[] = $this->getRandom(0, $maxID);
}

There are simpler ways to add numbers in a loop

The block to add to $randomNumbers could be simplified. Instead of using the while loop a for loop could be used:

for($randomNumberCounter = 0; $randomNumberCounter < $numberOfResults; $randomNumberCounter++) {
    $randomNumbers[] = $this->getRandom(0, $maxID);
}

A foreach loop could also be used with range() function:

foreach(range(1, $numberOfResults) as $i) {
    $randomNumbers[] = $this->getRandom(0, $maxID);
}

While it may be the least efficient, a functional approach could be used to assign the array in a single statement using array_fill() and array_map():

$randomNumbers = array_map(
    fn($number) => $this->getRandom(0, $maxID),
    array_fill(0, $numberOfResults, 0)
);

Current approach does not ensure all random numbers will be unique

Perhaps it would be better to ensure that a random number is not already in the array of random numbers before adding it to the array.

Add spaces around boolean operators for readability

Adhering to a standard is not required but it does help with readability when the style is consistent. Many PHP developers follow PHP Standards Recommendations like PSR-12. While it wasn't approved until 2019, PSR-12 replaced PSR-2 which was approved in 2012.

In PSR-12 is section 6 concerning Operators

6.2. Binary operators

All binary arithmetic, comparison, assignment, bitwise, logical, string, and type operators MUST be preceded and followed by at least one space:

if ($a === $b) {
    $foo = $bar ?? $a ?? $b;
} elseif ($a > $b) {
    $foo = $a + $b * $c;
}

Some binary operators in the code above are preceeded and followed by a space - e.g.

$maxID = $this->getMax();

However, other places do not- e.g.

$qb->expr()->gte("r.id", "?".$count),

Since double quotes are used $count could be moved into the string, since variables are parsed within double-quoted strings1 which would eliminate the need to append with .:

$qb->expr()->gte("r.id", "?$count"),
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.