Having duplicate code in a method for expiration date dropdown

[eldan88]

Hey Guys. I have created a drop down for a credit card expiration form drop down. As you can see below most of the code (html part) is duplicate. The most fundamental difference is the values for the for statement and the logic for the year for loop. I was wondering if I should leave it as is or create some sort of logic to not to make it repeat. I wasn't too sure on going through that because I might make it more complicated. What do you guys think? Please let me know! Thanks

public function expiration_date() {
  

// Credit Card Month Drop Down
   $option_select = "<select>";
for($month = 1; $month <= 12; $month++ ) {  
$option_select .= "<option value=\"{$month}\">{$month}</option>";
} $option_select .= "</select>";


// Credit Card Year Drop Down
  $this_year = date("y");
  $option_select .= "<select>";
  for($year = $this_year; $year <= ($this_year + 15); $year++ ) {  


   $option_select .="<option value=\"{$year}\">{$year}</option>";
   }// end of for loop
   $option_select .= "</select>";

return $option_select;

[Rifts]

it's already a pretty small function I would be happy leaving it like that.

[eldan88]

I agree. Thank Rifts!

[Psycho]

Never duplicate code - I don't care how compact it is. The primary reason is maintainability. If you ever need to make a change to how the duplicative functionality works, you have to go find all instances and update them. Or, if a bug is found you only have one place to fix the bug for all places where it would be. Otherwise, you may only fix it in the one place the bug was discovered and it would exist elsewhere. In this case, more code is a good thing.

 

As to this specific example, I typically create a function specifically for creating select options that I use for all the select fields on a site. For example, I see a flaw with what you have above in that if there is an error in the form you should reload the form and make the previously selected values 'sticky'. There is no logic in the above to accomplish that. Also, I would not put the actual <select> tags in the function. Instead, those should be included in the presentation code (i.e. HTML). Make the code reusable!

 

Example:

<?php

public function createOptions($optionList, $selectedValue=false)
{
    $options = '';
    foreach($optionList as $value => $label)
    {
        $selected = ($selectedValue==$value) ? ' selected="selected"' : '';
        $options = "<option value='{$value}'{$selected}>{$label}</option>\n";;
    }
    return $options;
}

public function month_options($selectedMonth=false)
{
    //Create a list of select options for month
    $months = array();
    for($month = 1; $month <= 12; $month++ )
    {
        $months[$month] = date("M", mktime(0, 0, 0, $month, 1, 2000));
    }
    return createOptions($months, ($selectedMonth);
}

public function year_options($selectedYear=false, $count)
{
    //Create a list of select options for year
    $years = array();
    $this_year = date("y");
    for($i = 0; $i <= $count; $i++)
    {
        $years[$this_year + $i] = $this_year + $i;
    }
    return createOptions($years, $selectedYear);
}

?>

<select name="exp_month">
  <?php echo month_options($_POST['exp_month']); ?>
</select>

<select name="exp_year">
  <?php echo year_options($_POST['exp_month'], 15); ?>
</select>

Edited March 27, 2014 by Psycho

[eldan88]

Hey Pshyco. Thanks a lot for showing an example. I have tried to go through the code you posted, but I am having a hard time understanding it. Just wanted to ask you a couple questions and see if you can help me better understand it.

 

1. In the code below what was for reason for assigning a date() function to the $months[$month] array?
   

    for($month = 1; $month <= 12; $month++ )
       {
           $months[$month] = date("M", mktime(0, 0, 0, $month, 1, 2000));
       }
   

2. Same question with the year... Why are you preforming and addition $years[$this_year + $i] and then assigning the same addition to the array?? 

   for($i = 0; $i <= $count; $i++)
       {$years[$this_year + $i] = $this_year + $i;// I don't understand this?
   
   
       }
   
   

3. How would I go about using the createOptions function with the last 2 functions???

Thanks!!

[Psycho]

1. In the code below what was for reason for assigning a date() function to the $months[$month] array?

That is so the "label" of the options will be "Jan", "Feb", "Mar", etc. but the values for the options will still be 1, 2, 3, . . .

 

 

2. Same question with the year... Why are you preforming and addition $years[$this_year + $i] and then assigning the same addition to the array?? 

Because the function to create options uses the INDEX of the array as the value and the VALUE of the array as the label. The function can be used to create options for any select list. Say you have a list of users in your database, you would use the UserID as the value and the User's name as the text/label.

 

3. How would I go about using the createOptions function with the last 2 functions???

Seriously? I don't think you looked at the sample code very well. Those two functions will call the createOptions() function as I showed. I also showed how you would actually implement the functions in the HTML code.

[eldan88]

Psyhco. Thanks for the clarification. I will go through this and try to get  abetter understanding!