<p>Hello guys, I am working on implementing a router on top of net/http, there is nothing fancy about it, and it's my first time to write a router "or a useful package ever [imposter syndrome detected :D]". I hope you guys can give me a feeback on my golang skils and how to improve this little project, I already have wrote some issues that i will be working on, but let's see what you have in mind.</p>
<p><a href="https://github.com/emostafa/garson">https://github.com/emostafa/garson</a></p>
<hr/>**评论：**<br/><br/>DenzelM: <pre><p>Hey Eslam, I read over your router. The code is well-structured; it was an easy read. It's cool when people, like you, dive in and learn something new, and it's even more awesome when they reach out to the community for feedback. With that said, allow me to point out what caught my eye:</p>
<p>First, a basic note on <strong>slice expressions</strong> before we get into the structural review: when re-slicing a slice in the case of expressions like <code>matches[0][1:len(matches[0])]</code>, you can simplify to <code>matches[0][1:]</code> which implies slicing from one to the end. <a href="https://golang.org/ref/spec#Slice_expressions">Slice expressions</a> is a great section, in the Go specification, on all the ways you can slice and dice.</p>
<p>Now, let's review the structure and semantics of the router, specifically:</p>
<ul>
<li>Returning HTTP errors (like 404)</li>
<li>regexp.MustCompile for initialization of globals, not locals</li>
<li>Compiling the same regexp over and over</li>
<li>Return error on invalid path, don't panic</li>
<li>Make the Router concurrency-safe</li>
</ul>
<p><strong>Returning HTTP errors (like 404)</strong></p>
<p>Go's net/http package provides a couple nice built-in methods for returning HTTP errors: <a href="https://golang.org/pkg/net/http/#NotFound">http.NotFound</a> can be used to return an error in your router handler. Transforming:</p>
<pre><code>route, err := router.Try(r.URL.Path, r.Method)
if err != nil {
NotFound(w)
return
}
</code></pre>
<p>Into:</p>
<pre><code>route, err := router.Try(r.URL.Path, r.Method)
if err != nil {
http.NotFound(w, r)
return
}
</code></pre>
<p>And thus saving you a bit of code, since you can eliminate your <em>NotFound</em> method. Internally, <code>http.NotFound</code> simply calls <a href="https://golang.org/pkg/net/http/#Error">http.Error</a> which can be used when you want to write back a more general 4xx/5xx error.</p>
<p><strong>regexp.MustCompile for initialization of globals, not locals</strong></p>
<p><a href="https://golang.org/pkg/regexp/#MustCompile">regexp.MustCompile</a> is exactly the same as <a href="https://golang.org/pkg/regexp/#Compile">regexp.Compile</a>, except for one very important detail. <em>MustCompile</em> panics when it doesn't understand the input.</p>
<p>That's useful when you want to initialize a global, as the documentation states:</p>
<blockquote>
<p>It simplifies safe initialization of global variables holding compiled regular expressions.</p>
</blockquote>
<p>But <em>MustCompile</em> is not so useful when initializing local variables. You'll want to use <em>Compile</em> for that and report an error back to the user.</p>
<p><strong>Compiling the same regexp over and over</strong></p>
<p>Another thing to keep in mind -- if a regular expression doesn't change, you only want to compile it once. Therefore, you shouldn't continue to recompile it. In the <em>add</em> function we have:</p>
<pre><code>if strings.Contains(route.Path, ":") {
re := regexp.MustCompile(`:(\w+)`)
// ...
}
</code></pre>
<p>Which means that we're recompiling that regexp every time <em>add</em> is called, even though it doesn't change. We can move that into a package-global variable. And being a package-global variable, <em>MustCompile</em> also begins to make more sense. So you may have something like this at the top of your package:</p>
<pre><code>var patternRE = regexp.MustCompile(`:(\w+)`)
</code></pre>
<p>In addition, we're recompiling a given route's regexp every time <em>Try</em> is called. Instead, we should probably update the <em>Route</em> structure to maintain a regexp instead of a string:</p>
<pre><code>type Route struct {
// ...
Path regexp.Regexp
}
</code></pre>
<p>And then we can compile the path in <em>add</em> when the new route is generated. That's more efficient.</p>
<p><strong>Return error on invalid path, don't panic</strong></p>
<p>Once again though, we should opt to return an error when adding a malformed path instead of panicking. Here's a test case that demonstrates the behavior I'm talking about:</p>
<pre><code>func TestAddingRoutePanicsOnBadInput(t *testing.T) {
defer func() {
if r := recover(); r != nil {
t.Error("Router panics on bad input, instead of returning error")
}
}()
router := New()
router.Get(`\p{malformed}`, nil)
router.Try("test", "GET")
}
</code></pre>
<p>That panic comes very late in the game; after the user has added their route, and after they've then tried routing something to it. We don't want to panic at all though. Instead <em>Get</em>/<em>Post</em>/<em>Put</em>/<em>Delete</em> should all return an error on a malformed path. So, we need to use <code>regexp.Compile</code> in place of <code>regexp.MustCompile</code>.</p>
<p><strong>Make the Router concurrency-safe</strong></p>
<p>As it stands now, your router is not concurrency-safe. And that's a bad thing for a web server because concurrency is the name of the game. If 2+ requests come in, matching the same route, it's possible that they will not receive the proper parameters. This is due to an architectural issue. I've created a test-case to demonstrate the behavior:</p>
<pre><code>func TestRoutesAreThreadSafe(t *testing.T) {
router := New()
router.Get("/:name", nil)
route1, _ := router.Try("/first", "GET")
if param := route1.Params["name"]; param != "first" {
t.Error(`For path "/first", expected :name="first", got`, param)
}
route2, _ := router.Try("/second", "GET")
if param := route2.Params["name"]; param != "second" {
t.Error(`For path "/second", expected :name="second", got`, param)
}
if param := route1.Params["name"]; param != "first" {
t.Error("Thread-unsafe sharing between two routes parameters")
}
}
</code></pre>
<p>If the responses to two routes are interleaved, they receive the same parameters because the internal <em>Params</em> map is destructively updated. How can we fix this? Well, we need to address it at a structural level.</p>
<p>When a route is matched by <em>Try</em> it returns a pointer to a single <em>Route</em> structure that includes the matched parameters in <em>Params</em>. Think about what matching a route really means... when we match a path, we want the handler and the parameters. Not the <em>Route</em>. In fact, <em>Route</em> is just a convenient internal structure for us to represent the mapping between (method, path, parameter parsing policy) => handler.</p>
<p>So, we need to reorganize <em>Route</em>, <em>Try</em>, and <em>parseParams</em>. There's no reason for <em>Params</em> to exist on a <em>Route</em>, so we'll remove that. Thus, there's no need for <em>parseParams</em> to be a method of <em>Route</em>, we can just make that a helper method. Now, when <em>Try</em> matches a <em>Route</em>, it simply parses out the parameters, and returns the handler along with the parameters and an error. The signature of <em>Try</em> becomes:</p>
<pre><code>func (r *Router) Try(path string, method string) (http.HandlerFunc, map[string]string, error)
</code></pre>
<p>By the way, I would probably define a type to represent the parameters.</p>
<pre><code>type Params map[string]string
</code></pre>
<p>Just to make things cleaner. And then you'll update the <em>ServeHTTP</em> method of your router to work with the new <em>Try</em>. That will make everything concurrency-safe.</p>
<p>Hopefully that helps, Eslam. Keep at it, continue asking questions, and have fun!</p></pre>emostafa: <pre><p>wow ! thanks man, that was the best feedback i have ever had, i wish i can upvote your comment 100 times not just one. I'll read your comment again and apply them on my code and get back to you.</p>
<p>again, thanks for your time and your effort :)</p></pre>emostafa: <pre><p>i wanted to ask about not panicing on malformed route path, by what i understood the functions Get(), Post() .. etc should return an error ? because i took a look on the source code of other routers and they don't return error from such functions.</p></pre>DenzelM: <pre><p>You're welcome Eslam. And you know what, I thought about it, and I think you're right in this instance. (This doesn't invalidate my previous recommendation.) However, note the subtlety -- they panic on a different code path. (So, you'll still have to update the code.)</p>
<p>Let's talk about why they panic because it is interesting. When the consumer of your router wants to create a new route, they're assuming a priori that the path they're passing in is valid. In other words, their code would be incorrect if the router returned an error and continued on with business. Some would argue that returning an error violates the contract provided by the router.</p>
<p>We panic when invariants or conditions (pre-/post-conditions) are invalidated. When we panic, as a library designer, we're saying it's not worth continuing the execution of this program because it would violate the program's correctness. It's up to the library designer to decide when this is appropriate. And generally, it's rarely appropriate.</p>
<p>In this instance, I tend to agree with you, panicking is appropriate. The difference being that you must panic upon definition of the route, not when the route is found and executed. Does that help clear things up?</p></pre>emostafa: <pre><p>I understand what you said, i implemented something in add() but i am not sure if it solves the issue. can you take a look ?
<a href="https://github.com/emostafa/garson/blob/master/router.go#L67" rel="nofollow">https://github.com/emostafa/garson/blob/master/router.go#L67</a></p></pre>DenzelM: <pre><p>Yup, that's the proper place to add it. But you can simplify... you'll want to go back and look over what the difference between <code>regexp.Compile</code> and <code>regexp.MustCompile</code> means while keeping in mind that I flipped my position, in this case, and agreed with your conclusion on panicking.</p></pre>Schpaencoder: <pre><p>Regarding the comments on panic, I use something I like to call 'panic oriented programming' - which means I spread my panics like todo's in all the code paths I need to take care of, so whenever I hit the path, the panic orients me towards what to do. </p>
<p>Of course, the goal in this is to remove all the panicking. </p></pre>SuspectGod: <pre><p>Wow that's actually pretty cool! </p></pre>goomba_gibbon: <pre><p>It's probably fine to loop over the Routes array like that but I expect performance would be better using a map. Perhaps using something like this:</p>
<pre><code>if val, ok := dict["foo"]; ok {
// Route found
}
</code></pre>
<p>I hope this helps. Also, welcome! </p></pre>
