Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

Thanks!

// 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;
share|improve this question

migrated from stackoverflow.com Sep 29 '12 at 7:56

This question came from our site for professional and enthusiast programmers.

1  
You could replace the switch statement with an lookup array. And instead of the for-loop, you can use an foreach-loop –  Philipp Sep 28 '12 at 20:45
3  
Is there any particular reason why you're not using { and }? –  Jan Kuboschek Sep 28 '12 at 22:26
    
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. –  SweetTomato Oct 10 '12 at 22:11
1  
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. –  AD7six Oct 30 '12 at 11:00
add comment

2 Answers 2

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;
}
?>
share|improve this answer
    
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. –  SweetTomato Oct 10 '12 at 21:46
add comment

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);
share|improve this answer
    
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? –  SweetTomato Sep 28 '12 at 21:06
    
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 –  Serge Sep 28 '12 at 21:10
    
@SweetTomato it's called a "ternary operator". Search for that term on this page –  bhamby Sep 28 '12 at 21:10
    
php.net is a very good souce of information on php. I believe the best one. –  Serge Sep 28 '12 at 21:11
    
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]; –  SweetTomato Sep 28 '12 at 21:16
show 6 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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