Help me make this code more efficient

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
ruthsimon
Forum Newbie
Posts: 19
Joined: Mon Aug 19, 2002 5:44 pm

Help me make this code more efficient

Post 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
User avatar
nielsene
DevNet Resident
Posts: 1834
Joined: Fri Aug 16, 2002 8:57 am
Location: Watertown, MA

Post 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)
User avatar
theChosen
Forum Newbie
Posts: 15
Joined: Sun Aug 18, 2002 11:00 am
Location: RO, Europe

Post 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');
?>
User avatar
nielsene
DevNet Resident
Posts: 1834
Joined: Fri Aug 16, 2002 8:57 am
Location: Watertown, MA

Post by nielsene »

theChosen: that's a nice little trick for cleaning up the nasty if, cool!
fatalcure
Forum Contributor
Posts: 141
Joined: Thu Jul 04, 2002 12:57 pm
Contact:

Post by fatalcure »

yup, that is cool, learn something new :)
User avatar
phpPete
Forum Commoner
Posts: 97
Joined: Sun Aug 18, 2002 4:40 pm
Location: New Jersey

Post by phpPete »

That's very cool
Man...I wish I could come back with working code like that as quick!
ruthsimon
Forum Newbie
Posts: 19
Joined: Mon Aug 19, 2002 5:44 pm

Post 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
Post Reply