<p>Trying to learn Go. Love getting feedback about my projects, etc.. Threw up a simple project on GitHub just for the sake of getting feedback.</p>
<p>Project is supposed to be a web server that takes in JSON, creates or deletes data in MySQL, and responds with JSON.</p>
<p><a href="https://github.com/GarrettSYHampton/GoPersonAPI">https://github.com/GarrettSYHampton/GoPersonAPI</a></p>
<hr/>**评论：**<br/><br/>PsyWolf: <pre><p>No tests? Why no tests? Why you no like tests? You make tests sad. :-( ... Seriously though, writing tests in Go is a joy. <a href="https://smartystreets.com/blog/2015/02/go-testing-part-1-vanillla">Here's a good blog</a> post to get you started.</p>
<p>Also, out of curiosity, why did you split your person into a bunch of different files? Most gophers would put that in a single file.</p>
<p>P.S. If you wanna get fancy, there are some really neat advanced testing tools like <a href="https://github.com/smartystreets/goconvey/">GoConvey</a> (check out this sweet <a href="https://www.youtube.com/watch?v=wlUKRxWEELU">demo video</a>), but I recommend starting out with the more standard "go test" and working your way up.</p></pre>GarrettSYHampton: <pre><p>Added a test for "Create" and "Delete"! Anything special about the tests I should know?</p>
<p><a href="https://github.com/GarrettSYHampton/GoPersonAPI/blob/master/src/person/crud_test.go" rel="nofollow">https://github.com/GarrettSYHampton/GoPersonAPI/blob/master/src/person/crud_test.go</a></p></pre>terrason: <pre><p>You probably don't want to share any variables between those tests. The tests are not guaranteed to run in order, and tests should not depend on any external state.</p>
<p>For your delete test, you should instead test by running the method on an instance of Person you create in the test, typically a configured literal.</p>
<p>If you need tests to run in order (which is not the case here, to be clear), check out <a href="https://godoc.org/testing#hdr-Main" rel="nofollow">Main</a>.</p></pre>flynnguy: <pre><p>In addition to GoConvey, I'm a big fan of the assert package in <a href="https://github.com/stretchr/testify" rel="nofollow">testify</a>.</p></pre>Yojihito: <pre><blockquote>
<p>Also, out of curiosity, why did you split your person into a bunch of different files? Most gophers would put that in a single file.</p>
</blockquote>
<p>I find that horrible, you end with a 400+ LOC file with no structure. I prefer the Java way, 1 file per "class".</p></pre>PsyWolf: <pre><p>I'm right there with you. OP originally had a single person "class" with 3 small methods split across 4 files. That's significantly more broken up than the 1 file per class "java style" that you're talking about.</p></pre>Growlizing: <pre><p>I would also like to recommend GoConvey, but start with just plain 'go test' for a few weeks =)</p></pre>tucnak: <pre><p>Use <a href="https://github.com/alecthomas/gometalinter">https://github.com/alecthomas/gometalinter</a></p></pre>intermernet: <pre><p>Some initial suggestions, in addition to those provided by others:</p>
<ul>
<li><p><code>Model</code> should probably be <code>Person</code> (It will make the code easier to read when used by others)</p></li>
<li><p><code>os.Getenv</code> will return an empty string if the environment variable doesn't exist. You could either check for these and provide feedback where appropriate, or, as this is a package, provide a "configuration" struct in which to define these. Or you could allow users to provide the full connection string, or even the MySQL connection returned by <code>database, err := sql.Open("mysql", d)</code>. Packages should probably not rely on environment variables.</p></li>
<li><p>Packages should probably not use the <code>log</code> package in most instances. Instead, return errors with descriptive strings that can be logged by code that includes the package. Pass the info up the chain!</p></li>
</ul>
<p>Hope that helps!</p></pre>GarrettSYHampton: <pre><p>Good call on "Model". Renamed it to "Person".</p>
<p>Replaced my os.Getenv with a "config.go" that checks for these variables on init, and panics if they don't exist.</p>
<p>Removed "log" import from my package. Makes total sense. Good call!</p></pre>barsonme: <pre><p>server/create.go and server/delete.go seem to share a lot of code that is unnecessarily duplicated.</p>
<p>server/routes.go doesn't use field names in a struct literal. It's allowed, but probably shouldn't be done because it can cause subtle bugs.</p>
<p>Comments should also explain <em>why</em> or explain when you do something that's not blatantly obvious. Comments like this:</p>
<pre><code>// Parse data from request
if err := json.NewDecoder(r.Body).Decode(&req); err != nil ...
</code></pre>
<p>Just clutter the code. It's pretty obvious what's going on.</p>
<p>See my PR for more reasons why your Go code sucks :-)</p></pre>GarrettSYHampton: <pre><p>Thoughts on how to de-duplicate server/create.go and server/delete.go? Originally I had two functions, one that returned an interface from the JSON request, and another that printed JSON response but our dev team told me to pull it.</p></pre>barsonme: <pre><p>The biggest issue was the anonymous structs which were shared between files. Other than that, I wouldn't pull to much out of the handlers because it'd over complicate the code.</p></pre>climbinglepton: <pre><p>I agree with barsonme, the anonymous structs are ugly. Just declare a struct. You can just turn that create.go/delete.go into one unexported function and then make Create and Delete functions that call that function as appropriate. Repeated code like that makes your project impossible to maintain as it grows. I want to do horrible things to some of the people I work with because of Ctrl+C/Ctrl+V code.</p></pre>GarrettSYHampton: <pre><p>Originally, I had "too much" code abstracted, and our dev team told me to tear it all down. Having a hard time figuring out where to "draw the line in the sand" with regards to inline vs abstracting.</p></pre>GarrettSYHampton: <pre><p>Thanks for the input! Anon struct removed and replaced.</p></pre>
