2
\$\begingroup\$

I have a catalogue of activities that are marked based on the age group that they are relevant to. There are 14 checkboxes for checking what grades. K, 1 - 12, and Adult. I have a switch statement to change the K and Adult to a numeric representation.

The following PHP code grabs the grades checked and displays them in consecutive groups. Aka. K, 1, 2, 3, 5, 6, 7, 10, 11, Adult will display as K - 3, 5 - 7, 10 - Adult.

What I would like to ask is, is this a clumsy solution for my problem? Can I learn to make this more efficient or less prone to issues?

// generated through database, but set manually here for testing
$new_grades_array = array(K, 1, 2, 3, 5, 6, 7, 10, 11, Adult)

// grades related variables 
$last = ""; // records previous grade in loop for checking consecutiveness
$display = ""; // concatenated display of results
$conseq = FALSE; // records if previous grade was consecutive

for ($i = 0; $i < count($new_grades_array); $i++):

     // set grade digit for K and Adult
     switch ($new_grades_array[$i]):
           case "K":
               $currentgrade_dig = 0;
               break;
           case "Adult":
               $currentgrade_dig = 13;
               break;
           default:
               $currentgrade_dig = $new_grades_array[$i];
     endswitch;

     // concatenates string to display variable based on situation
     if ($i == 0): // if the first grade listed

          $display = $new_grades_array[$i];

     elseif ($i +1 == count($new_grades_array)): //if the last grade listed

          if ($conseq != FALSE): 
               $display .= " - " . $last . ", " . $new_grades_array[$i]; 
          else: 
               $display .= ", " . $new_grades_array[$i];
          endif;

         $conseq = FALSE;

     elseif ($currentgrade_dig - $last ==1): // if consecutive number from previous
          $conseq = TRUE;

     else: // if not a consecutive number

          if ($conseq != FALSE): 
               $display .= " - " . $last . ", " . $new_grades_array[$i];
               $conseq = FALSE;
          else:
               $display .= ", " . $new_grades_array[$i];
          endif;
     endif;

     $last = $new_grades_array[$i];

endfor;

print $display;
\$\endgroup\$
4
  • 1
    \$\begingroup\$ You could replace the switch statement with an lookup array. And instead of the for-loop, you can use an foreach-loop \$\endgroup\$
    – Philipp
    Sep 28, 2012 at 20:45
  • 3
    \$\begingroup\$ Is there any particular reason why you're not using { and }? \$\endgroup\$
    – Jan Kuboschek
    Sep 28, 2012 at 22:26
  • \$\begingroup\$ The BEST reason, I think, is because it is in a template and I believe it is somewhat standard in templating? I was working inside of a template someone else had created and they were using the alternative syntax. I actually prefer the alternative syntax because I think it makes my code easier for me to read. Since then, I've read that it is an old syntax that it is being used less. So I am not sure if I will continue when coding in general. \$\endgroup\$ Oct 10, 2012 at 22:11
  • 1
    \$\begingroup\$ Using alternate syntax in php code (where you're not jumping in and out of php/html) makes it harder to read because blocks are not immediately identifiable. \$\endgroup\$
    – AD7six
    Oct 30, 2012 at 11:00

2 Answers 2

1
\$\begingroup\$

Here are some tips.

1) Have a function that returns the grade value.

This helps avoid duplicated code and increases readablilty.

Code:

function getGradeValue($val){
  switch ($val) {
  case "K":
    $val = 0;
    break;
  case "Adult":
    $val = 13;
    break;
  default:
  }
  return $val;
}

2) Create a function to retrieve the final output.

This allow for an easier way to test the overall functionality and helps to eliminate global variables.

3) Create unit tests to speed up testing

Simple Testcases:

function println( $str ){
  print( $str . "<br/>\n" );
}
function testThis( $arr, $expect ){
  $result = getGroupedGradesAsString( $arr );
  if( $result == $expect ){
    println( "Passed:" );
  }else{
    println( "Fail:" . $result . " != " . $expect );
  }
}
testThis( array("K", 1, 2, 3, 5, 6, 7, 10, 11, "Adult"), "K - 3, 5 - 7, 10 - 11, Adult" );
testThis( array("K"),  "K" );
testThis( array("K", "Adult"),  "K, Adult" );
testThis( array("K", 1, 2, 7, 11, 12), "K - 2, 7, 11 - 12" );
testThis( array( 6, 7, 8), "6 - 8" );
testThis( array("K", 2, 4, 6, 8, 10, 12), "K, 2, 4, 6, 8, 10, 12" );
testThis( array(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, "Adult"),  "1 - Adult" );

