1
\$\begingroup\$

I've got the following code where:

  • $urlParams is the school year from 7 to 11
  • $Comms['Overall'] is the total amount of "commendations" a student has

This code iterates through the numbers based on the school year to determine which "award" a student has got.

Is there a better way to write this code?

if($urlParams['y'] == 7){
    if((300 <= $Comms['Overall']) && ($Comms['Overall'] <= 599)){ $Award = "Bronze"; }
    elseif((600 <= $Comms['Overall']) && ($Comms['Overall'] <= 899)){ $Award = "Silver"; }
    elseif((900 <= $Comms['Overall']) && ($Comms['Overall'] <= 1199)){ $Award = "Gold"; }
    elseif((1200 <= $Comms['Overall']) && ($Comms['Overall'] <= 1499)){ $Award = "Platinum"; }
    elseif($Comms['Overall'] >= 1500){$Award = "Diamond";}
}

elseif($urlParams['y'] == 8){
    if((250 <= $Comms['Overall']) && ($Comms['Overall'] <= 549)){ $Award = "Bronze"; }
    elseif((550 <= $Comms['Overall']) && ($Comms['Overall'] <= 849)){ $Award = "Silver"; }
    elseif((850 <= $Comms['Overall']) && ($Comms['Overall'] <= 1099)){ $Award = "Gold"; }
    elseif((1100 <= $Comms['Overall']) && ($Comms['Overall'] <= 1299)){ $Award = "Platinum"; }
    elseif($Comms['Overall'] >= 1300){ $Award = "Diamond"; }  
}

elseif($urlParams['y'] == 9){
    if((200 <= $Comms['Overall']) && ($Comms['Overall'] <= 449)){ $Award = "Bronze"; }
    elseif((450 <= $Comms['Overall']) && ($Comms['Overall'] <= 699)){ $Award = "Silver"; }
    elseif((700 <= $Comms['Overall']) && ($Comms['Overall'] <= 899)){ $Award = "Gold"; }
    elseif((900 <= $Comms['Overall']) && ($Comms['Overall'] <= 1099)){ $Award = "Platinum"; }
    elseif($Comms['Overall'] >= 1100){ $Award = "Diamond"; }
}

elseif($urlParams['y'] == 10){
    if((150 <= $Comms['Overall']) && ($Comms['Overall'] <= 399)){ $Award = "Bronze"; }
    elseif((400 <= $Comms['Overall']) && ($Comms['Overall'] <= 599)){ $Award = "Silver"; }
    elseif((600 <= $Comms['Overall']) && ($Comms['Overall'] <= 799)){ $Award = "Gold"; }
    elseif((800 <= $Comms['Overall']) && ($Comms['Overall'] <= 999)){ $Award = "Platinum"; }
    elseif($Comms['Overall'] >= 1000){ $Award = "Diamond"; } 
}

elseif($urlParams['y'] == 11){
    if((150 <= $Comms['Overall']) && ($Comms['Overall'] <= 299)){ $Award = "Bronze"; }
    elseif((300 <= $Comms['Overall']) && ($Comms['Overall'] <= 499)){ $Award = "Silver"; }
    elseif((500 <= $Comms['Overall']) && ($Comms['Overall'] <= 699)){ $Award = "Gold"; }
    elseif((700 <= $Comms['Overall']) && ($Comms['Overall'] <= 849)){ $Award = "Platinum"; }
    elseif($Comms['Overall'] >= 850){ $Award = "Diamond"; }
 }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ If this code works, but just needs improvement, it's off topic here. Please post it on Code Review instead. \$\endgroup\$
    – Carcigenicate
    Commented Oct 30, 2016 at 16:57

6 Answers 6

Reset to default
5
\$\begingroup\$

You should separate these magic numbers into a new data structure. Then have some code run over it. This will be much nicer to read.

For example:

function getAward($year, $commends) {
    $commendationsNeeded = [
        7 => [
            "Bronze" => 300,
            "Silver" => 600,
            "Gold" => 900,
            "Platinum" => 1200,
            "Diamond" => 1500
        ],
        8 => [
            "Bronze" => 250,
            "Silver" => 550,
            "Gold" => 850,
            "Platinum" => 1100,
            "Diamond" => 1300
        ]
    ];
    $maxScore = 0;
    $maxAward = "None";
    foreach ($commendationsNeeded[$year] as $award => $needed) {
        if ($needed <= $commends && $maxScore < $needed) {
            $maxScore = $needed;
            $maxAward = $award;
        }
    }
    return $maxAward;
}

echo getAward(7, 700); // Prints "Silver"
\$\endgroup\$
2
\$\begingroup\$

Despite being 'off-topic' you could try along these lines.

$param = $urlParams['y'];
$score = $Comms['Overall'];


