Could I get a code review for a simple Go CLI tool I wrote to grab Reddit Users Imgur uploads?

<p>I created <a href="https://github.com/chrisgreg/go-get-im" rel="nofollow">this</a> a few days ago because I needed a new Go project to practise with but I&#39;m still quite new to the language - can someone give me a bit of a code review?</p> <p>Apologies if this is the wrong place to ask for this - unsure of any other Go subreddits around - I also don&#39;t really know any other Go devs to ask myself.</p> <hr/>**评论：**<br/><br/>elagergren: <pre><p>Sure. What I noticed right off the bat:</p> <ul> <li><p><code>IsAlbum</code> uses <code>regexp.MatchString</code> which calls <code>regexp.Compile</code> each invocation. It&#39;d make sense to create an unexported global to prevent the compilation each time.</p></li> <li><p><a href="https://github.com/chrisgreg/go-get-im/blob/0278cc14968844cdd9b1176c78d24851070897c2/utils/imgur.go#L29" rel="nofollow">You</a> <a href="https://github.com/chrisgreg/go-get-im/blob/0278cc14968844cdd9b1176c78d24851070897c2/utils/imgur.go#L34" rel="nofollow">panic</a> <a href="https://github.com/chrisgreg/go-get-im/blob/0278cc14968844cdd9b1176c78d24851070897c2/utils/imgur.go#L39" rel="nofollow">a lot</a>. Don&#39;t do this. Instead, return an error. If it&#39;s an error people should be able to check (e.g., <code>io.EOF</code>), export some sort of type or function that allows the user to check for it. Otherwise, wrap it in a little bit of state (e.g., <code>fmt.Errorf</code>) and bubble it up.</p></li> <li><p>You don&#39;t need to create a new <code>http.Client</code> for each invocation of <code>GetAlbumImageLinks</code>. I&#39;ll come back to this one in a minute.</p></li> <li><p>Don&#39;t forget to close <code>Response.Body</code> after calling <code>Do</code>.</p></li> <li><p>It&#39;s a *nix convention to not print out anything to stdout <em>except</em> necessary output. Logging (typically) goes to stderr. </p></li> <li><p><code>if x == true</code> is logically equivalent to <code>if x</code>.</p></li> <li><p><code>CleanUrl</code> should probably be <code>CleanURL</code> since Go&#39;s naming conventions say to use all-caps for acronyms (i.e., HTTP not Http). (There are other instances of this as well.)</p></li> <li><p><code>ReplaceAllString</code> in <code>CleanUrl</code> could just use <code>strings.Replacer</code> instead of regular expressions.</p></li> <li><p><code>CreateFolder</code> doesn&#39;t return an error, but <code>MkdirAll</code>does.</p></li> <li><p>In <code>ParseAlbumJson</code> you pass a pointer to a pointer to <code>ImgurAlbumApiResponse</code> to <code>json.Unmarshal</code>. This probably wasn&#39;t intentional. Also, consider using <code>:=</code> in lieu of <code>var x = ...</code>.</p></li> <li><p>WRT the comment about <code>http.Client</code>, a lot of the library could be methods on some sort of struct. This means you could have <code>type Client struct { c *http.Client, ... }</code> and let users determine whether they want to use <code>http.DefaultClient</code> or not. (I&#39;d pass that parameter in via some sort of <code>ClientOption</code> type, e.g. <code>type ClientOption func(c *Client)</code>)</p></li> </ul></pre>bustyLaserCannon: <pre><p>Awesome stuff - I&#39;ll look to fix it up in the next few days.</p> <p>Thanks for your help</p></pre>bustyLaserCannon: <pre><p>One more question I forgot to mention in the OP - could Goroutines effectively speed this up? Would it make sense to spawn a Goroutine for each link being parsed - or is so little work being done before handing off to IO or a network request that it&#39;d be a negligible boost? </p></pre>elagergren: <pre><p>Only one way to find out :-) There&#39;s no real way to know without testing it.</p></pre>bustyLaserCannon: <pre><p>:)</p></pre>shovelpost: <pre><p>I believe your package naming follows some <a href="https://youtu.be/zzAdEt3xZ1M?t=6m30s" rel="nofollow">anti patterns</a>. I also think that you should rename <code>go-get-im.go</code> to <code>main.go</code>.</p> <p>You are also missing a License, documentation and tests. See this <a href="https://goreportcard.com/report/github.com/chrisgreg/go-get-im" rel="nofollow">report</a>.</p> <p>The code looks ok-ish but I believe it could be structured better. There are also some missing error checks, like <a href="https://github.com/chrisgreg/go-get-im/blob/master/utils/utils.go#L36" rel="nofollow">here</a> and <a href="https://github.com/chrisgreg/go-get-im/blob/master/api/reddit.go#L38" rel="nofollow">most importantly here</a>. Also I don&#39;t believe you should be using panic so <a href="https://github.com/chrisgreg/go-get-im/blob/master/api/imgur.go#L60" rel="nofollow">freely</a>. Better <a href="https://github.com/golang/go/wiki/CodeReviewComments#dont-panic" rel="nofollow">replace your panics with errors</a>.</p> <p>You can also simplify <a href="https://github.com/chrisgreg/go-get-im/blob/master/utils/imgur.go#L77" rel="nofollow">this line</a> to <code>if album {</code></p> <p>I recommend using <a href="https://github.com/alecthomas/gometalinter" rel="nofollow">gometalinter</a> which will help you finding some of those problems easier.</p></pre>bustyLaserCannon: <pre><p>Thanks for pointing those out, i&#39;ll watch that video too</p></pre>allhatenocattle: <pre><p>I agree with what <a href="/u/elagergren" rel="nofollow">/u/elagergren</a> said. </p> <p>In addition: There is no check that an argument (username) is passed, so you get the following user experience:</p> <p>panic: runtime error: index out of range</p> <p>not great. Checking that one argument is passed <code>len(os.Args) == 2</code> and if not, print a help message would be a good way to handle it.</p> <p>If you end up needing additional options, look at the flags package.</p> <p>utils is not a good package name</p> <p><a href="https://github.com/chrisgreg/go-get-im/blob/master/utils/imgur.go#L51" rel="nofollow">https://github.com/chrisgreg/go-get-im/blob/master/utils/imgur.go#L51</a> </p> <p>Is that intended, or left over from debugging?</p></pre>