<p>TL;DR <a href="https://github.com/ptqa/resource_manager">https://github.com/ptqa/resource_manager</a></p>
<p>Hi there. I'm new to golang (did 1 path in go before that) and I'm not a developer, but I've tried to implement test challenge I've found in the internet. Basically it's REST API for accessing array of structures.</p>
<p>I'm creating workers (goroutines) and send tasks for them using hashring, so I can run add/remove resources concurrently. I also use gin gonic as router. main() function is inside resm.go, code is mostly inside resources.go. I've tried to create tests with goconvey, not sure if did it right.</p>
<p>Any feedback is welcome.</p>
<hr/>**评论：**<br/><br/>613style: <pre><p>Some of these are personal style, but most are idiomatic:</p>
<ul>
<li><p>Consider renaming resm.go to main.go, then you won't have to tell people where your main function is.</p></li>
<li><p>Message type is declared in resm.go but not used there. It's also exported but none of its members are. Declare it where it's used and comment "ch" to tell readers what it's for or name it something more descriptive.</p></li>
<li><p>There's no need for comments like "Init arr" or "initialize resources." Instead, add a lot more comments to resources.go. Assume your reader understands what the Go language does, and focus on <em>why</em> you're doing things instead of <em>what</em> you're doing. Every function should have a comment directly above it that describes it and starts with the name of the function. Don't be afraid of multi-line comments.</p></li>
<li><p>The error message "Failed" could be more precise. Consider defining all possible errors at the top of the file, and "return errFailedToAllocate" or something similar. Typing the same error string "Failed" from multiple functions sets yourself up for a making a typo and not noticing.</p></li>
<li><p>Why is your resources variable named arr? Maybe just name it resources. Then it's easier to read.</p></li>
<li><p>Prefer more small functions over fewer big functions. In main, each of those routes could be a function. Can you break up any of the functions in resources into smaller functions?</p></li>
<li><p>Put comments for members of the Resources type inside the type instead of above it.</p></li>
<li><p>Add comments at the top of files to tell readers what the file does.</p></li>
<li><p>A function shouldn't modify the value of its arguments (when they're not pointers). In tryDeallocate, prefer "internalID := id-1" or "Resource{id-1,...}" over mutating id and then using it directly.</p></li>
<li><p>Functions that are exported should start with capital letters. The ones in resources.go should be made more consistent.</p></li>
<li><p>Structure elements that start with capital letters are exported. Should all members of Resources be exported?</p></li>
<li><p>Prefer "func (r *Resources)" over "func (a *Resources)"</p></li>
<li><p>Don't write your own JSON generating code. You already used the standard ones in config.go, so also use them in resources.go.</p></li>
</ul></pre>ptQa-: <pre><p>Thank you very much. I did JSON generation this way, because I thought that converting my array to map and then to JSON would be inefficient.</p>
<p>I'll fix everything else you've mentioned. </p></pre>613style: <pre><p>There's a saying: first make it work, then make it right, then make it fast. When you worry about optimizing for speed early in the process, it often doesn't make any real difference but it creates an opportunity for bugs and makes it harder to read. </p>
<p>The other problem is you may not have enough information to make a good decision about which is faster. For example, you use one += for each element of the a.members. Is that allocating a new string each time you do it? If so, it's probably a lot slower than converting once to a map. The normal way to optimize such a loop would be create a vector of bytes ahead of time, then fill it up by remembering the index of the last byte you wrote and continuing each time from there, using the copy() call. Then there's only 1 allocation.</p>
<p>IE:</p>
<pre><code>buf := make([]byte, someBigEnoughSize)
lastPos := 0
for i := 0; i < len(a.members); i++ {
lastPos += copy(buf[lastPos:], convertToString(a.members[i]))
}
</code></pre>
<p>This isn't JSON, but the idea is there. If you're satisfied with log2(n) allocations, then bytes.Buffer simplifies it. But you still shouldn't do it even this way. Go's standard library can serialize an array to JSON directly.</p>
<pre><code>numbers := []int{1, 2, 3, 4, 5}
jsonBytes, _ := json.Marshal(numbers)
</code></pre>
<p>Which code block can you understand at a glance, and be confident has no bugs at a glance?</p></pre>IntellectualReserve: <pre><p>If you had an open pull request, I could do it from my phone right now.</p></pre>robvdl: <pre><p>Another thing about commenting exported functions, structs, etc. I use Atom and Go Plus plugin and it recommends me you should start the comment with the name of the thing that is being commented.</p>
<p>e.g.</p>
<pre><code>// Init is a function that initializes the application
func Init() {
}
</code></pre>
<p>Note that the comment starts with "Init", if I don't do this, the Go Plus atom plugin gives me a warning.</p></pre>