4) Use a loop to find the last consistent number.

Refer to getGroupedGradesAsString() in the final code for more information.

Final Code:

<?php
// By Larry Battle [http://bateru.com/news/]
function getGradeValue($val){
  switch ($val) {
  case "K":
    $val = 0;
    break;
  case "Adult":
    $val = 13;
    break;
  default:
  }
  return $val;
}
function getGroupedGradesAsString($arr) {
  $output = "";
  for ($i = 0, $len = count($arr); $i < $len; $i++) {
    $output .= (($i) ? ", " : "" ) . $arr[$i];
    if( getGradeValue($arr[$i]) == getGradeValue($arr[$i+1])-1 ){
      do{
        $i++;
      }while( getGradeValue($arr[$i]) == getGradeValue($arr[$i+1])-1);
      $output .= " - " . $arr[$i];
    }
  }
  return $output;
}
?>

Demo here: http://codepad.org/zi8oavlv

Here's a shorter version but might be a litter bit harder to understand.

<?php
    // By Larry Battle [http://bateru.com/news/]
function getGradeValue($val){
    return ( $val == "K" || $val == "Adult" ) ? (($val == "K") ? 0 : 13 ) : $val;
}
function getGroupedGradesAsString($arr) {
  $output = "";
  for ($i = 0, $len = count($arr); $i < $len; $i++) {
    $output .= (($i) ? ", " : "" ) . $arr[$i];
    if( getGradeValue($arr[$i]) !== getGradeValue($arr[$i+1])-1 ){
        continue;
    }
    while( getGradeValue($arr[++$i]) === getGradeValue($arr[$i+1])-1);
    $output .= " - " . $arr[$i];
  }
  return $output;
}
?>
\$\endgroup\$
1
  • \$\begingroup\$ Hi Larry! Thanks for your tips, I will use these for future programming! I am impressed at how short your getGroupedGradesAsString function is and will study that to improve my coding later. Also, my output was not completely correct, but in yours it is. I didn't catch it because I was only testing it against one array situation. I have never seen a test function to compare output with desired output, so thank you for sharing that. \$\endgroup\$ Oct 10, 2012 at 21:46
0
\$\begingroup\$

If we are speaking about conversion of some numeric value to its text representation when some very limited number of numeric values has non-numeric text representation and vice versa, then the most efficient way will be:

$display = $i == 0? "K" : ($i == 13 ? "Adult" : $i);
$i = $text == "k" ? 0 : ($text == "Adult" ? 13 : $text);
\$\endgroup\$
11
  • \$\begingroup\$ Thanks! This looks like it will be very useful. I am not familiar with the above syntax though, could you explain it or give me a term to lookup? \$\endgroup\$
    – SweetTomato
    Sep 28, 2012 at 21:06
  • \$\begingroup\$ It is a ternary operator. It exist in many modern languages. op1 ? op2 : op3 The first operand is a boolean expression. If its value true then the op2 is evaluated otherthise op3 is evaluated \$\endgroup\$
    – Serge
    Sep 28, 2012 at 21:10
  • \$\begingroup\$ @SweetTomato it's called a "ternary operator". Search for that term on this page \$\endgroup\$
    – bhamby
    Sep 28, 2012 at 21:10
  • \$\begingroup\$ php.net is a very good souce of information on php. I believe the best one. \$\endgroup\$
    – Serge
    Sep 28, 2012 at 21:11
  • \$\begingroup\$ Thank you! I used the ternary operator by changing my switch statement to: $currentgrade_dig = $new_grades_array[$i] == "K" ? 0 : $new_grades_array[$i] == "Adult" ? 13 : $new_grades_array[$i]; \$\endgroup\$
    – SweetTomato
    Sep 28, 2012 at 21:16

Your Answer

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

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