<p>Hi,</p>
<p>I'm new to golang and want to progress. Can you please review my code ?</p>
<p><a href="https://github.com/snwfdhmp/duck">https://github.com/snwfdhmp/duck</a></p>
<hr/>**评论：**<br/><br/>klauspost: <pre><p>Welcome to Go! Good to see you jumped straight in!</p>
<p>With this first experience behind you, take some time to look through some of the best practices out there. The basic ones are these:</p>
<ul>
<li>Must read: <a href="https://golang.org/doc/effective_go.html">https://golang.org/doc/effective_go.html</a></li>
<li>More good practices: <a href="https://github.com/golang/go/wiki/CodeReviewComments">https://github.com/golang/go/wiki/CodeReviewComments</a></li>
</ul>
<p>You can find much more out there by googling for it. I am sure other people also have some helpful links. Youtube also has a lot of great talks.</p>
<p>Here are some of the most obvious things that jumped out when skimming the code. They are pretty basic, but also very common when people move to Go from other languages:</p>
<p>Usually main functions are placed in <code>cmd/duck</code> folder, shared packages as directories in the root, or in 'pkg', but you have a lot of "extra" stuff in your repo. Drop the in-repo binary/build.</p>
<blockquote>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/src/duck.go#L5">https://github.com/snwfdhmp/duck/blob/master/src/duck.go#L5</a></p>
</blockquote>
<p>Don't use relative imports.</p>
<blockquote>
<p><a href="https://github.com/snwfdhmp/duck/tree/master/src/usage">https://github.com/snwfdhmp/duck/tree/master/src/usage</a></p>
</blockquote>
<p>Keep this where it is used, ie main package.</p>
<blockquote>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/src/parser/parser.go#L19">https://github.com/snwfdhmp/duck/blob/master/src/parser/parser.go#L19</a></p>
</blockquote>
<p>Go documentation does not have special tags. Write as plain text what it does. See the standard library for how to write documentation.</p>
<blockquote>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/src/configuration/configuration.go#L86">https://github.com/snwfdhmp/duck/blob/master/src/configuration/configuration.go#L86</a></p>
</blockquote>
<p>Return "error" instead of bool.</p>
<blockquote>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/src/configuration/configuration.go#L127">https://github.com/snwfdhmp/duck/blob/master/src/configuration/configuration.go#L127</a></p>
</blockquote>
<p>This and many, many other places you are ignoring errors. Don't do that. Search for 'errcheck' for a tool that will show them to you.</p>
<blockquote>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/src/configuration/configuration.go#L50">https://github.com/snwfdhmp/duck/blob/master/src/configuration/configuration.go#L50</a></p>
</blockquote>
<p>Instead of storing configuration as a package level variable, return it to the caller.</p>
<blockquote>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/src/configuration/configuration.go#L438">https://github.com/snwfdhmp/duck/blob/master/src/configuration/configuration.go#L438</a></p>
</blockquote>
<p>No need for getters - all these are directly available.</p>
<blockquote>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/src/configuration/configuration.go#L454">https://github.com/snwfdhmp/duck/blob/master/src/configuration/configuration.go#L454</a></p>
</blockquote>
<p>Don't use panic for error handling.</p></pre>snwfdhmp: <pre><p>Thank you very much for your answer ! I will read your links now.</p>
<p>About the review, there're some points I didn't understand :</p>
<blockquote>
<p>Drop the in-repo binary/build.</p>
</blockquote>
<p>I don't understand what you mean here</p>
<blockquote>
<p>Don't use relative imports.</p>
</blockquote>
<p>How should I do it ? I read this before but if I use absolute path, building this on a different computer will fail ?</p>
<blockquote>
<p>Go documentation does not have special tags. Write as plain text what it does. See the standard library for how to write </p>
</blockquote>
<p>I'm not sure to understand what you mean here ? Where should I write this documentation ?</p>
<blockquote>
<p>Instead of storing configuration as a package level variable, return it to the caller.</p>
</blockquote>
<p>Where should it be stored then ? Because in many other functions I assume that the 'project.conf' has been read and Conf contains the right values. Should I read the file every time I need it ?</p>
<blockquote>
<p>Don't use panic for error handling.</p>
</blockquote>
<p>What is your advice for handling errors ? Just displaying them ?</p></pre>klauspost: <pre><blockquote>
<p>"Drop the in-repo binary/build."</p>
</blockquote>
<p>You have the "duck" executable as well as latest.tar.gz in your repo. No real need for that. If you want binary releases, use githubs "releases" for that.</p>
<blockquote>
<p>I read this before but if I use absolute path</p>
</blockquote>
<p>Relative to your $GOPATH :) Change <code>"./parser"</code> to <code>"github.com/snwfdhmp/duck/src/parser"</code>.</p>
<blockquote>
<p>Where should I write this documentation?</p>
</blockquote>
<p>Instead of @returns and similar, use plain descriptions of it. See <a href="https://golang.org/doc/effective_go.html#commentary" rel="nofollow">https://golang.org/doc/effective_go.html#commentary</a> -
<a href="https://blog.golang.org/godoc-documenting-go-code" rel="nofollow">https://blog.golang.org/godoc-documenting-go-code</a> and look in the standard library for a good reference point.</p>
<blockquote>
<p>Where should it be stored then?</p>
</blockquote>
<p>Pass configuration values to where it is needed. It is not a deadly sin to have it available, however it will make things much harder to test, since your behavior depends on configuration settings in another package. </p>
<p>Your main package is free to store its global state, however be aware that nothing can (or should) import it.</p>
<blockquote>
<p>What is your advice for handling errors.</p>
</blockquote>
<p>Handle (and optionally log) or pass upwards. Main function can <code>log.Fatal(err)</code> if it gets an error. But always handle them, never panic. See "Effective Go", <a href="https://blog.golang.org/error-handling-and-go" rel="nofollow">https://blog.golang.org/error-handling-and-go</a> and use google for more.</p></pre>juniorsysadmin1: <pre><p>Can I ask what is going on with the downvotes? Is this not the right sub to ask golang questions? </p></pre>gopher1717: <pre><p>Don't mind. Some people here (the minority) are a bit toxic. They forgot they were also newbies in the past.</p>
<p>You're free to ask questions here, but there are also other places, like <a href="https://forum.golangbridge.org/" rel="nofollow">the forum</a> or the Gophers Slack.</p></pre>snwfdhmp: <pre><p>Is it boring for some people to ask this kind of thing on reddit ? Sorry if it is, I'm new to Reddit.</p></pre>gopher1717: <pre><p>What happens is that Go is a new language, and until few time ago to there were mostly early adopters, and this kind of people usually look to more advanced content.</p>
<p>Now that more newbies are learning Go (and that is good!), same of the more advanced devs may find dealing with newbies boring.</p>
<p>But you should not feel bad about it! They're the minority. We see this behavior more on Reddit, other Go communities are more welcome to newbies.</p>
<p>Be welcome!</p></pre>allhatenocattle: <pre><p><a href="https://github.com/snwfdhmp/duck/blob/master/cmd/duck/duck.go#L73" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/cmd/duck/duck.go#L73</a>
If you use a bufio.NewScanner(os.os.Stdin), the logic will be a bit cleaner, won't need to strip the newlines</p>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/cmd/duck/duck.go#L122" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/cmd/duck/duck.go#L122</a>
break statements aren't needed in the switch (I see that in a lot of places)</p>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/pkg/logger/logger.go#L49" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/logger/logger.go#L49</a>
this is confusing since log.Fatal in stdlib always exits vs yours checks if err is nil</p>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/pkg/logger/logger.go#L53" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/logger/logger.go#L53</a>
the os.Exit(1) isn't needed, log.Fataln exits</p>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/pkg/parser/parser.go#L53" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/parser/parser.go#L53</a>
instead of if shouldContinue != true { use</p>
<pre><code>if ! shouldContinue {
</code></pre>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L119" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L119</a>
comment doesn't match actual path</p>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L197-L200" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L197-L200</a>
I don't understand this part, isn't Lings a func local variable?</p>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L230" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L230</a>
use os.Mkdir</p>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L264" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L264</a>
looks like the break always occurs, do you only want that loop ran once?</p>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L266" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L266</a></p>
<pre><code>if ! installed {
</code></pre>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L418" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L418</a>
If there is a return in an if statement, the else isn't needed, could be converted to</p>
<pre><code>//looking for the commands corresponding to the label
for _, ling := range Lings {
if ling.Label == label { // if scheme's label is input, return it
return ling.Commands
}
// look in its aliases
for _, alias := range ling.Aliases {
if alias == label {
return ling.Commands
}
}
}
</code></pre>
<p>edit: formatting</p></pre>snwfdhmp: <pre><p>Thanks a lot for your review ! I applied all changes you told me excepting the one with the logger package which requires more refactoring.</p>
<blockquote>
<p>I don't understand this part, isn't Lings a func local variable?</p>
</blockquote>
<p>I don't understand what you don't understand</p>
<blockquote>
<p>looks like the break always occurs, do you only want that loop ran once?</p>
</blockquote>
<p>No, the
<code>
if err != nil {
continue
}
</code>
L245-247 continue the loop if the package could not be downloaded</p></pre>allhatenocattle: <pre><blockquote>
<p><a href="https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L197-L200" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L197-L200</a></p>
</blockquote>
<p>oh, I was missing that Lings was package global, not local to the func, in which case I don't think
<a href="https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L183" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L183</a>
is necessary since it was already declared in
<a href="https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L53" rel="nofollow">https://github.com/snwfdhmp/duck/blob/master/pkg/configuration/configuration.go#L53</a></p></pre>snwfdhmp: <pre><p>May I ask too what you think about the project itself ? Well made ?
How can I improve the quality of the project/tool ?</p>
<p>Does it feel comfortable to use ? (there're informations in the README about how to install, or bellow a little documentation to see of it works)</p></pre>gopher1717: <pre><p>I didn't tried the project myself, but I'll give a few suggestions based on the readme.</p>
<ul>
<li>I would remove the Emojis from the project description and README</li>
<li>Don't tell people to run a bash script to install the project. Go users should install it with <code>go get</code></li>
<li>You may want to automate the release process to GitHub releases with <a href="https://github.com/goreleaser/goreleaser" rel="nofollow">GoReleaser</a></li>
<li>The README doesn't make it clear how to I config these tasks. Should I manually create that JSON file? In which folder? With what name? Etc...</li>
</ul></pre>snwfdhmp: <pre><p>Thank you for your answer, I will commit all of this soon !</p></pre>klauspost: <pre><blockquote>
<p>Don't tell people to run a bash script to install the project.</p>
</blockquote>
<p>For releases that is fine IMO, since users may not have Go an environment set up, but otherwise I <code>go get</code>should be ok.</p></pre>snwfdhmp: <pre><p>I made some improvements, I'm still working on it.
You can see what I already did here : <a href="https://github.com/snwfdhmp/duck" rel="nofollow">https://github.com/snwfdhmp/duck</a></p></pre>
