<p>Hey Gophers,</p>
<p>I've gotten pretty carried away with Golang since picking it up a few weeks ago, and I feel like it's time for a sanity check.</p>
<p>If anyone has time, I'd really appreciate a quick "code review" of sorts. Basically I just want to make sure I'm not doing anything majorly stupid. The project is a stripped down reddit clone btw.</p>
<p>Repo: <a href="https://github.com/brwhale/GoServer?files=1">https://github.com/brwhale/GoServer?files=1</a></p>
<p>next steps are to make an interface for the data layer so I can mock it and do tests.</p>
<hr/>**评论：**<br/><br/>alecthomas: <pre><p>Hello! Overall, code is pretty reasonable. I think the biggest problems are globals, and use of <code>util.Check(err)</code>.</p>
<ul>
<li>Outside of your <code>main()</code> function you should almost never have code like <code>util.Check(err)</code>. Code like that takes control flow away from the caller. This is not good for refactoring, reusability or testing. In general you should <em>always</em> return errors.</li>
<li>You have a number of of globals, eg. <code>database.db</code>, <code>database.sc</code>. These are almost never a good idea. They complicate testing, reuse, and reasoning.</li>
<li>Your handlers use globals indirectly via <code>.Insert()</code> methods, etc. You can avoid this in two ways: make your handlers be member functions. eg. Instead of <code>func Boards(w http.ResponseWriter, r *http.Request)</code> have <code>func (c *Controllers) Boards(w http.ResponseWriter, r *http.Request)</code> where <code>Controllers</code> is a struct with a <code>*db.DB</code> member. Secondly, change all your database access functions to accept a <code>*db.DB</code> parameter and use that rather than the global. You could also wrap <code>*db.DB</code> in your own <code>Database</code> type with these functions as convenience methods. eg. <code>db := database.New(...); db.GetComment(id)</code>. You could then use this in your <code>Controllers</code> struct instead of a <code>*db.DB</code> directly.</li>
<li>Methods on structs should almost always<sup>1</sup> have a pointer receiver. ie. <code>func (c *Comment) Insert()</code> rather than <code>func (c Comment) Insert()</code>. Why? Because Go passes by value, and the latter will receive a complete copy of the Comment. This is a) slow and b) confusing when you attempt to modify the receiver, as the modifications will not stick. </li>
<li>Similar to the above, struct fields, and slice elements that are structs should almost always be pointers to structs. ie. <code>Comments []*Comment</code> not <code>Comments []Comment</code>. <sup>1</sup></li>
<li>Use <code>log.Print*</code> rather than <code>fmt.Print*</code>in modules. Only use <code>fmt.Print*</code> in your main function, for the operator.</li>
<li>In some places (like <a href="https://github.com/brwhale/GoServer/blob/master/database/securecookie.go#L21">this</a>) you are not checking errors. Use a static analyser like <a href="https://github.com/kisielk/errcheck">errcheck</a> (or just use <a href="https://github.com/alecthomas/gometalinter">gometalinter</a> - shameless plug!) to pick up issues like this, among others.</li>
</ul>
<hr/>
<ol>
<li>There are <em>occasionally</em> reasons to not use pointer receivers, but when starting out with Go it just adds confusion. Stick to a simple rule to start with: <em>always pass structs by pointer, pass everything else by value (slices, maps, string, ints, floats, etc.)</em>.</li>
</ol></pre>snerp: <pre><p>Ahh! Awesome, that pointer receiver thing actually clears up a big point of confusion, now I should be able to make GetChildComments 3n time instead of the recursive function which takes god knows how long.</p>
<p>Also, the globals were intended to become injected dependencies, but I wasn't sure the best way to go about it in Go. This was actually my main reason for this thread. Big thanks for the description!</p>
<p>In a few places, like inserting the securecookie, I didn't check the error because I was treating those calls as "fire and forget", in these cases is the best practice to log the error and move on?</p>
<p>Thanks super hard by the way!!! This is amazingly helpful.</p></pre>alecthomas: <pre><blockquote>
<p>In a few places, like inserting the securecookie, I didn't check the error because I was treating those calls as "fire and forget", in these cases is the best practice to log the error and move on?</p>
</blockquote>
<p>Yes, exactly. Sometimes, even if you don't expect something to fail, it can, and you want to know about it when debugging.</p></pre>titpetric: <pre><p>@alecthomas if it's not too much trouble, would you mind taking the same time for the book example <a href="https://github.com/titpetric/books/tree/master/12fa-docker-golang/chapter7" rel="nofollow">here</a>. I would appreciate that very much.</p></pre>alecthomas: <pre><p>Sure thing!</p>
<p>Overall looks fairly reasonable, just a few minor comments.</p>
<ul>
<li>You generally never need to use <code>make()</code> unless you're setting capacity. Prefer eg. <code>msgs := []*TwitterMessage{}</code> over <code>msgs := make([]*TwitterMessage, 0)</code></li>
<li>Group your imports <code>import (\n"a"\n"b"\n)\n</code></li>
<li><a href="https://github.com/titpetric/books/blob/master/12fa-docker-golang/chapter7/common/redis.go#L14" rel="nofollow">This</a> type can simply be <code>type RedisOption func(*Redis)</code>.</li>
<li>Typically, structs are populated when they're created, rather than settings attributes after the fact like you have <a href="https://github.com/titpetric/books/blob/master/12fa-docker-golang/chapter7/common/redis.go#L52" rel="nofollow">here</a>. Prefer <code>v := &mystruct{f: 1, g: 2}</code> over <code>v := &mystruct{}; v.f = 1; v.g = 2</code>. Also, there's no need to set conn to nil, as that is the default "zero" value for pointers, interfaces, maps, and slices.</li>
<li>It's not clear what the purpose of <a href="https://github.com/titpetric/books/blob/master/12fa-docker-golang/chapter7/common/redis.go#L65" rel="nofollow">this</a> method is?</li>
<li>For your <code>Register</code> methods, I would probably pass a <code>*http.ServeMux</code> in, to make them more reusable. The stdlib has a bunch of global variables for making life easier, but you (generally) don't want to rely on them in production code.</li>
</ul></pre>titpetric: <pre><p>I wouldn't say anything you do it stupid, but I would add some things to make my life easier.</p>
<p>I tend to keep packages self contained. You seem to know what MVC is (in terms of general principle), but you seem to mix the "model" part with whatever is in your database package. On paper, the database package could be reusable between many projects, but in reality it's tightly coupled to your app and doesn't stand on it's own feet. This is why a lot of Go developers seem to recommend just keeping with a no-packages single repo. My advice to you would be to split the actual model part to be more tightly coupled with what you have in handlers.</p>
<p>I'd suggest to go with <code>user.go</code> and <code>user_http.go</code> to separate the model part (database/user) and the http handler (controller/user). Either way, I'd move user.go to the controllers.</p>
<p>As an example I can link to a book sample from my 12FA with Docker and Go book: <a href="https://github.com/titpetric/books/tree/master/12fa-docker-golang/chapter7" rel="nofollow">https://github.com/titpetric/books/tree/master/12fa-docker-golang/chapter7</a> (handlers are in api/, self-contained service clients are in common/ - the naming is poor, but sufficient).</p>
<p>There's also the question of how you handle configuration. While I can't say I found "the best" way, generally the best argument is for your code to be testable. This means that the configuration for a database connection should come from main() - either with environment or like the example, using <code>namsral/flag</code> (environment/command line flags) or more complex configuration libraries like viper, which also support reloading - <a href="https://github.com/spf13/viper" rel="nofollow">https://github.com/spf13/viper</a> - something like this makes your code testable, meaning you can supply configuration from your test code, and not rely on outside files to get credentials from.</p>
<p>I know there's something about my example feels wrong - mainly because the Redis struct handles not only the connection to Redis, but also "Save", storing of this connection to be made available to api handlers with <code>GetRedis</code>. The "better" way would be to split redis into two parts - a redis connection pool and a redis client. The redis client is by itself nothing else than that. main() would create the connection pool and add the default redis connection, while apis would just get the active (or even named!) connection struct from the pool. The alternative is somehow to make all the connection configuration accessible to apis, however without structuring them (which is possible), I would suspect that it's just more verbose and copy/paste. Duplicating connection setup code in every api is not what I feel comfortable doing, so I'll probably refactor to pool/client I'm mentioning. When it comes to testing, you'd just set up a Mock client into a pool and run your data layer functions as you would.</p>
<p>Finally, I'd suggest going with a front-end mvc like Angular or Vue (my vote goes for Vue due to size/docs/simplicity). Go templating while rich is just slower in terms of development time, and templates are quite tightly coupled with what you get from Go code. With Vue you get a richer front-end layer that is popular enough to get developers for it. Imagine you actually built your own Reddit as a company - it's worth it already to enable a different calibre of developer to join your team and split some responsibilities between front and back-end. Maybe it's also my preference because as a Go developer I don't like to mess with HTML in Go code, and vice-versa as a HTML developer I don't want to mess with Go code. Having API driven development practices pays for itself twice over even before the second developer joins the team.</p>
<p>I'm drifting into non-code review territory, so this would be a good end to the comment :). Good luck with development in Go, it's really really great. Far more rewarding than Node, anyway :)</p></pre>snerp: <pre><p>Cool, thank you for the in depth reply! I'm excited to explore your suggestions.</p>
<blockquote>
<p>With Vue you get a richer front-end layer that is popular enough to get developers for it. Imagine you actually built your own Reddit as a company - it's worth it already to enable a different calibre of developer to join your team and split some responsibilities between front and back-end.</p>
</blockquote>
<p>Yeah, I think if I was planning on hiring someone I would use Angular or something. I just personally do not like Javascript, at all, so I'm trying to get away with as little as possible. I'm experimenting with using Go Templates as Components, which has been working ok so far.</p></pre>titpetric: <pre><p>In fairness, I wrote a PHP template engine (<a href="https://github.com/titpetric/minitpl" rel="nofollow">https://github.com/titpetric/minitpl</a>) and I like ERB style template engines (or EJS in javascript) which expose the full language syntax with which you're working with. I tried egon in Go for a similar approach (and even patched it up a bit <a href="https://github.com/titpetric/egon" rel="nofollow">https://github.com/titpetric/egon</a>) but what it does is compile templates with external tooling which I'd love to be able to hook into a simple <code>go run</code> or <code>go build</code>. I realize that Go is a language that compiles, but coming from a PHP/JS background where "anything" goes, the templating is too restrictive for me. So I tend to keep it where it is flexible, and that is Vue/JS/Front-end in this case. Anyway, I am not very much bothered about it, I seem to have resolved it pretty well, and I ported much of my MiniTPL syntax to EJS as well, so most of the time I'm not even learning a new template syntax - which is why I stay away from things like MustacheJS and similar. Pseudo-templating languages are the worst, and I can only wish to have either the same templating language everywhere, or exposing each programming language primitives and syntax in full, which Go unfortunately doesn't give me with their template packages :(</p></pre>comrade-jim: <pre><p><a href="https://blog.gopheracademy.com/advent-2016/exposing-go-on-the-internet/" rel="nofollow">https://blog.gopheracademy.com/advent-2016/exposing-go-on-the-internet/</a></p>
<p>This article indicates that a production server should have the timeout parameters of the http.Server struct set. Maybe you did this and I missed it. </p>
<p>Go 1.8 will support graceful http shutdown with context IIRC, which is also essential imo. </p></pre>snerp: <pre><p>Awesome! I was not doing this. I added timeouts and basic TLS, I think I'll wait for 1.8 to set up the rest. Thanks!! </p></pre>
