<p>I am attempting to learn Go by building a simple RESTful API. I'd prefer not to use any frameworks, so I can learn more thoroughly.</p>
<p>I've already discovered how cumbersome the error checking can be, null values in structs... and how they can be a pain with PATCH.</p>
<p>Anyway, I am having a good time. I'd appreciate some code review if any of you have time :)</p>
<p><a href="https://github.com/shwing/moledro">https://github.com/shwing/moledro</a></p>
<hr/>**评论：**<br/><br/>gohacker: <pre><p>Panicing in handlers is a faux pas. Instead just log an error and return an appropriate HTTP status (500 or whatever).</p></pre>feelsmagical: <pre><p>Good call, thank you. I've added some better error handling. Any further advice?</p>
<p><a href="https://github.com/shwing/moledro/commit/145e1a0c190130cb801f8fcc14b7a83e5cddab29" rel="nofollow">https://github.com/shwing/moledro/commit/145e1a0c190130cb801f8fcc14b7a83e5cddab29</a></p></pre>Growlizing: <pre><p>The error handling seems excessive and intrusive as you start, but then you realise, in reality, most every function call can fail.
A tip here can be to log --> return the error.</p>
<pre><code>if err != nil {
log.Printf('error getting db: %v', err)
return nil, err
}
</code></pre>
<p>Or just collect the logging in the caller method (more difficult getting a stack trace or line number where the error origininated).</p></pre>michaelbironneau: <pre><p>In sub.go, you are ignoring the error return of all db functions (eg. line 44, Get() method could return an error). You should instead change your *Sub functions to return this error to the handler and follow gohacker's suggestion.</p></pre>feelsmagical: <pre><p>Thanks. I've added some better error handling. Any further advice?</p>
<p><a href="https://github.com/shwing/moledro/commit/145e1a0c190130cb801f8fcc14b7a83e5cddab29" rel="nofollow">https://github.com/shwing/moledro/commit/145e1a0c190130cb801f8fcc14b7a83e5cddab29</a></p></pre>kortemy: <pre><p>First thing I noticed is that all your code is in main package. For a small API it is not a problem per se, but generally it is better idea to decouple your code into packages, and expose (make public) only necessary things. For example db declared in main.go is accessed in sub.go as well. I would suggest creating for example dba package, where all your db code lives, and you can control access to database by exposing only needed functions.</p>
<p>I also support what <a href="/u/gohacker">/u/gohacker</a> and <a href="/u/michaelbironneau">/u/michaelbironneau</a> said. </p>
<p>Never panic in handler. Actually avoid panic in whole user request flow - just log the error and return some error code. Panic is for unexpected errors, like failing to connect to db in main.go:34. Also, where those panics are invoked, you are writing those errors as json to response. My usual approach to handling errors is to define few common errors (404, 500, etc...) and write them instead. </p>
<p>I know error checking can be bothering at times, and you can skip it where you deem unnecessary. Fetching stuff from db is not the place to skip. Some db drivers will return error on 0 records found - you need to catch that and inform the user that results are empty.</p></pre>feelsmagical: <pre><p>Great feedback. Anything else? Multiple returns in the handlers feel funky.</p>
<p>Are my naming conventions idiomatic? For some reason I have a tendency to name functions with lowercase letter for the first word? Maybe too much JS.</p>
<p>I feel a bit weird about having all of these functions on global namespace. I feel like it makes more sense to have things like Model.Create() Model.Get() etc. Is this just because I've been tainted by Ruby?</p></pre>Hard_NOP_Life: <pre><p>The naming convention in Go is to do camelCase, which you seem to do most of the time, but not all of the time. Whatever you pick you should try to be consistent. There's no need to uppercase the names of your handlers.</p></pre>kortemy: <pre><p>Multiple returns in handlers (and functions in general) is not a bad practice. I like the "early return" way, check for errors and return if needed.</p>
<p>You can still have Model.Create() or Model.Get(), depending how you model it. You can create package "sub", then you would have to import "sub" and call sub.Get() and sub.Create(). It is a good idea to create a separate namespace for each model. You can also attach some functions to Sub struct as well, then you can execute them over a specific object. For example sub.Delete(), or sub.Save().</p></pre>Hard_NOP_Life: <pre><p>I see you repeat setting the header in all your handlers:</p>
<pre><code>w.Header().Set("Content-Type", "application/json; charset=UTF-8")
</code></pre>
<p>You should consider wrapping this up into a function that you call rather than just writing the same line in all the handlers. That way if in the future you need to add more headers (such as the cors headers for example) you don't need to go update every single one.</p></pre>feelsmagical: <pre><p>Should I use a middlewear library to do this? Negroni?</p></pre>Hard_NOP_Life: <pre><p>Using a middleware library is probably overkill. You can either write something that returns the correct headers and just call that, like this:</p>
<pre><code>func setCorsHeaders(response http.ResponseWriter, req *http.Request) http.ResponseWriter {
response.Header().Set("Access-Control-Allow-Origin", req.Header.Get("Origin"))
response.Header().Set("Access-Control-Allow-Credentials", "true")
response.Header().Set("Access-Control-Allow-Headers", "AUTHORIZATION, CONTENT-TYPE")
response.Header().Set("Access-Control-Allow-Methods", "GET, PUT, POST")
return response
}
</code></pre>
<p>or do what I do, and wrap your handler in a closure:</p>
<pre><code>func withHeaders(f func(http.ResponseWriter, *http.Request) func(http.ResponseWriter, *http.Request){
return func (response http.ResponseWriter, request *http.Request) {
response.Header().Set("Content-Type", "application/json; charset=UTF-8")
f(response, request)
}
}
</code></pre>
<p>And then you just change your route registration so it looks like this:</p>
<pre><code>router.GET("/s", withHeaders(controllers.SubIndex))
</code></pre>
<p>Eventually you may want a middleware library if your project gets large, but for smaller APIs it really isn't needed. </p></pre>feelsmagical: <pre><p>Thanks, this is super helpful.</p>
<p>Do you have any recommendations on getting started writing integration tests?</p></pre>intermernet: <pre><p>Not really "getting started", but one of the more illuminating pieces of code I've read from the Go std lib is the builtin net/http server tests (<a href="https://golang.org/src/net/http/serve_test.go" rel="nofollow">https://golang.org/src/net/http/serve_test.go</a>). Some of the tests are simple, some of them are quite complex, but you should be able to get an idea for what you can do.</p>
<p>Also, the httputil package is pretty useful for http server tests. <a href="http://golang.org/pkg/net/http/httputil/" rel="nofollow">http://golang.org/pkg/net/http/httputil/</a></p></pre>nokkare: <pre><p>Do not forget to run "go fmt" and "go vet". There's also some other cool tools listed at <a href="http://dominik.honnef.co/posts/2014/12/an_incomplete_list_of_go_tools/" rel="nofollow">http://dominik.honnef.co/posts/2014/12/an_incomplete_list_of_go_tools/</a></p></pre>SupersonicSpitfire: <pre><p>Run:</p>
<p>go fmt</p>
<p>go vet</p>
<p>golint</p>
<p>go test</p>
<p>Also build with:</p>
<p>go build -race</p>
<p>and test multiple concurrent requests with apache "ab" or the "gor" utility.</p></pre>nindalf: <pre><p>Another commenter mentioned middleware. I'd suggest you look into that and use the standard library where possible. Here are a <a href="https://github.com/nindalf/linkto/blob/master/middleware.go" rel="nofollow">few examples</a> and a <a href="https://medium.com/@matryer/the-http-handlerfunc-wrapper-technique-in-golang-c60bf76e6124" rel="nofollow">blog post</a> that explains the idea. A particularly useful one is replacing the ResponseWriter with one that automatically logs all responses.</p>
<p>You could replace <code>renderError()</code> and <code>renderNotFound()</code> with the standard library <code>http.Error()</code>. There are a couple of examples of using this in the first link. You don't need to explicitly set <code>http.StatusOK</code>, this is implicitly set if you call <code>w.Write()</code> without setting any other header.</p>
<p>I'd say that its probably a good idea to store state that is common to all your handlers in a struct. Here is <a href="https://github.com/nindalf/linkto/blob/master/main.go#L23" rel="nofollow">an example</a> of this pattern. Also, its probably best to rely on the standard http library alone and stick to the <code>http.HandlerFunc</code> interface since its so simple and powerful. If you need routing, use the excellent <a href="http://www.gorillatoolkit.org/pkg/mux" rel="nofollow"><code>gorilla/mux</code></a> library which is compatible with <code>http.HandlerFunc</code>. </p>
<p>Its a good idea to run <code>vet</code>, <code>goimports</code> and <code>golint</code> every time the file is saved. The GoSublime plugin for Sublime and go-plus plugin for Atom both do this. They would have helped you catch issues like exported methods/variables/types (ie, those starting with a capital letter) not having a comment explaining what they do.</p>
<p>Controllers is not a Go pattern. You'll find that Go code talks about routers, multiplexers, and request handlers, but not controllers. Here is a <a href="https://www.reddit.com/r/golang/comments/3b2qgg/a_strategy_for_architecting_a_golang_web/csif92y" rel="nofollow">comment</a> that talks about bringing conventions from languages like Ruby and Python to Go. I see that another commenter brought this up as well.</p>
<p>The directory structure of models and controllers is not optimal and better to keep the files in a flat structure. The exception is when the directories are separate libraries altogether.</p>
<p>Lastly, a minor nitpick not specific to Go. its probably better to write <code>1024*1024</code> instead of a "magic" number like 1048576, since it improves readability.</p>
<p>I hope you find these suggestions useful. Have fun coding :)</p></pre>BOSS_OF_THE_INTERNET: <pre><p>Make sure to add comments. People looking for documentation will thank you. It's readable without, but definitely better with comments. Example:</p>
<p><a href="https://godoc.org/github.com/shwing/moledro/controllers" rel="nofollow">https://godoc.org/github.com/shwing/moledro/controllers</a></p></pre>intermernet: <pre><p>The Go Report Card site (an entry into the Gopher Gala Hackathon) provides some nice supplementary data and suggestions for an initial, automated code review:</p>
<p><a href="http://goreportcard.com/report/shwing/moledro" rel="nofollow">http://goreportcard.com/report/shwing/moledro</a></p>
<p>It combines the output from go fmt, go lint, go vet and go cyclo (cyclomatic complexity) into a nice, easy to read GUI.</p>
<p>You can of course install and run these tools independently, but I find it's a nice "one-stop-shop" for a first pass at things you may not have known about, or obvious errors that have slipped through the net.</p>
<p>The output for go lint is probably the most important of the tools for your purposes.</p></pre>nokkare: <pre><p>This line doesn't do what you expect it to do:
<a href="https://github.com/shwing/moledro/blob/master/controllers/sub.go#L22" rel="nofollow">https://github.com/shwing/moledro/blob/master/controllers/sub.go#L22</a></p>
<pre><code>if err := json.NewEncoder(w).Encode(err); err != nil {
</code></pre>
<p>You could replace it with:</p>
<pre><code>type jsonError struct {
Error string `json:"error"`
}
if err := json.NewEncoder(w).Encode(jsonError{err.Error()}); err != nil {
</code></pre>
<p>The following is a matter of taste, but I find it nicer to partition your application per feature rather than technology. The reasoning for this is that developers/customers tend to code/speak features over technologies.</p>
<p>For example:</p>
<pre><code>github.com/shwing/moledro/cmd/webserver/main.go
github.com/shwing/moledro/cmd/apiserver/main.go # Just an example if the app publishes an API
github.com/shwing/moledro/sub.go # contains everything that relates to Sub: web, models, sql, etc
</code></pre>
<p>I would just package the main executable separately and start with one monolithic package and split new packages through organic growth rather than pure fabrication (model, controller). These technically split packages tend to split the conceptual objects into separate packages and the code will no longer reveal the model. It will also become harder to think and reason about in your mind.</p></pre>feelsmagical: <pre><p>Thanks, this is some great feedback.</p>
<p>What is the idiomatic approach for refactoring something like this: <a href="https://github.com/shwing/moledro/blob/master/controllers/handlers.go#L16" rel="nofollow">https://github.com/shwing/moledro/blob/master/controllers/handlers.go#L16</a></p>
<p>I've got more or less the same func 4x that take a different type in the argument. </p></pre>nokkare: <pre><p>Go handles this very differently from other languages. Some developers might find this really weird because it isn't something that they are used to (resistance is fu.*).</p>
<p>The most obvious solution is to refactor towards only one function that knows about all types. This approach is very fragile and therefore not a good idea.</p>
<p>There is a nice article at <a href="http://jordanorelli.com/post/32665860244/how-to-use-interfaces-in-go" rel="nofollow">http://jordanorelli.com/post/32665860244/how-to-use-interfaces-in-go</a> This article describes your case in detail.</p>
<p>Go's simplistic approach to this problem is that you have to write the code separately for each type. There is no shortcut to success. Perhaps you want a special case for User struct's password marshaling. That should be placed to User struct's member function and should not be placed in some generic function that knows about each type. This simplistic approach is truly the sweet spot for Go. I <3 it!</p>
<p>Also take a look at sort.Interface. It describes an approach to sorting where the sorted type doesn't matter!</p></pre>nokkare: <pre><p>Here's some sample code: <a href="http://play.golang.org/p/CG8JQePuAr" rel="nofollow">http://play.golang.org/p/CG8JQePuAr</a></p>
<p>I initially thought that you have to explicitly cast the marshal result, but Go handles that pretty transparently!</p>
<p>There's an intentional bug in the second JSON string.</p></pre>
