<p>At the company where I work (small company), I am the most senior of only two Go web developers. As a result, it is difficult for me to get quality code reviews of my work. I mainly have to base what I do off examples (from within the standard library and online).</p>
<p>I thought that perhaps one way I could address this was through getting feedback for an example of my work online. So, I built this: <a href="https://github.com/kiwih/heyfyi">https://github.com/kiwih/heyfyi</a></p>
<p>You can access it online at <a href="http://hey.fyi">http://hey.fyi</a></p>
<p>This is a very small but complete demo web application written in Go. It is aimed at being a resource for new Go developers. I was hoping that you guys at this subreddit could look through it and tell me if I have any code smells - so at the end of the day I both have learnt something and also had a chance to contribute to the amazing Go community.</p>
<p>I am aware that sqlite is a poor choice DB for online deployment, and I know that my Javascript has some problems too. I'm mainly looking for feedback on my architecture / go code / security.</p>
<p>Thanks for any responses! </p>
<hr/>**评论：**<br/><br/>kawera: <pre><p>In server.go, the session store key is hardcoded. Maybe a good ideia to change that.</p>
<p>Purely by convention, static files(css...) goes into a folder named "static" and not "files".</p></pre>kiwihammond: <pre><p>I wasn't sure if hardcoding the session store key was too worrisome, given that I only use it to store (via a cookie) a second randomly-generated session ID for logged in users. What do you think?</p>
<p>As for folder name, that's easy enough to change. </p>
<p>EDIT: Moved the session store key into an environment variable.</p></pre>izuriel: <pre><p>As some very quick (I haven't read a lot) tidbit on some things I saw stand out at me. This isn't me thinking you've done anything wrong with any of this, just some things I don't know if you're ware of not. First, is you can safely omit the type name when defining slices, mostly in your test function (I've looked at a shocking total of 1 file so far) and you have some code like:</p>
<pre><code>SpiderFact := fact.Fact{
// Omitted for brevity...
References: []fact.References{
fact.Reference{
// Omitted due to brevity...
},
},
}
</code></pre>
<p>You can simply write that as:</p>
<pre><code>SpiderFact := fact.Fact{
// Omitted for brevity...
References: []fact.References{
{
// Omitted due to brevity...
},
},
}
</code></pre>
<p>Which <em>may</em> or <em>may not</em> be a good choice. I just saw that you didn't do it so I wanted to point it out in case it's not something that you're familiar with. And I think go 1.5 actually added this support in for maps, which was previously missing the functionality while slices have had it.</p>
<p>That's more of just an FYI, not any real suggestion for improvement. I, personally, will look over and see what I think although I feel like you're probably more experienced in general than I am as I'm only working in Go for personal projects at the moment. But I figure I can at least potentially learn something from the project!</p></pre>kiwihammond: <pre><p>I actually didn't know that! Cheers!</p></pre>syn_ack: <pre><p>If you wanted to make it completely self-contained you could use <a href="https://github.com/cznic/ql">ql</a>, however I've just tried to do a quick replacement and it's throwing errors --- to do with the sql-dialect and the gorm <-> db mapping and not your code.</p>
<p>It may be more effort than it's worth. Especially if the plan is to use it for production.</p>
<p>I should say, I'm going to find it useful to have this sample to work from for a small pet project I'm working on at the moment!</p></pre>kiwihammond: <pre><p>I'd not heard of this. What's the performance like for it?</p></pre>syn_ack: <pre><p>I've got no idea. I can't even find any reports stats online either. I've not really looked into performance (I don't use go professionally so it doesn't matter to me at the moment). </p></pre>znpy: <pre><p>Hey!</p>
<p>This is a very good idea and I might do the same in the future!</p>
<p>In the meantime, I modified some code and made a pull-request!</p>
<p><a href="https://github.com/kiwih/heyfyi/pull/3" rel="nofollow">https://github.com/kiwih/heyfyi/pull/3</a></p>
<p>I see you have parameters hardcoded in the source, and a better way to do that is to have parameters as environment variables: <a href="http://12factor.net/config" rel="nofollow">http://12factor.net/config</a></p>
<p>So basically what I did is:</p>
<ul>
<li>Add a .gitignore file ignoring _ENV.sh</li>
<li>Add an ENV_SAMPLE.sh file</li>
<li>Add code to read environment variables</li>
</ul>
<p>So now you can have a file named DEV_ENV.sh in your source tree on your development machine with all your parameters, and customize it without worrying of configuration getting shared in the commits. There are some great horror stories of people publishing ssh keys and/or API keys in the form of publicly readable git commits on github.</p>
<p>When you're starting development, make sure you run: <code>source DEV_ENV.sh</code> to load all the environment variable from the <code>DEV_ENV.sh</code> file :)</p>
<p>Example (from the pull request):</p>
<pre><code> /* On platforms like heroky/dokku this should be PORT, not HTTP_PORT. */
HTTP_PORT = os.Getenv("HTTP_PORT")
if len(HTTP_PORT) == 0 {
log.Println("$HTTP_PORT was not set, defaulting to 3000")
HTTP_PORT = "3000"
}
</code></pre></pre>thorhs: <pre><p>I would rather suggest getting the settings from the command line. There is a nice command line parser on github that can default to envy variables, which is nice (<a href="https://github.com/codegangsta/cli" rel="nofollow">https://github.com/codegangsta/cli</a>). I've used it with great success and I totally recommend it for this kind of stuff. </p></pre>lapingvino: <pre><p>you can actually do that: HTTP_PORT=8080 yourcommandhere</p></pre>znpy: <pre><p>Yes, but:</p>
<ol>
<li><p>That functionality is actually provided by the shell, not go (this is fine, just pointing this out).</p></li>
<li><p>If doing this, altering the configuration may require an edit of the Procfile (and a totally unneeded commit). Configuration is not code.</p></li>
</ol></pre>lapingvino: <pre><p>True.</p>
<ol>
<li>You could do both if really necessary.</li>
<li>I'm a big proponent of sensible defaults ;).</li>
</ol></pre>znpy: <pre><p>Passing arguments on the command line is cool, but it can be troublesome on platforms like Heroku/dokku and similar.</p>
<p>One of the key advantages of using environment variables is that you can set them outside of the codebase and just restart (or in some cases even reload) the application.</p></pre>kiwihammond: <pre><p>I agree with this - I use managed Azure with my company, and that passes in stuff from environment variables. At the moment, the arguments (such as the server address) come from a flag (or it should have done, the fact that it hadn't was a bug). Just looking at your change, I spotted this though:</p>
<pre><code>if len(COOKIE_STORE_SALT) == 0 {
log.Fatal("$COOKIE_STORE_SALT was not set, cannot go on. Exiting.")
} else {
store = sessions.NewCookieStore([]byte("9s7YD807h*&DHhihSD123434SASDD__834HUSJNCxczc123!@#sd85"))
}
</code></pre>
<p>That doesn't actually use the env variable! I'll accept then modify it to.</p>
<p>EDIT: Done this, and moved the environment variable loading into the run package.</p></pre>znpy: <pre><p>Yes, that was my mistake. It should have initialized the <code>store</code> object using the <code>COOKIE_STORE_SALT</code> variable.</p>
<p>Glad you spotted it!</p></pre>izuriel: <pre><p>To go along with this line of thought I found the viper library to be useful which enables configuration through file, command line args and the environment with clear rules on which ones win out in the end. Not too difficult to configure either. </p></pre>zeroXten: <pre><p>I'm a relative Go noob, especially when it comes to webdev. Is it usual to put the main() function in a subdirectory? I was trying to find it as a starting point, but you don't have a main.go in the root of the project and after a bit of looking around I found it in /run. Seems a bit strange to me.</p></pre>kiwihammond: <pre><p>It's something we started doing at our company. There are three main reasons, at least for me -</p>
<p>a) You can use the loader package to hold non-go things, i.e. templates, static files, etc. This is just an organisation thing, and mostly personal preference.</p>
<p>b) Your loader can pass in the arguments and it's easy to change how this is done (see the latest commit - the run package loads the environment variables instead of the heyfyi package)</p>
<p>c) We like to use GoDoc on all of our code! You can't (to the best of my knowledge) use GoDoc on a 'package main'. So, by having a different package main, then we can use GoDoc on them. </p></pre>QThellimist: <pre><p>Check out pgweb. Its the most beutiful code I've ever seen both in frontend and backend. </p>
<p>Not saying it's the best approach but you can and imo you should put main.go in the most upper level. It makes everything easier for the new lookers</p></pre>kiwihammond: <pre><blockquote>
<p>pgweb</p>
</blockquote>
<p>It does seem quite clean! For Azure, we have to make the most upper level package main, but I find when developing non-Azure projects I swap back to this layout (I just find it cleaner for some reason!) </p>
<p>It does seem that more people do it the other way around though, so I will bear that in mind.</p></pre>
