# Tag Info

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 ...

7

First off, congrats on making your own, that's the best way to learn and usually you don't need all the added overhead that a prefab provides. In the implementation of MVC I like to use, there is a router between the view and controller, so its more of a MVRC rather than MVC. Just another level of abstraction, but I like it. It seems like what you are ...

7

I agree with luiscubal about restructuring the code (with the one minor difference that I would use braces), however, I think you could make something a bit cleaner with array_shift. $this->controller = array_shift($this->pageData); $this->action = array_shift($this->pageData); $this->values =$this->pageData; This is functionally ...

7

There seems to be some code repetition. Perhaps this could be improved like this: if ($url_chunks_num >= 1)$this->controller = $this->pageData[0]; if ($url_chunks_num >= 2) $this->action =$this->pageData[1]; if ($url_chunks_num > 2)$this->values = array_slice($his->pageData, 2); Or if ($url_chunks_num >= 1) { $... 7 This is not OOP. Observe how your class contains only static functions. There are no class properties. This can be written equivalently: namespace Router; function getPath($uri = null){ // Same implementation (I have just cut it out for readability). } function getQuery($uri = null){ // Same implementation. } function follow($uri = null){ /...

7

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(); ... 7 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 What you've written is more or less the standard implementation of spl_autoload(). This is an equivalent approach:$paths = array( get_include_path(), ROOT . DS . 'library' . DS . 'intranet' . DS . 'classes', ROOT . DS . 'application' . DS . 'controller', ROOT . DS . 'application' . DS . 'model', ROOT . DS . 'application' . DS . '...

5

Large wall of text incoming. Go to the bathroom, grab a drink, etc... Try/Catch The first thing I would refactor is your try/catch statement. Its just way too large, though that may just be the internal comments (which I'll get to next). Try blocks should really only encompass those statements that should be try'd. Everything else is just unnecessary ...

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

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

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

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

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 => ... 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 ... 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

Nice initiative on a lightweight MVC framework! Note wanted to add more reference links, but it won't allow me to do more thanb 2 Config Pass the default controller setting to the function or Pass all routing related settings to the __constructor. and use them as class attributes Loader class The setup isn't all that bad. Nice and lightweight. A few ...

3

Are you intending to reuse the route, handleAuth, and handleRequestData functions? They could be "private" to the Client constructor function: function Client(req, res) { function route() { //... } function handleAuth() { //... } function handleRequestData() { //... } route(); handleRoute(); ...

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

First rewrite you code without the following things: global static @ Your code isn't looking an object oriented approach right now. If you can forget the usage of the two mentioned keywords (global is really bad and the static can be a hard question where to use it) we can give you further advice how you can separate in code the functionalities. I don't ...

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

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 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

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 ...

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