0

I am creating a reusable node.js NavigationController class so I can reuse this in other server side projects If I may need or someone else may find it useful.

Here's the use case.

var navController = new NavigationController({
    routes : {
        '/user/:action/:anything' : 'UserController',
        '/app/:action' : 'AppController',
        '/file/:action' : 'FileController',
        '/feedback/:action' : 'FeedbackController',
        '/:anything' : 'ErrorController'
    },
    ErrorController : 'ErrorController'
});
navController.init();

The user upon server request can call this function of that object.

navController.navigate(req, res);

Now this and controllers are being called rightly. The thing under navigate(req, res) function which is a part of calling appropriate controller object based on URL is defined as function named getRouteByPath(path). This private function will get the route and will allow navigate(req, res) function to get controller class name to call.

var getRouteByPath = function(path) {
    for(var route in config.routes) {
        var routeRegex = '';

        var routeParts = route.split('/');

        for(var rp = 0; rp < routeParts.length; rp++) {

            // checking if route part starts with :
            if(routeParts[rp].indexOf(':') === 0) {

                // this is "anything" part
                routeRegex += '[/]?([A-Za-z0-9]*)';

            } else if(routeParts[rp] != "") {
                routeRegex += '[/]?' + routeParts[rp];
            }
        }

        var routeRegexResult = path.match(routeRegex);
        if(routeRegexResult) {
            console.log(routeRegexResult);
            return route;
        }
    }
    return null;
};

I am too worried about this function as if this is the right way to do that?

Umair A.
  • 6,690
  • 20
  • 83
  • 130
  • This question belongs to [codereview.stackexchange.com](http://codereview.stackexchange.com). – katspaugh Jul 18 '12 at 14:57
  • By that definition of posting questions in appropriate sites, StackOverflow should only have questions related to stacks which is overflowed :) – Umair A. Jul 18 '12 at 15:20

2 Answers2

2

Some flaws:

  • Why do you use the slash as a character class ([/])? No need to do that, only in regex literals you'd need to escape it with a backslash (like /\//g). Just use a single "/" instead (new RegExp("/", "g")).

  • .indexOf(<string>)==0 does work, but searches the whole string and is not very efficient. Better use startswith, in your case routePart.charAt(0)==":"

  • <string>.match(<string>) - I recommend building a new RegExp object and use .test, as you don't want to match - there is also no need to build capturing groups, I think, because you only return the route string but no matches (okay, you log them).

  • You want to check if the whole path matches your regexp? Dont forget to add ^ and $. Your current regex for the AppController also matches a route like /user/app/example.

  • Why is your slash (and only the slash) optional (/?)? Not only I don't think this is what you want, but it also opens the doors to catastrophic backtracking when building regexes like /\/?user\/?([A-Za-z0-9]*)\/?([A-Za-z0-9]*)/

    To escape that, you would need to make the whole group optional: (?:/([^/]*))?.

  • Also, you should build the regexes only once (on initialisation) and store them in a cache, instead of building them each time you call getRouteByPath. The compilation of the RegExp is somewhere hidden in your code, although it needs to happen.

Community
  • 1
  • 1
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • 1
    Don't use `exec` here, use `RegExp.prototype.test`. Also `routePart.indexOf(':') == 0` is better written as `routePart.charAt(0) == ':'`. – katspaugh Jul 18 '12 at 15:05
  • Thanks, that's exactly what I meant :-) Also going to add the indexOf thing – Bergi Jul 18 '12 at 15:06
  • [/]? is there because if the route defined is like /user/:action/:anything and user request an URL /user/me or /uer/me/you, the controller will be invoked at both requests. Is that not the normal way of doing this? – Umair A. Jul 18 '12 at 15:13
  • Can you also explain this point? "Why do you use the slash (/) as a character class? No need to do that, only in regex literals you'd need to escape it with a backslash." – Umair A. Jul 18 '12 at 15:15
  • There is a need of capturing groups because I want to get arguments to pass to controllers. – Umair A. Jul 18 '12 at 15:23
  • I've expanded the section on the optional slash (please read the linked article). `|` The point is that you don't need a character class for single characters. I'd like to know why you've used one. `|` If you want to get the matched arguments, you need to pass them to the controllers somehow. Currently you only return the route string – Bergi Jul 18 '12 at 15:43
  • Great guide! Still your first point isn't clear to me and why do you prefer me RegExp object? – Umair A. Jul 18 '12 at 15:43
  • 1
    Every invocation of `match` with a string creates one, and that "compilation" is unnoticed in your code altough being a comparatively massive computation. See also my last point – Bergi Jul 18 '12 at 15:50
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/14084/discussion-between-umair-ashraf-and-bergi) – Umair A. Jul 18 '12 at 16:08
0

A few notes:

routeRegex += '[/]?([A-Za-z0-9]*)';

says the routes may or may not be present // will match maybe a + is more suited than a *.

Also since I thin _ is allowed in a web route

Your .split('/'), will remove all / from the route so it shouldn't be in split list

Rishi Diwan
  • 338
  • 1
  • 10
  • [/]? is there because if the route defined is like /user/:action/:anything and user request an URL /user/me or /uer/me/you, the controller will be invoked at both requests. Is that not the normal way of doing this? – Umair A. Jul 18 '12 at 15:13
  • Right my brain censored out the fact you append them all together, also `[/]?` says it may or may not be present, don't we always want it present? – Rishi Diwan Jul 18 '12 at 17:18