9

It seems a little risky to me to just blindly use the language designation that the URL contains. If it's possible to trick Express into looking somewhere in your file system besides where you expect (perhaps with dots in the path), then you could have a security issue. It would be safer if you checked to see if it was one of a list of known-safe ...


8

Unfortunately, this is a very large question to ask. So there will be gaps in my answer but i will answer it best i can using code where possible. You do not want to couple your router with the controllers. They both do different things. If you do combine them, you end up with a mess (as you have found out). class Router { private $routes = array(); ...


8

You don't need to define empty actions in your Controller. Your code should work even if you delete all the empty actions, leaving you only with the views and routes: page_controller.rb: class PageController < ApplicationController end views: adventure.html.erb cooking.html.erb dancing.html.erb programming.html.erb reading.html.erb running.html.erb ...


7

It's looking really good. Some things I might consider that aren't necessary but could help move the code to some better abstractions and hence allow for easier unit testing of features include. Abstracting your service away from UI related actions by removing the dependency on HttpContextBase. It looks like all you are using it for is to obtain the ...


5

On a side note, is there a difference betwen "routing" and "URL rewriting"? The terminology isn't all that clear to me. Yes. Generally, "routing" describes the process of mapping URLs to code in some form. The standard example would be mapping a URL to a method of a controller. On the other hand, "rewriting" doesn't map a URL to code, but maps a URL to a ...


4

I notice my controller headers seem fairly bloated, mostly because of the need to include the the modules, services, and factories in them. So maybe something like You can use the implicit method of dependency injection, where as long as the variable names match the dependency name, Angular will figure it out for you. However, for production, you'll need ...


4

What is a router used for? Translate one address to another one connection two different network to eahc other depending on rules. It will never instantita a browser to display for example a HTTP request. Your class doing to much and much lesser than it should Your router should only deal with the input address and not instantiating a controller and other ...


4

A few minor tweaks could help you out: Add a close method to all of your views, which cleans up your DOM, and unbinds any bound events (eg, with this.stopListening()). This will prevent 'zombie events' -- events bound to views which are no longer rendered. Move the logic for switching pages to a high-level application contoller. This will keep your router ...


4

The first improvement you definitely want is capturing groups. These groups allow you to get rid of splitting at "/" and working on magic indices and instead use a Matcher to extract the parts from a matched string. This greatly simplifies your delegating methods that work on a String to using the Matcher instance you used and threw away in your if-...


4

Your function currently search for files in specific directory. This is good, but is nor routing, nor URL rewriting. I could say it is very rudimentary routing. Consider following examples: http://domain.com/show/ - list all users http://domain.com/show/124 - list specific user (with ID = 124) Second one can not be done with your router. URL ...


4

Things you've done well There's a lot to like here. This is good code, easy to read and understand. I especially like: Comments Standard formatting (two-space indent, etc.) Short methods A little more vertical white space Comment blocks that preceed a method, module, or class definition should be preceded by a blank line. So instead of this: module ...


4

I am personally a fan of pretty much limiting the "routing" aspect of a front-controller like this to just getting the request to the proper controller for further processing. But before talking about that, I want to ask you whether what you really have here is a "router" or a "URI parser"? It seems like just a URL parser to me in that is does no routing. ...


4

I found this post because I needed something that did what you implemented. I tried to use the code and ran into some nasty routing bugs whereby Angular was experiencing internal errors. The cause is this bit: // Get the value of all the BehaviorSubject observables in the route so that // the values can be directly accessed from the route ...


4

I see some things that may help you improve your program. Separate interface from implementation It makes the code somewhat longer for a code review, but it's often very useful to separate the interface from the implementation. In C++, this is usually done by putting the interface into separate .h files and the corresponding implementation into .cpp files. ...


3

If this unit of code is a router, then it should remain a router. Have the controller handle its dependencies. You could have multiple controllers, each with its own dependencies. You wouldn't want a single router code become a monster just to keep up with the controllers. class Router{ private $request; private $cleanUri; private $controller; ...


3

Let's look at your questions individually: Am I utilizing dependency injection correctly? Yes and no. You are, essentially, injecting the routes. That's fine. But you extract them from a $config object. If you are working out some MVC (micro-) framework, then chances are that config object is an instance of a specific class. As your code-base grows, so ...


3

Not a lot to review, obviously the most repeated part of your code is appointment_reminder_responses, if you create a variable called root and then built your routes with concatenations like root + "/confirmation_thanks" then that would be a good start. Furthermore you create a function that takes a string and modifies so that "xxx/zzzz/yyyy" becomes "...


3

I would not create custom controller actions. up and down are attributes of a Vote, so I would do the following: resources :widgets do resources :votes, only: [:new, :create] end And then send a POST to VotesController#create with params[direction: 'up'] and have an attribute on Vote called direction. Then validate Vote#direction can only be up or down: ...


3

The first thing I notice is that you are using the superglobals directly in your class. Normally you want to inject a request object into the class which contains all the info about the request. The next thing I notice is the use of static methods, e.g.: \Models\Session::checkSession() and Config::$defaultLoggedInController Which not only makes you ...


3

Where you have: $class = ucfirst($this->autoMap($command[0]), $this->classAliases); I think it should be: $class = ucfirst($this->autoMap($command[0], $this->classAliases)); The closing ) is in the wrong place. Besides that I like your router, maybe it would be even better if it had a module control (so you could have something like: module/...


3

Unless you are deliberately studying regular expressions, you should look into Jersey - UriTemplate.match() would probably make your life a lot easier. public void route(HttpServletRequest request) { String url = request.getRequestURI(); // ... } First thing to notice here, is that you don't use the request object after you have extracted the uri ...


3

I must first say that I am an hater of the out functionality because it makes your code look awful. Given that, I would change your GetIdAndKeys method to the following, if you don't want to create a class for the result...: public static Tuple<int, Dictionary<string, string>> GetIdAndKeys(RouteValueDictionary dataTokens) { if (dataTokens....


3

Don't repeat yourself public function set($route) { if ( is_array($route) ) { foreach ( $route as $regex => $control ) { $this->routes[$regex] = $control; } } else { $default_routes = $this->generate_default_routes($route); foreach ( $default_routes as $regex => ...


3

Your algorithm seems to be working. Here is the few things I would do to improve it: // The routes should be a parameter of the function function getParams(routes, url) { var params = null; // Depending on the targeted browser, you could use the native forEach method routes.forEach(function (route) { // I personally prefer to use methods in here ...


3

I haven't quite understood what Babel is used for at all. Babel is used for transpilation - the process of converting unsupported/future syntax into one that is supported at the moment. For instance, your destructuring assignment: const { Router, Route, IndexRoute, Redirect, Link, IndexLink } = ReactRouter ...


3

Looking pretty neat! But it is missing some important functionality: Ability to define the method of request. There are many kinds of HTTP request methods. Click here to learn more. The fact that you have to call the "dyn" method for dynamic URLs is just not my taste. Other routers are capable of determining if the URL is dynamic or not. This is pretty ...


3

The common definition of XSS is that code faults which allow someone to manipulate your website appearance, access prohibited sections of data, view inaccessible files or possibly steal credentials and cheat your app's users into entering data on the wrong website. One of the most commonly used XSS vulnerabilities is what we call "free input" when the app ...


3

That's a nice mini-flask exercise. ROUTES = None Deferring the assignment to later, and initializing to Routes(), would seem more natural, to avoid having to test for is None. global ROUTES ROUTES = self ... if __name__ == '__main__': @Route('/') Oh, wait, you're not testing against None. You're just jamming it in there. Consider making the ...


3

You can parameterize the @path annotation with regular expressions to specify one class/method that accepts multiple paths @Path("/{firstpath: first|second}/{param}") // will match /first/{param} and /second/{param} public class First { @GET @Path("/{data}{p:/?}{difference:(\w*)}") // {p:/?} makes "/" optional, {difference:(\w*)} allows for ...


Only top voted, non community-wiki answers of a minimum length are eligible