Page 1 of 1

Help me make this code more efficient

Posted: Mon Aug 19, 2002 5:44 pm
by ruthsimon
I'm passing variables for an event's start and end times from a form with three fields for each time (hour, minute, and am/pm). On the current page I'm validating that the start time is prior to the end time. If the time is not valid I want to display the form fields again with the selected value being the time variables passed originally (so users can see the erroneous times they entered).

Here are my POST variables:
$Start_HOUR,$Start_MIN,$Start_AMPM
$End_HOUR,$End_MIN,$End_AMPM

Since the form fields are identical for start and end times (except for the variable names), I don't want to have to list them twice, but I can't figure out how to get around it. You'll also see a few other inefficiencies around the selected MIN and AMPM.

Code: Select all

/* Use drop down lists to select the start and end times */
   	echo 
	"Start Time:<br>\n<td>";
   	/* Drop down list for the hour */ 
		for ($n=1; $n<=12; $n++)
		&#123;
		echo "<option value=$n";
		if ($n == '$Start_HOUR')
                                &#123;
		echo " selected";
		&#125;
     echo ">$n\n";
		&#125;
		echo "</SELECT>\n"; 
	/* Drop down list for the minute */ 
		echo "<select name= '$Start_MIN'>\n";
		echo "<option value='00'>00\n 
		<option value='15'";
 		if ('$Start_MIN' == 15)
                                &#123;
		echo " selected";
		&#125;
       echo ">15\n 
		<option value='30'";
 		if ('$Start_MIN' == 30)
                                &#123;
		echo " selected";
		&#125;
       echo ">30\n 
		<option value='45'";
 		if ('$Start_MIN' == 45)
                                &#123;
		echo " selected";
		&#125;
       echo ">45\n 
		</SELECT>\n"; 
	/* Drop down list for AM/PM */ 
		echo "<select name= $value.'_AMPM'>\n";
		echo "<option value='AM'";
 		if ('$Start_AMPM' == 'AM')
                                &#123;
		echo " selected";
		&#125;
       echo ">AM\n 
		<option value='PM'";
 		if ('$Start_AMPM' == 'AM')
                                &#123;
		echo " selected";
		&#125;
      echo ">PM\n
		</SELECT><p>\n"; 
	&#125;
Then I repeat all this for the End Time.
I know there's a better way. Any help would be appreciated. Ruth

Posted: Mon Aug 19, 2002 6:15 pm
by nielsene
What type of efficiency are you aiming for? Easier to read/shorter to write? Faster for the server?

I'm guessing you mean the first because of the reference to the duplication of code blocks.

In that case what I would do is
pull the whole section of code you listed into a function that takes a "prefix" variable. lets say you call the function makeTimeDropDown.

Code: Select all

function makeTimeDropDown($prefix,$min,$hour,$ampm)
&#123;
   /* Use drop down lists to select the start and end times */ 
      $result =  "$prefix Time:<br \>\n<td>"; 
      /* Drop down list for the hour */ 
      $result .= "<select name="$prefix"."_HOUR>\n";
      for ($n=1; $n<=12; $n++) 
      &#123; 
          $result .= "\t<option value="$n""; 
          if ($n == "$hour") 
          &#123;  
              $result .= " selected="selected""; 
          &#125; 
          $result .= ">$n</option>\n"; 
      &#125; 
      $result .= "</select>\n"; 
   /* Drop down list for the minute */ 
      $result .= "<select name= "$prefix"."_MIN">\n"; 
      $minChoices = array("00","15","30","45");
      $numMinChoices = count($minChoices);
      for ($i=0; $i<$numChoices; $i++)
      &#123;
           $result.= "\t<option value="&#123;$minChoices&#1111;$i]&#125;" ";
           if ($minChoices&#1111;$i]==$min)
           &#123;
                $result .= "selected="selected"";
           &#125;
           $result .= ">&#123;$minChoices&#1111;$i]&#125;</option>\n";
       &#125;
       $result .= "</select>\n";
 
   /* Drop down list for AM/PM */ 
      $result .= "<select name="$prefix"."_AMPM">\n"; 
      $result .= "\t<option value="AM"; 
      if ($ampm == 'AM') 
      &#123; 
          $result .= " selected="selected""; 
      &#125; 
      $result .= ">AM</option>\n 
      $result .= "\t<option value="PM""; 
      if ($ampm == 'PM') 
      &#123; 
          $result .=  " selected="selected""; 
      &#125; 
      $result .=  ">PM</option>\n"; 
      $result .= </select><p />\n"; 
      return $result;    
&#125;
Then you echo it out with

Code: Select all

echo makeTimeDropDown("Start",$Start_MIN,$Start_HOUR,$Start_AMPM);
echo makeTimeDropDown("Finish",$Finish_MIN,$Finish_HOUR,$Finish_AMPM);
I haven't been able to test this, so there might be a bug or two... I'm away from my development machine right now.

The "trick" i used to clean up the minute tests could also be applied to the am/pm test, but that seems almost overkill. I also added a few close option tags, changed open <br>s to empty <br />, and other html->xhtml modification. (Current crusade of mine....)

If I was to play with it some more I would probably add two optional arguments to the function, one to toggle between 12/24 hour time and one to provide the minChoices array so that the calling function can set the granularity of the time. (12/24 hour defaults to 12 and minChoices defaults to the 15 minute increments used here or something like that)

Posted: Mon Aug 19, 2002 6:47 pm
by theChosen
You might consider this code also:

Code: Select all

<?php

  function WriteText($Title,$Hour,$Minute,$AMPM)&#123;
    //title
    echo "$Title:<br>\n<SELECT NAME=".str_replace(" ","",$Title)."_HOUR>\n";
    //hour
    $Trick1&#1111;$Hour]=" selected";
    for ($n=1; $n<=12; $n++)
    &#123;
      echo "<option value=$n".$Trick1&#1111;$n].">$n\n";
    &#125;
      echo "</SELECT>\n";
    //minute
    echo "<select name=".str_replace(" ","",$Title)."_MIN>\n";
    $Trick2&#1111;$Minute]=" selected";
    for ($n=0;$n<=60;$n+=15)&#123;
      echo "<option value='$n'".$Trick2&#1111;$n].">$n\n";
    &#125;
    echo "</SELECT>\n";
    // ? AM PM
    $Trick3&#1111;$AMPM]=' selected';
      echo "<select name=".str_replace(" ","",$Title)."_AMPM>\n";
      echo "<option value='AM'".$Trick3&#1111;'AM'].">AM\n";
      echo "<option value='PM'".$Trick3&#1111;'PM'].">PM\n";
      echo "</SELECT><p>\n";

  &#125;

  WriteText("Start Time",5,45,'PM');
  WriteText("End Time",6,30,'AM');
?>

Posted: Mon Aug 19, 2002 6:51 pm
by nielsene
theChosen: that's a nice little trick for cleaning up the nasty if, cool!

Posted: Mon Aug 19, 2002 6:54 pm
by fatalcure
yup, that is cool, learn something new :)

Posted: Mon Aug 19, 2002 7:09 pm
by phpPete
That's very cool
Man...I wish I could come back with working code like that as quick!

Posted: Tue Aug 20, 2002 5:33 pm
by ruthsimon
Thank you nielsene and theChosen for elegant suggestions that worked easily. I knew there had to be a better way.

I'm still a pretty novice scriptor, and trying to learn more how to elevate my code above the literal.

Thanks again, Ruth