$matrix= array(
    7 => array(
        'Bronze'    =>  range(300,599),
        'Silver'    =>  range(600,899),
        'Gold'      =>  range(900,1199),
        'Platinum'  =>  range(1200,1499),
        'Diamond'   =>  range(1500,10000)
    ),
    8 => array(
        'Bronze'    =>  range(250,549),
        'Silver'    =>  range(550,849),
        'Gold'      =>  range(850,1099),
        'Platinum'  =>  range(1100,1299),
        'Diamond'   =>  range(1300,10000)
    ),
    9 => array(
        'Bronze'    =>  range(200,449),
        'Silver'    =>  range(450,699),
        'Gold'      =>  range(700,899),
        'Platinum'  =>  range(900,1099),
        'Diamond'   =>  range(1100,10000)
    ),
    10 => array(
        'Bronze'    =>  range(150,399),
        'Silver'    =>  range(400,599),
        'Gold'      =>  range(600,799),
        'Platinum'  =>  range(800,999),
        'Diamond'   =>  range(1000,10000)
    ),
    11 => array(
        'Bronze'    =>  range(150,299),
        'Silver'    =>  range(300,499),
        'Gold'      =>  range(500,699),
        'Platinum'  =>  range(700,849),
        'Diamond'   =>  range(850,10000)
    )
);

if( $param && $matrix && array_key_exists( $param, $matrix ) ){
    $arr = $matrix[ $param ];
    foreach( $arr as $award => $range ){
        if( in_array( $score, $range ) ){
            echo 'The score of '.$score.' for `$urlParams["y"]` = '.$param.' results in an award of '.$award;
        }
    }
}
\$\endgroup\$
0
\$\begingroup\$

If it performs the way you expect I would leave it. Or you could try to rethink the logic and combine statements that meet similar conditions. You could also look into php switch statements if you want to use a different approach. http://php.net/manual/en/control-structures.switch.php

\$\endgroup\$
0
\$\begingroup\$

Use switch:

switch ($urlParams['y']) {
case 7:
    if((300 <= $Comms['Overall']) && ($Comms['Overall'] <= 599)){ $Award = "Bronze"; }
    elseif((600 <= $Comms['Overall']) && ($Comms['Overall'] <= 899)){ $Award = "Silver"; }
    elseif((900 <= $Comms['Overall']) && ($Comms['Overall'] <= 1199)){ $Award = "Gold"; }
    elseif((1200 <= $Comms['Overall']) && ($Comms['Overall'] <= 1499)){ $Award = "Platinum"; }
    elseif($Comms['Overall'] >= 1500){$Award = "Diamond";}
    Break;
case 8:
    if((250 <= $Comms['Overall']) && ($Comms['Overall'] <= 549)){ $Award = "Bronze"; }
    elseif((550 <= $Comms['Overall']) && ($Comms['Overall'] <= 849)){ $Award = "Silver"; }
    elseif((850 <= $Comms['Overall']) && ($Comms['Overall'] <= 1099)){ $Award = "Gold"; }
    elseif((1100 <= $Comms['Overall']) && ($Comms['Overall'] <= 1299)){ $Award = "Platinum"; }
    elseif($Comms['Overall'] >= 1300){ $Award = "Diamond"; }
    Break;
case 9:
    if((200 <= $Comms['Overall']) && ($Comms['Overall'] <= 449)){ $Award = "Bronze"; }
    elseif((450 <= $Comms['Overall']) && ($Comms['Overall'] <= 699)){ $Award = "Silver"; }
    elseif((700 <= $Comms['Overall']) && ($Comms['Overall'] <= 899)){ $Award = "Gold"; }
    elseif((900 <= $Comms['Overall']) && ($Comms['Overall'] <= 1099)){ $Award = "Platinum"; }
    elseif($Comms['Overall'] >= 1100){ $Award = "Diamond"; }
    Break;
case 10:
    if((150 <= $Comms['Overall']) && ($Comms['Overall'] <= 399)){ $Award = "Bronze"; }
    elseif((400 <= $Comms['Overall']) && ($Comms['Overall'] <= 599)){ $Award = "Silver"; }
    elseif((600 <= $Comms['Overall']) && ($Comms['Overall'] <= 799)){ $Award = "Gold"; }
    elseif((800 <= $Comms['Overall']) && ($Comms['Overall'] <= 999)){ $Award = "Platinum"; }
    elseif($Comms['Overall'] >= 1000){ $Award = "Diamond"; }
    Break;
case 11:
    if((150 <= $Comms['Overall']) && ($Comms['Overall'] <= 299)){ $Award = "Bronze"; }
    elseif((300 <= $Comms['Overall']) && ($Comms['Overall'] <= 499)){ $Award = "Silver"; }
    elseif((500 <= $Comms['Overall']) && ($Comms['Overall'] <= 699)){ $Award = "Gold"; }
    elseif((700 <= $Comms['Overall']) && ($Comms['Overall'] <= 849)){ $Award = "Platinum"; }
    elseif($Comms['Overall'] >= 850){ $Award = "Diamond"; }
    Break;
}

