improve code

Desbrina

currently i have the below function that works, but is quite long.
I was wondering if anyone would have done it differently, especially the if statements near the bottom, the ones starting with

if($num >= $common && $num <= $uncommon)

What they are simply doing is trying to get a possibility.
So:
To find a common you have an 80% chance
To find an uncommon you have an 10% chance
To find a rare you have a 6% chance
To find an Ultra-Rare you have a 4% chance

	$uncommon = 80;
		$rare = 90;
		$urare = 96;
		$string = '';
		if(isset($_COOKIE['calona']))
		{
			$uid = $_COOKIE['calona'];
		}
		else
		{
			$uid = 0;
		}
		$uid = 1;
		while($count < $amount)
		{
			$num = rand(1,100);
			$type = '';

		if($num >= $common && $num <= $uncommon) // Common
		{
			$type = 'Common';
		}
		elseif($num > $uncommon && $num <= $rare) // Uncomon
		{
			$type = 'Uncommon';
		}
		elseif($num > $rare && $num <= $urare) // Rare
		{
			$type = 'Rare';
		}
		elseif($num > $urare) // Ultra-Rare
		{
			$type = 'Ultra-Rare';
		}

		$sql = "SELECT c.url, c.sdesc
		FROM creatures AS c
		RIGHT JOIN user_access AS u ON u.cid = c.ID AND u.uid = '".$uid."'
		WHERE c.rarity = 'common'
		AND c.type = 'Egg'
		ORDER BY RAND( )
		LIMIT 1";
		$result = mysql_query($sql) or die(sql_error($sql));
		while($row = mysql_fetch_array($result))
		{
			$string .= '<p><img src="'.$row['url'].'" alt="'.$row['sdesc'].'" border="0"><br/>'.$row['sdesc']."</p>\n";
		}
		$count++;
	}
	return $string;
}

laserlight

Switching to the use of magic numbers for illustration, this is one way to do it:

if($num < 80)
{
    $type = 'Common';
}
elseif($num < 90)
{
    $type = 'Uncommon';
}
elseif($num < 96)
{
    $type = 'Rare';
}
else
{
    $type = 'Ultra-Rare';
}

Albos

switch ($num) {
    case ($num < 80):
        $type = "Common";
        break;
    case ($num < 90):
        $type = "Uncommon";
        break;
    case ($num < 96):
        $type = "Rare";
        break;
    default:
        $type = "Ultra Rare";
}

Not much difference from the above code but this is a way of doing it using the switch method.

laserlight

Albos wrote:
Not much difference from the above code but this is a way of doing it using the switch method.

switch ($num) should be changed to switch (true), but frankly I feel that a switch is inappropriate for the situation.

Albos

Taken from the PHP Manual: http://uk2.php.net/manual/en/control-structures.switch.php

switch

The switch statement is similar to a series of IF statements on the same expression. In many occasions, you may want to compare the same variable (or expression) with many different values, and execute a different piece of code depending on which value it equals to. This is exactly what the switch statement is for.

I am sorry but i do have to disagree with you on this one laserlight as i do not see how you would use a switch statement otherwise. 😃

laserlight

Albos wrote:
I am sorry but i do have to disagree with you on this one laserlight as i do not see how you would use a switch statement otherwise.

I say that it is inappropriate because the classical use of a switch is to match a value in the switch condition with a corresponding label whose value is a constant expression. (The restriction on a constant expression allows compilers more leeway in implementing the switch, from what I understand.) As such, using labels with non-constant expressions does not match the canonical usage of a switch while providing pretty much no benefit over an if-else chain, and thus I feel that it is a case of being unnecessarily fanciful.

Albos

This may be true, but to the OP switch is another way of comparing something against something else and is far easier way of doing it than your original way. Hope my post gave you some insight into the switch function 🙂

laserlight

Albos wrote:
is far easier way of doing it than your original way

That is false. It normally is true (because you don't need to keep repeating the variable to compare with) when the labels have constant expressions, but it is not true when the labels have non-constant expressions (because you repeat the variable to compare with in the label's expression).

Desbrina

so, is the if statements the better one to use?

laserlight

Desbrina wrote:
so, is the if statements the better one to use?

In my opinion, yes. I do not see what is there to be gained from using a switch, and I feel that a switch is not the canonical solution, and hence does not keep to the principle of least surprise.

bradgrafelman

Desbrina;10911170 wrote:
so, is the if statements the better one to use?

Beyond laserlight's (well conceived) explanation above, one might argue that it simply comes down to the programmer's choice.

Both with accomplish the task without causing errors/warnings/notices/etc., so pick whichever one seems easiest to follow to you. If you have carpal tunnel, note that laserlight's suggestion contains less characters to type. :p

Weedpacket

bradgrafelman wrote:
If you have carpal tunnel, note that laserlight's suggestion contains less characters to type.

Ah, well, if you're going to be like that,

$type = 'Ultra-Rare';
if($num < 96)
	$type = 'Rare';
if($num < 90)
	$type = 'Uncommon';
if($num < 80)
	$type = 'Common';

is equivalent and saves at least twenty-one keystrokes :p:p