Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have created a nice url structure for my site.

In my .htaccess I have:

FallBackResource /index.php

And in my router class I have:

class init {

function __construct($url)
{
    $URLElements = explode('/', $url) ; // Adjust if needed.

    if($url)
    {
        $class = $URLElements[0] ;

        if(substr_count($url, '/') == 1)
        {
            $method = $URLElements[1] ;
        }

    }

    if(($t = substr_count($url, '/')) > 1)
    {
        for($i=2;$i<$t+1;$i++) {
            echo $URLElements[$i].'<br />';
        }
    }

}

}

Is this ok and what are your thoughts (how can it be improved etc)?

share|improve this question

1 Answer 1

up vote 2 down vote accepted

There's not really much to review here, but a few quick items:

  • This is going to sound weird at first glance, but you have a route() method that takes a path instead of depending directly on the request uri.
    • A router's job is to route. By hard coding the $_SERVER['REQUEST_URI'], you've made it limited to one data source.
    • route($path) would be much more flexible, and it would mean that one object could be used for routing more than 1 time.
    • Think about test driven design. How would you test a hard coded $_SERVER['REQUEST_URI']? (Other than putting a hacky assignment to it somewhere)
  • Never assume that array indexes exist
    • If someone goes to index.php or /foo, so on, then $URLElements might not have 2 elements
  • As it is, your class does nothing useful -- you probably know that though :)
    • A major improvement would be making it actually do something :)
share|improve this answer
    
Thanks, I have implemented your comments (see edit). Yes it doesn't really do anything yet, I will be putting in all the stuff like class_exists and validation etc shortly. –  imperium2335 Nov 29 '12 at 17:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.