More information here http://php.net/manual/en/control-structures.switch.php

\$\endgroup\$
0
\$\begingroup\$

Please check this solution, This is best I can think of now. feel free to change any logic if you get any error..

<?php
switch($urlParams['y']){

    case '7' :
    $array = array(300 , 600, 900,1200, 1500);
    $number = (int) $Comms['Overall'];
    $position  = closest($array,$number);
    $Award = getAward($position);
        break;
    case '8' :
    $array = array(250, 550,850, 1100, 1300);
    $number = (int) $Comms['Overall'];
    $position  = closest($array,$number);
    $Award = getAward($position);
        break;
    case '9' :
    $array = array(200, 450, 700, 900, 1100);
    $number = (int) $Comms['Overall'];
    $position  = closest($array,$number);
    $Award = getAward($position);
        break;
    case '10' :
    $array = array(150, 400, 600, 800, 1000);
    $number = (int) $Comms['Overall'];
    $position  = closest($array,$number);
    $Award = getAward($position);
        break;
    case '11' :
    $array = array(150, 300, 500, 700, 850);
    $number = (int) $Comms['Overall'];
    $position  = closest($array,$number);
    $Award = getAward($position);
        break;


}

function getAward($position){
    case "0" :  return "Bronze";
     break;
    case "1" : return "Silver";
     break;
    case "2" : return "Gold";
     break;
    case "3" : return "Platinum";
     break;
    case "4" : return "Diamond";
     break;
    case "5" : return "Diamond";
     break;
    case "null" : return "Diamond";
     break;
}
function closest($array, $number) {
    sort($array);
    $i=0;
    foreach ($array as $a) {
        if ($a >= $number) return $i;
        $i++;
    }
    return end($array); // or return NULL;
}
?>
\$\endgroup\$
0
\$\begingroup\$

You can do a big swich around, because it's exactly what you are doing. You can also add a temporary variable, which will make your code a lot more readable. Also you can remove some useless parenthesis in your conditions: ((cond1) && (cond2)) => (cond1 && cond2)

$Overall = $Comms['Overall'];
switch ($urlParams['y'])
{
    case 7:
        if (300 <= $Overall && $Overall <= 599) { $Award = "Bronze"; }
        elseif (600 <= $Overall && $Overall <= 899) { $Award = "Silver"; }
        elseif (900 <= $Overall && $Overall <= 1199) { $Award = "Gold"; }
        elseif (1200 <= $Overall && $Overall <= 1499) { $Award = "Platinum"; }
        elseif $Overall >= 1500 { $Award = "Diamond"; }
    break;
    case 8:
        if (250 <= $Overall && $Overall <= 549) { $Award = "Bronze"; }
        elseif (550 <= $Overall && $Overall <= 849) { $Award = "Silver"; }
        elseif (850 <= $Overall && $Overall <= 1099) { $Award = "Gold"; }
        elseif (1100 <= $Overall && $Overall <= 1299) { $Award = "Platinum"; }
        elseif $Overall >= 1300 { $Award = "Diamond"; }
    break;
    case 9:
        if (200 <= $Overall && $Overall <= 449) { $Award = "Bronze"; }
        elseif (450 <= $Overall && $Overall <= 699) { $Award = "Silver"; }
        elseif (700 <= $Overall && $Overall <= 899) { $Award = "Gold"; }
        elseif (900 <= $Overall && $Overall <= 1099) { $Award = "Platinum"; }
        elseif $Overall >= 1100 { $Award = "Diamond"; }
    break;
    case 10:
        if (150 <= $Overall && $Overall <= 399) { $Award = "Bronze"; }
        elseif (400 <= $Overall && $Overall <= 599) { $Award = "Silver"; }
        elseif (600 <= $Overall && $Overall <= 799) { $Award = "Gold"; }
        elseif (800 <= $Overall && $Overall <= 999) { $Award = "Platinum"; }
        elseif $Overall >= 1000 { $Award = "Diamond";  }
    break;
    case 11:
        if (150 <= $Overall && $Overall <= 299) { $Award = "Bronze"; }
        elseif (300 <= $Overall && $Overall <= 499) { $Award = "Silver"; }
        elseif (500 <= $Overall && $Overall <= 699) { $Award = "Gold"; }
        elseif (700 <= $Overall && $Overall <= 849) { $Award = "Platinum"; }
        elseif $Overall >= 850 { $Award = "Diamond"; }
    break;
 }
\$\endgroup\$

Your Answer

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