I just wrote a horrible PHP function, I need some help (elseif chain - switch?)

Question

Hi,

I am making a site that determines the value of an array based on what time it is. I wrote this awful (functional) script, and am wondering if I could have made it more concise. I started with a case/switch statement, but had trouble getting multiple conditionals working with it. Here's the dirty deed:

if ($now < november 18th) {
    $array_to_use = $home;
}
elseif (november 18th < $now && $now < november 21st ) {
    $array_to_use = $driving;
}
elseif (november 21st < $now && $now < november 22nd) {
    $array_to_use = $flying;
}
...
...
...
elseif (february 1st < $now) {
    $array_to_use = $arrived;
}
else {
    $array_to_use = $default;
}

The schedule is actually more complicated and has 13 elseifstatements in it. Can someone please confirm that I just had coder's block and that there's a better way to do this?

EDIT: I changed the Unix Timestamps to rough real times so it's easier to understand what I'm doing (hopefully)

EDIT 2: Please forgive the currently broken Javascript clock, but this is the site I'm working on:

Time Table.

Each array is based on my location, and there are 15 "they are currently" based on the time it is. It's a small problem domain with known start/end times, so flexibility isn't key, just getting it all written. You can see how the time is continuous, and only one array of strings needs to be selected at a time.

Answer 1

You can use a switch statement, doing something like this:

switch (true)
{
    case $now < 1258783201:
        // your stuff
        break;
    case $now < 1258783201
        // more of your stuff
        break;
    //...
}

That's at least a little cleaner...

Answer 2

First , please please please take out your hardcoded numbers and put them into constants.

$FLIGHT_START_TIME = 1258956001;
$FLIGHT_END_TIME   = 1260511201;

Second, I would make mini functions for each of the conditionals:

I.e.

function isFlying($time)
{
    return ( $FLIGHT_START_TIME < $time && $time < $FLIGHT_END_TIME );
}

Third, take your whole set of conditionals, and put it into a function to get your current state, and replace in your function calls:

function getStateArrayForTime($time)
{

   if (isDriving($time)
   {
       return $driving;
   }
   if ( isFlying($time) )
   {
        return $flying;
   }
...etc
}

Last, replace the whole inline section of code with your single function call:

$currentState = getStateArrayForTime($now);

As other posters have also commented, at this point you can use a data table driven function to return the state if you know only the start and end time will be the state parameters:

so replace the implementation of getStateArrayForTime with:

function getStateArrayForTime ($time)
{
// 
$states = array (
    array("startTime" => 1258956001, "endTime" => 1260511201, "state" => $flying),
    array("startTime" => 1260511201, "endTime" => 1260517000, "state" => $driving),
..etc...
);
    foreach($states as $checkStateArray)
    {
        if($checkStateArray['startTime'] < $time && $time < $checkStateArray['endTime'])
        {
            return $checkStateArray['state'];
        }
    }
    return null;
}

Finally, some people might ask "why do things in this order?" I can't claim credit at all, other than in the application, but Martin Fowler has a great book called "Refactoring" that explains why you clean code up one step at a time, and test at each step of the way, then finally replace functions wholesale that don't make sense, all the while testing that they are functionally equivalent.

Answer 3

Something like this:

$array_to_use = null;
$dispatch = array(1258783201, $home, 1258956001, $driving, ..., $arrived);
for ($i=0; i<count($dispatch); $i+=2) {
    if ($now<$dispatch[$i]) {
        $array_to_use = $dispatch[$i+1];
        break;
    }
}
if ($array_to_use==null) $array_to_use = $dispatch[count($dispatch)-1];

You also need to think about whether you need "<" or "<=" condition.

Answer 4

It might be overkill, but I would have done something like this so that I could put all the time ranges in one clear spot:

@timeWindows = ({ start -> 0, end -> 1258783201, array -> $home },
                ... ,
                {start -> 1260511201, end -> MAXVAL, array -> $arrived});

and then a loop like

$array_to_use = $default;

foreach (my $window in @timeWindows) {
   if (($now > $window->start) && ($now < $window->end)) {
       $array_to_use = $window->array;
       last;
   }
}

Sorry it's in Perl, I don't know PHP, but I imagine it's similar.

Answer 5

You can put the time and array to use in an array and loop them to select.

$Selctions = array(
    1258783201 => $Home,
    1258956001 => $Driving,
    1260511201 => $Flying,
    ...
    1260511201 => $Arriving
);

// MUST SORT so that the checking will not skip
ksort($Selction);
$TimeToUse = -1;
$Now       = ...;
foreach ($Selctions as $Time => $Array) {
    if ($Now < $Time) {
        $TimeToUse = $Time;
        break;
    }
}
$ArrayToUse = ($TimeToUse != -1) ? $Selctions[$TimeToUse] : $Default;

This method can only be used when the times has no gap (one range right after another).

Hope this helps.

Answer 6

You might want to learn the Command Pattern; it can also help in this circumstance.