<p>Just wanted to thank the community for all for the feedback I had received from my <a href="https://www.reddit.com/r/golang/comments/5ar0wl/request_for_code_review/" rel="nofollow">last post</a>. </p>
<p>After a bit of reading I've decided to make a second attempt by refactoring my TODO app:</p>
<p><a href="https://github.com/Schweppesale/todo/tree/refactored" rel="nofollow">https://github.com/Schweppesale/todo/tree/refactored</a></p>
<p>Changes include a flattening of the existing directory structure and a separated api package for servicing the appropriate use-case.</p>
<hr/>**评论：**<br/><br/>shovelpost: <pre><p>All I can say is that this looks like normal Go code and it is much more readable especially compared to the previous Java-esque version. So basically from now on, most of the things you will see in this thread are probably nitpicks. Awesome job!</p>
<p>I am a little confused by your <code>satori</code> package. This looks like an external dependency to me since it imports <code>github.com/satori/go.uuid</code>. If that is the case then you <em>might have to consider vendoring it</em>. You can first start doing vendoring by hand to learn the procedure and once you are tired of it, you can use any of the available tools to do it easier. My personal favorite is <a href="https://github.com/FiloSottile/gvt" rel="nofollow">gvt</a> but there are many others. We will also hopefully get an official tool for this job. There is a team working on this.</p>
<p>In short, the folder <code>vendor</code> in your project is treated as special. Everything you place under it, you can import in your project AS IF you had done <code>go get github.com/satori</code> and placed this external package under <code>$GOPATH/src/github.com/satori</code>. You can try this right away if you use Go 1.6+ (if my memory serves correct). Copy all the source files from <code>$GOPATH/src/github.com/satori</code> and place them under <code>$GOPATH/src/github.com/Schweppesale/todo/vendor</code>. So the final path is gonna look like this: <code>$GOPATH/src/github.com/Schweppesale/todo/vendor/github.com/satori</code>. You can freely delete <code>$GOPATH/src/github.com/satori</code> now. If did you did everything correctly then your project should be compiling just fine. </p>
<p>Why do all this trouble? The source of <code>github.com/satori</code> basically lives in your project and will always be the same code you just vendored (unless you update it). Now if I want to use your package I can be sure that, no matter what, it's gonna work. Consider the alternative (aka no vendoring), then I might try to use your package and <code>github.com/satori</code> decided to do a breaking change in their code. So as soon as I <code>go get github.com/satori</code> (go get always brings the latest version) then I won't be able to use your project unless I fix the breaking change error that <code>github.com/satori</code> introduced in their codebase.</p>
<p>That said, you do not <em>have</em> to do vendoring. If your project is small and/or you are sure the package you are importing is very stable, you could just keep things simple. (In Google they use a monorepo, which is the logic behind $GOPATH/src but for the rest of us, it doesn't always work). It also depends on how you deploy and distribute your project. If you only deploy/share a single binary then you might not need vendoring but you also need to consider what other developers have to do to be able to use your project in case they want to contribute. My point is that, in the end, vendoring comes down to choice but it is something you have to learn to do at some point if you want to work with important Go projects.</p>
<p>This is summarized very well in Peter Bourgon's <a href="https://peter.bourgon.org/go-in-production/#dependency-management" rel="nofollow">article</a>.</p>
<p>EDIT: About Peter Bourgon's article, ignore the table with <code>_vendor</code>. This is an old article and things have changed since then but the first table is still very accurate.</p>
<p>I might find more nitpicks later but at first glance things look pretty good. You are improving at a <em>very</em> fast pace!</p></pre>allhatenocattle: <pre><p>Overall it looks a lot more idiomatic.</p>
<p>There is still some 'stuttery' names such as the TaskRepository interface has methods like FindTasks() GetTasksByUniqueID(). Since all it deals with are tasks, I'd consider renaming them without Tasks, so Find(), GetByUniqueID(), etc.</p>
<p>Looking at the logic of Find there might be a better name, since it doesn't actually filter anything, just returns them all.</p>
<p><a href="https://github.com/Schweppesale/todo/blob/refactored/infrastructure/memory/repositories.go#L31" rel="nofollow">https://github.com/Schweppesale/todo/blob/refactored/infrastructure/memory/repositories.go#L31</a>
There is an if/else statement with returns in both sections, you could remove the else clause (but keep the 2nd return statement).</p>
<p>In /infrastructure/repositories/memory/memory.go I think you could get rid of the keys field and just use a range statement on the tasks map when you want to call FindAll</p>
<p>Should use the mutex around the FindAll logic</p>
<p>I don't think you need an interface for the UUID generator</p></pre>shovelpost: <pre><p>I found some more time to check your code. Good to see you already did some refactoring that includes vendoring! Here are some more suggestions:</p>
<ul>
<li>Consider <a href="https://talks.golang.org/2014/names.slide#18" rel="nofollow">changing some names</a> in your code. For example:</li>
<li>Why does your Task need a field called <code>uniqueID</code>? Are you planning to include some kind of non unique ID later? I think just ID is enough to describe the field.</li>
<li>I am glad you used Getters with the Go naming (without Get in front) but in this case, do you really need getters?</li>
</ul>
<p>You could consider something like this (notice everything is exported):</p>
<pre><code>type Task struct {
ID string
Title string
Description string
CreatedOn time.Time
UpdatedOn time.Time
}
</code></pre>
<ul>
<li><p>Personally I might also go for <code>Created</code> instead of <code>CreatedOn</code> to keep it short and sweet but this is personal preference.</p></li>
<li><p>Similarly, do you really need the <code>func NewTask(title string, description string) Task {</code> constructor? </p></li>
<li><p>Again this leans towards the personal preference side, but I've found that in Go instead of keeping our "models" fat and smart like we sometimes do when writing Java, it seems better to keep them as simple, lean structs with the only purpose of passing around data.</p></li>
<li><p>So yeah, I get what you are going for with the private fields. The client can only make a new task using your constructor <code>NewTask</code> which will guarantee that your Task will have dates as well. But you might want to let the creator of the task worry about such details. For example:</p></li>
<li><p><code>task := &Task{Title: "My first task", Description: "Blah"}</code> This will allow anyone to quickly make a new Task (pointer in this case). The functions which are responsible for storing the Task in the database should be the ones to worry about filling in the <code>CreatedOn</code> field.</p></li>
<li><p>Following the same logic, you do not really need Setters either. All this embedded logic in your Setters looks troublesome to me. I mean sure I get it, each time you modify a field you want to make sure that <code>UpdatedOn</code> stays up to date. Well, let the function that actually updates your Task worry about that stuff.</p></li>
<li><p>Also consider some better comments on each exported variable. They need to be complete sentences that end with '.' and it is much better instead of explaining the obvious, <a href="https://youtu.be/PAAkCSZUG1c?t=19m7s" rel="nofollow">to explain what they are used for</a>.</p></li>
<li><p>Your <code>logger</code> package seems unnecessary to me. It is a neat idea actually that your logger package knows how to create a <code>TaskRepository</code> but it leads to so much extra code for nothing. Logging is something that the final user of <code>TaskRepository</code> should use. Why embed "the logic" of logging directly in your <code>TaskRepository</code> functions? </p></li>
<li><p>Many people say that it is a good practice to keep logging as a dependency. Consider the final "Service" that uses your code (I think you've named it Server). So your <code>Server</code> should be responsible for doing the actual logging.</p></li>
<li><p>On the topic of logging, as far as I am concerned, you better stick to logging only what is important. This is very a controversial topic actually. Some people insist of logging everything and even use logging levels. Maybe this is the right way to do it on a large and complex system but in your case, make sure that you only log what is really important and that the actual logging is done by the final user of your <code>TaskRepository</code>.</p></li>
<li><p>Speaking of your <code>TaskRepository</code> interface, since this is a small application and does not use dozens of different repositories, maybe you could rename it <code>Tasker</code> or something short and sweet like that! I don't know but <code>NewTasker</code> sounds much better to me than <code>NewTaskRepository</code> but I digress.</p></li>
<li><p>If you want something "less heavy" than Mongo, consider using <a href="https://github.com/boltdb/bolt" rel="nofollow">Bolt</a> which is written in Go. It looks ideal for a project like this and you could go as far as using it in production too.</p></li>
<li><p>I am not sure how I feel about your <code>api</code> package. On a first read, it looks to me as if it recreates the code that would belong in the <code>memory</code> package.</p></li>
</ul>
<p>There might be more stuff you can improve but I am gonna stop here. Again I want to emphasize that most of the things I mentioned above are just suggestions which can come down to personal preference. Use what works best for you.</p></pre>
