1
\$\begingroup\$

Is this the best way of doing this? I wanted to make sure the grammar is correct when the array is output.

1 car, 2 cars.

The output is correct but is there an easier way generally? I just want to make sure I am doing this in a proper way. I don't have the benefit of the experience of the majority of you guys so I am asking for general guidance. Or is it a case of "if the code works then that's all that matters".

<?php
//generates a random number between 1 -10.
function randomnumber(){
    $randomnumber = mt_rand(1, 10);
    return $randomnumber;
}

//Creates array.
//Checks to see if the random number is greater than 1
//Adds an 's' to the entry if greater than one
for ($i = 0; $i < 10; $i++){
        $rannum = randomnumber();
        if ($rannum > 1){
            $adds = "s";
        }else{
            $adds = "";
        }
        $products[$i] = $rannum." car".$adds;
}

//Outputs the final array.
foreach ($products as $current){
    echo $current."<br />";
}
?>
\$\endgroup\$
  • \$\begingroup\$ I've provided about 4 possible alternatives, but they're all geared towards "optimization", sort of. You might get the impression I find your code terrible (which it isn't), but it's kind of hard to know what you mean by an easier way. Personally, I'd ditch the second loop and use implode, because I find it easier. Some old-school C developers might find my for-loop using 2 variables the easier alternative... could you take a look, perhaps ask some follow-ups or expand on what easyness means to you? \$\endgroup\$ – Elias Van Ootegem Jul 29 '13 at 11:34
7
\$\begingroup\$

Just a little detail, to kick off: you're not declaring $products as an array anywhere. PHP will create a new variable, and assign it an empty array for you, true enough, but if you change your ini settings to E_STRICT | E_ALL, you'll notice that it doesn't do this without complaining about it.
It's always better to declare and initialize your variables beforehand. writing $products = array(); isn't hard, nor is it very costly. At any rate it's less costly than producing a notice.

Anyway, about your actual code: There is room to make your code more efficient way, so let's look at it piecce by piece:

function randomnumber(){
    $randomnumber = mt_rand(1, 10);
    return $randomnumber;
}

This function is just calling mt_rand, and returning the result. But only after the return value of mt_rand has been assigned to a local variable. That's just wasteful. Look at it from a machine's standpoint:

randomnumber();//call <name:randomnumber>
  \\
   \==> lookup function in memory, goto, and start
   $randomnumber = mt_rand(1,10);
     \\                   \\
      \=> allocate memory  \=> lookup function, pass 2 arguments & call
        ||     /\                                                 //
        ||     ||=======then, assign return valueof: =============/
        ||
        \== return COPY of assigned value
            then flag $randomnumber for GC (Garbage Collection)

That's just a hell of a lot of work, for a simple call to mt_rand. You can easily omit the allocation and garbage collection steps by changing the randomnumber function to this:

function randomnumber()
{
    return mt_rand(1,10);
}

This does exactly the same thing, without the overhead of an assignment. Still, a function call that just calls another function is a bit silly. If anything, it's an alias to an existing function, with default params. But you're still returning a copy of the return value of the core function (mt_rand). Just ditch the function, and in-line mt_rand(1,10);
If you insist, keep the alias, but define default arguments:

function randomnumber($from = 1, $to = 10)
{
    $from = (int) $from;//make sure we're dealing with ints
    $to = (int) $to;
    if ($from > $to)
    {//check order of params
        $tmp = $from;
        $from = $to;
        $to = $tmp;
    }
    return mt_rand($from, $to);
}
randomnumber();//returns mt_rand(1,10)
randomnumber(10,20);//returns mt_rand(10,20)
randomnumber(200);//returns mt_rand(10, 200)
randomnumber(null, 5);//returns mt_rand(0,5)

But that's only of any use if you're going to use this alias function throughout, and don't want to bother passing those 2 arguments all the time, or (in case of passign variables) you're not sure what they might contain (null, 'a string', (float) '0.123',...).
But we're drifting off topic. Next, the array-constructing loop:

//Creates array.
//Checks to see if the random number is greater than 1
//Adds an 's' to the entry if greater than one
for ($i = 0; $i < 10; $i++){
        $rannum = randomnumber();
        if ($rannum > 1){
            $adds = "s";
        }else{
            $adds = "";
        }
        $products[$i] = $rannum." car".$adds;
}

This isn't too bad, really, just remove the function-call, and you're good, and perhaps shorten the if-else to a ternary or a single if-statement. you could argue that assigning the random number to a variable isn't really necessairy. You're using $i as an array-key, so you could assign the value to the array directly, and check the value there

for($i=0;$i<10;$i++)
{
    $products[$i] = mt_rand(1,10);//$i is the key, so we don't need any other vars
    $products[$i] .= 'car '.($products[$i] > 1 ? 's' : '');
}
//without ternary:
for($i=0;$i<10;$i++)
{
    $products[$i] = mt_rand(1,10).' car';//just concat car already
    if ($products[$i] > 1)
    {//comparing to int will cast $products[$i] to int
     //since the string starts with an int, it'll compare that int
     //'4 car' > 1 ~~> 4 > 1 ~~> true ==> '4 car' .= s ===> 4 cars
        $products[$i] .= 's';
    }
}

Then, the output loop:

//Outputs the final array.
foreach ($products as $current){
    echo $current."<br />";
}

Ok, that's perfectly valid code, too, but wouldn't it be a lot shorter just writing this:

echo implode('<br/>', $products);

This turns an array into a string, and separates all values with a <br/>. To add another break at the end (and/or beginning) of this string:

echo '<br/>', implode('<br/>', $products), '<br/>';

I use this all the time if I want to dump a quick <ul> to the screen:

echo '<ul><li>', implode('</li><li>', $array), '</li></ul>';

So, your code ends up looking like:

for($i=0;$i<10;$i++)
{
    $products[$i] = mt_rand(1,10);//$i is the key, so we don't need any other vars
    $products[$i] .= 'car '.($products[$i] > 1 ? 's' : '')
}
echo implode('<br/>', $products);

Or, if regexes don't scare you and you like some unmaintainable code, you could even do this:

$arr = array();
$i=0;
while($i++<10)
{
    $arr[] = mt_rand(1,10);
}
echo preg_replace('/(([2-9]+|10)\s+car)(?!s)/','$1s',implode(' car<br/>',$arr).' car');

But that's just terrible code to maintain...

If the array you're echo-ing isn't of any use to you except for your printing it out, you could just as well drop the array:

for($i=0, $j=mt_rand(1,10);$i<10;$i++, $j=mt_rand(1,10))
{
    echo $j, ' car', $j > 1 ? 's' : '', '<br/>';
}

Which, if you're a massochist, you can turn into a one-liner quite easily:

for($i=0, $j=mt_rand(1,10);$i<10;$i++, $j=mt_rand(1,10))  echo $j, ' car', $j > 1 ? 's' : '', '<br/>';

As you can see, I'm using $i to count the number of iterations, while at the same time $j is (re-)assigned a new random value upon each iteration, too.
Using multiple variables in a for-loop construct is most commonly done to avoid calling count too much when for-looping a numerically indexed array:

for($i=0, $j = count($array); $i<$j;$i++)
{
    var_dump($array[$i]);
}

instead of:

for($i=0;$i<count($array);$i++)
{
    var_dump($array[$i]);
}

Both of the loops above behave in the same way (provided $array doesn't change inside the loop), but the first one is more performant, because, as you probably know, after each iteration the conditional expression (ie $i<$j or $i<count($array)) is evaluated. In the second case, this means the length of $array will be counted time and time again, whereas in the first case, this is only done once.

\$\endgroup\$
  • \$\begingroup\$ Wow. Now that is a response. Many thanks for the big help. I do feel more comfortable declaring variables at the start. Would it be good idea to set E_STRICT | E_ALL in the ini anyway? I'm using a localhost environment. If that is unforgiving then it should encourage me to write very clean code. This answered a lot of questions for me and gave me the answers I was looking for. Many thanks. \$\endgroup\$ – Gav Cheadle Jul 29 '13 at 19:04
  • \$\begingroup\$ It's always a good idea to set E_STRICT | E_ALL in the ini, especially when writing new code. Notices, warnings and errors are there for a reason: to help you write better code. Ignoring them is like ignoring good advice. In a production environment, though, those same errors and warnings can provide clients with bad intentions with information about your code, which they might exploit compromise your site, that's why you can disable displaying them. On a localhost system, though: never supress them \$\endgroup\$ – Elias Van Ootegem Jul 30 '13 at 7:03
1
\$\begingroup\$

I also lack a bit of experiance. Often it is better to use classes. My probosal would be:

I did not test it.

<?php
class products {// or wathever name you like

    public $products = array();

    protected function generate_random_number(){
        return mt_rand(1,10);
    }   

    public function __construct(){
        for($i=0;$i<10;$i++){
            $ran = $this->generate_random_number();
            $this->products[$i] = $ran.' car'.(($ran>1)?'s':'');
        }
    }

    public function render(){
        return implode('<br />',$this->products);   
    }
}

$cars = new products();
echo $cars->render();
\$\endgroup\$
  • \$\begingroup\$ Many thanks for your response. I'm not quite onto classes yet but I do recognise them from when I learnt a bit of Java. The format seems the same. I see what you're getting at though and I can see how it would make sense. \$\endgroup\$ – Gav Cheadle Jul 29 '13 at 18:55
  • \$\begingroup\$ This code does violate the first of the SOLID principles Single responsibility principle, aswell as a few conventions (ie class names start with an UpperCase letter). But the main problem is that this class is both populating an array with data, and rendering the output. \$\endgroup\$ – Elias Van Ootegem Jul 30 '13 at 7:19
  • \$\begingroup\$ PHP doesn't have naming conventions. A separate class who's only method is render, which is just a wrapper over implode is a bit too over complicating, i think. \$\endgroup\$ – AlucardTheRipper Jul 30 '13 at 12:40
  • 1
    \$\begingroup\$ @AlucardTheRipper: Officially, PHP doesn't have standards, but there's a proposal in the making, and all major parties do pay notice to it, including ZendFW, Zend being the company behind PHP means these conventions aren't going away any time soon. Wrapping the render function is, indeed overkill, but then an object that does all the work in its constructor, and is then waiting for you to call the render method isn't quite efficient, either. \$\endgroup\$ – Elias Van Ootegem Jul 30 '13 at 14:23
  • \$\begingroup\$ If you go for OO code, then you might as well do it right. OOP is almost certainly going to be less efficient, but it'll almost certainly be easier to maintain and co-author, so you might aswell follow the SOLID principles from the get-go. anyway, If you want to avoid overkill: check the one-liner in my answer. It's horrible, but you find a faster way to do what the OP wanted in PHP \$\endgroup\$ – Elias Van Ootegem Jul 30 '13 at 14:25

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.