0
\$\begingroup\$

I wrote a function that can be used to generate a probability distribution for an array of elements (probGen()), and another that can be used to select an element from an array with a specified probability (probSelect()). I want to optimise my code.

probGen.php

<?
    require_once("randX.php");  #"randX()" generates a random floating point number in a specified range.  
    error_reporting(E_ERROR | E_WARNING | E_PARSE);

    function probGen(array $arr, float $control = 0.01) 
/*
*   Generates a valid, random probability distribution for a given array of elements, that can be used in conjunction with "probSelect()".
*   Input: 
        $arr: An array of elements.
        $control: A value that decides how much mass is allowed to be unilaterally dumped onto one element. A high value would permit distributions where most of the mass is concentrated on one element. 
        If an invalid value is provided, the default is used.
*   Output: An associative array where the keys are the elements in the original array, and the values are their probabilities. 
*/
    {
        $control = ($control <= 1 && $control >= 0)?($control):(0.00001);   #Use the default value if an invalid number is supplied.
        static $result = [];    #Initialises $result with an empty array on first function call.
        static $max = 1;    #Initialises $max with 1 on first function call.
        foreach ($arr as $value) 
        {
            $x = randX(0, $max);    #Random probability value.
            $result[$value] = ($result[$value] + $x)??0;    #Initialise the array with 0 on first call, and on subsequent calls increment by $x to assign probability mass.
            $max -= $x;     #Ensures that the probability never sums to more than one.
        }
/*
*   After the execution of the above code, there would be some leftover probability mass. 
*   The code below adds it to a random element.
*/
        $var = array_values($arr);
        if($max <= $control)    #To limit concentration of most of the probability mass in one variable.
        {
            $result[$var[rand(0,(count($var)-1))]] += $max; #Selects a random key and adds $max to it.
            return $result;
        }
        else
            return probGen($arr, $control); 
    }
?>

probSelect.php

<?
    require_once("confirm.php");

    function probSelect(array $arr) 
/*
*   A function to select an element from an array with indicated probabilites.
*   Input: An associative array whose keys are the elements to be selected from, and whose values are the associated probabilities.
*   Output: The selected element, or "NULL" if an invalid probability distribution was supplied. 
*/
    {
        if(confirm($arr))
        {
            $var = lcg_value(); #The random float that would be used to select the element.
            $sum = 0;
            foreach ($arr as $key => $value) 
            {
                $sum += $value;
                if($var <= $sum)
                    return $key;
            }
        }
        else
        {
            print("ERROR!!! The supplied probability distribution must sum to 1. <br>");
            return null;
        }
    }
?>

Dependencies

The required functions in case they are necessary for evaluating performance:

randX.php

<?
    function randX(float $a, float $b): float   
/*
*   Generates a random number between between two real numbers (both inclusive). 
*   Input: Two floating point numbers.
*   Output: A random floating point number.
*/
    {
        $max = max($a, $b);
        $min = min($a, $b);
        $x = $max - $min;
        $y = lcg_value()*$x;    
        return ($min+$y);
    }
?>

confirm.php

<?
    function confirm(array $arr)    #Confirms that the supplied array has a valid probability distribution
    {
        return (array_sum($arr) == 1)?true:false;
    }
?>
\$\endgroup\$
2
\$\begingroup\$

A few little things:

Don't put comments in between function declarations and curly braces (or anything similar to that like classes or methods)

That's hard to read and an eye sore. Put your function comments above the function itself. Ideally using standard docblock format.

/**
* Generates a valid, random probability distribution for a given array of elements, that can be used in conjunction with "probSelect()".
*   
* @param   array    $arr        An array of elements
* @param   float    $control    A value that decides how much mass is allowed to be unilaterally dumped onto one element. A high value would permit distributions where most of the mass is concentrated on one element. 
* @return  array                An associative array where the keys are the elements in the original array, and the values are their probabilities. 
*/
function probGen(array $arr, float $control = 0.01) 
{

Use better variable names

What does $arr mean? I can guess that it is probably an array but my IDE can already tell me that. What's actually in that array? $elements would be a better name based on the function comments. A better name would be $elementsOfSomething where something accurately describe the characteristic of those elements.

Always use curly braces for control structures

Although it is perfectly valid syntax to omit curly braces when a control structure only contains one line of code. But it is a best practice to always use them as they make the code more rreadable and prevent future errors. Future you or another developer may want to add a line to a control block and introduce a hard to find bug because they didn't realize the curly braces weren't there.

if($var <= $sum)
    return $key;

should be:

if($var <= $sum) {
    return $key;
}

Use echo over print()

print() is an alias of echo but there are minor differences between the two. Although they don't come into play here, it is the PHP convention to use echo for outputting content.

Don't output content from your functions

Your functions that do work and output content but only when there is an error are inconstant and doing too much. If you have an error, let the function report that back through a special return value (like false or null) or by throwing an exception. Let the calling code worry about reporting back the error and let the function focus on doing one thing and one thing only (just like a good OOP class should be doing).

You can simply statements that check for a Boolean to return a Boolean

Your statement return (sum($arr) == 1)?true:false; is checking if a statement is true and returning true if it is. false if it is false. So you can return the result of your conditional without having to explicitly return true or false because you already have that value:

return (sum($arr) == 1); // will return true or false

An alternative randx() function

The internet seems like it is already full of example functions that will generate a random float. They tend to be similar to the one below. Is there any reason you did not choose to go this route? It has fewer function calls so it should be more performant.

function frand($min, $max, $decimals = 0) {
  $scale = pow(10, $decimals);
  return mt_rand($min * $scale, $max * $scale) / $scale;
}

echo "frand(0, 10, 2) = " . frand(0, 10, 2) . "\n";

The PHP community prefers // to # for comments

Although # is a valid syntax for a one line comment in PHP, it is common practice to use //.

\$\endgroup\$
  • \$\begingroup\$ "The PHP community prefers // to # for comments" Do you happen to know why? \$\endgroup\$ – Mast Jan 4 at 10:37
  • \$\begingroup\$ @Mast I'm 99.44% sure this came about from the PEAR standards \$\endgroup\$ – John Conde Jan 4 at 18:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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