Jump to content

Code review for custom routing rules


sKunKbad

Recommended Posts

 In a project that I'm working on I can specify routing rules, which is somewhat similar to mod_rewrite but in PHP. It's currently set up to use full regular expressions, but it's kind of overkill. I'm trying to convert it to use routing rules similar to some of the php frameworks I've seen. The code I've written up below is working, and while it's unlikely that I would need anything more complex, I'm wondering if anyone would like to comment, offer suggestions, or offer criticisms. This little piece of code is just a part of a bigger routing class, but this is the code that I'm concerned with. Thanks.

<?php

$cfg['routes'] = [
    'cars/(:any)/(:any)/(:num).php' => 'cars/$3/$1/$2',
    'default_route'                 => 'cars/index',
    'trucks/parts?year=(:num)'      => 'parts/trucks/$1',
    'vans/articles(:any)'           => 'articles$1' 
];

$uris = [
    'cars/toyota/tercel/2014.php', # /cars/2014/toyota/tercel
    'default_route',               # /cars/index
    'trucks/parts?year=2014',      # /parts/trucks/2014
    'vans/articles?p=42342'        # /articles?p=42342
];

$i = 0;
foreach( $cfg['routes'] as $k => $v )
{
    $k = '|^' . preg_quote( $k ) . '$|uiD';

    $k = str_replace(
        [
            '\(\:any\)',
            '\(\:alnum\)',
            '\(\:num\)',
            '\(\:alpha\)',
            '\(\:segment\)'
        ], [
            '(.+)',
            '([[:alnum:]]+)',
            '([[:digit:]]+)',
            '([[:alpha:]]+)',
            '([^/]*)'
        ], 
        $k
    );

    echo '<br />' . $uris[$i] . '<br />' . $k . '<br />';
    
    if( @preg_match( $k, $uris[$i] ) )
    {
        echo preg_replace( $k, $v, $uris[$i] ) . '<br /><br />';
    }

    $i++;
}
Link to comment
Share on other sites

At first glance, the only thing that I could say is that it's hard to 'read', IMO. Personally, I would add functions with names of what the code really does. 

 

For example:

   $k = str_replace(
        [
            '\(\:any\)',
            '\(\:alnum\)',
            '\(\:num\)',
            '\(\:alpha\)',
            '\(\:segment\)'
        ], [
            '(.+)',
            '([[:alnum:]]+)',
            '([[:digit:]]+)',
            '([[:alpha:]]+)',
            '([^/]*)'
        ], 
        $k
    );

What is it doing?  

Also, I don't like using variable with only letters (like $k), except when it's a counter. I would probably use $uriRule or something like that. But, this is maybe just a personal view.

 

I guess that it's not the kind of criticism you were looking for? Why are you 'concerned' with this piece of code?  Performance?

Link to comment
Share on other sites

At first glance, the only thing that I could say is that it's hard to 'read', IMO. Personally, I would add functions with names of what the code really does. 

 

For example:

   $k = str_replace(
        [
            '\(\:any\)',
            '\(\:alnum\)',
            '\(\:num\)',
            '\(\:alpha\)',
            '\(\:segment\)'
        ], [
            '(.+)',
            '([[:alnum:]]+)',
            '([[:digit:]]+)',
            '([[:alpha:]]+)',
            '([^/]*)'
        ], 
        $k
    );

What is it doing?  

Also, I don't like using variable with only letters (like $k), except when it's a counter. I would probably use $uriRule or something like that. But, this is maybe just a personal view.

 

I guess that it's not the kind of criticism you were looking for? Why are you 'concerned' with this piece of code?  Performance?

 

After preg_quote slashes regex characters, this code replaces the placeholders with real regex.

 

Sure, performance is always a concern, but many eyes on the code could point out something that I didn't realize could be an issue.

Link to comment
Share on other sites

Why reinvent this wheel? It's been done by Symfony (and many other projects i'm sure), and packaged up ready to be used in any project.

 

http://symfony.com/doc/current/components/routing

 

I would agree with you, except that the router already exists, I'm just trying to modify it. Also, the symfony router is super heavy. The existing router may not be as configurable and awesome, but it's about a tenth of the code or less.

Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.