REGEXless dispatcher

Question

I've recently helped fellow SOFer to answer his question here: Routing in my PHP MVC framework

Then I thought

Hey, why not improve this code and put in my snippets library.

This is what I came up with:

/**
 * @author Gajus Kuizinas <[email protected]>
 * @copyright Anuary Ltd, http://anuary.com
 * @version 1.0.3 (2011 12 07)
 * @param array $routes e.g., array('page/:id' => array('controller' => 'page', 'parameters' => array('id' => 1)));
 */
function ay_dispatcher($url, $routes)
{
    $url            = array
    (
        'original'  => $url,
        'path'      => explode('/', parse_url($url, PHP_URL_PATH))
    );

    $url['length']  = count($url['path']);

    foreach($routes as $filter => $data)
    {
        // reset the parameters every time in case there is partial match
        $parameters     = array();

        $filter         = array
        (
            'original'  => $filter,
            'path'      => explode('/', $filter)
        );

        $filter['length']   = count($filter['path']);

        // this filter is irrelevent
        if($url['length'] <> $filter['length'])
        {
            continue;
        }

        foreach($filter['path'] as $i => $key)
        {
            if(strpos($key, ':') === 0)
            {
                $parameters[substr($key, 1)]    = $url['path'][$i];
            }
            // this filter is irrelevent
            else if($key != $url['path'][$i])
            {       
                continue 2;
            }
        }

        if(!array_key_exists('parameters', $data))
        {
            $data['parameters'] = array();
        }

        $data['parameters'] = array_merge($data['parameters'], $parameters);

        return $data;
    }

    return FALSE;
}

The 1.0.2 version https://gist.github.com/68bcb1e087237bde76ea.

I'd like to hear comments on the code logic. It is most certainly better in terms of performance than any regex dispatcher. Did I forget any essential features, though?

Answer 1

The original index is never used and can currently be removed from both url and filter.

Your use of temporary variables is actually making the code less easy to read.

1. Replace the length indexes which are just a temporary values to check for validity. I call this pathCount in the code below.
2. Replace parameters and the merging that you do with just continually adding to the array. Adding to the array should overwrite any existing values equivalently to the array_merge operation.

I would also change the name of $key to something else as it confuses me with it being an array key. (I would call it $component or something similar). I didn't make this change in the code below.

Edit:

In fact further to this $url and $filter are also temporary variables hurting your readability. They could be replaced at the appropriate points with:

 $urlPath = explode('/', parse_url($url, PHP_URL_PATH);
 $filterPath = explode('/', $filter);

The code then becomes (not tested):

function ay_dispatcher($url, $routes)
{
   $urlPath = explode('/', parse_url($url, PHP_URL_PATH));

   foreach($routes as $filter => $data)
   {
      $filterPath = explode('/', $filter);

      if (count($filterPath) !== $count($urlPath))
      {
          continue;
      }

      if(!array_key_exists('parameters', $data))
      {
          $data['parameters'] = array();
      }

      foreach($filterPath as $i => $key)
      {
          if(strpos($key, ':') === 0)
          {
              $data['parameters'][substr($key, 1)] = $urlPath[$i];
          }
          // this filter is irrelevent
          else if($key !== $urlPath[$i])
          {       
              continue 2;
          }
      }

      return $data;
   }

   return FALSE;
}

I am not sure that your logic for counting the filter path should just be a count? Perhaps you need a function for that as parameters used in the filter would effect the count?

Answer 2

1, I'd remove the $return variable at all and change

...
    $return = $data;
    break;
}
return $return;

to

...
    return $data;
}

return FALSE;

2, Using the same variable name twice is hard to read. I'd change

foreach($routes as $filter => $data)
{
    ...
    $filter         = array
    (
        'original'  => $filter,
        'path'      => explode('/', $filter)
    );

to

foreach($routes as $originalFilter => $data)
{
    ...
    $filter         = array
    (
        'original'  => $originalFilter,
        'path'      => explode('/', $originalFilter)
    );

Or you could rename the second $filter to $filterInfo or something like that. The same is true for the $url variable.

3, I'd extract out the condition from

// this filter is irrelevent
if($url['length'] <> $filter['length']) {
    continue;
}

to a function whose name comes from the text of the comment:

function isFilterRelevant($url, $filter) {
    if ($url['length'] == $filter['length']) {
        return true;
    }
    return false;
}

...
if (!isFilterRelevant($url, $filter)) {
    continue;
}

From Clean Code by Robert C. Martin, page 53-54:

Comments are not like Schindler’s List. They are not “pure good.” Indeed, comments are, at best, a necessary evil. If our programming languages were expressive enough, or if we had the talent to subtly wield those languages to express our intent, we would not need comments very much—perhaps not at all.

The proper use of comments is to compensate for our failure to express ourself in code. Note that I used the word failure. I meant it. Comments are always failures. We must have them because we cannot always figure out how to express ourselves without them, but their use is not a cause for celebration.

A local variable also would do it.

From the book Refactoring: Improving the Design of Existing Code:

Expressions can become very complex and hard to read. In such situations temporary variables can be helpful to break down the expression into something more manageable.

Introduce Explaining Variable is particularly valuable with conditional logic in which it is useful to take each clause of a condition and explain what the condition means with a well-named temp.

4, What does the following condition check?

if(strpos($key, ':') === 0)

I can guess, but a function with a proper name would be better here.