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

I created this a few days ago because I needed a new Go project to practise with but I'm still quite new to the language - can someone give me a bit of a code review?

Apologies if this is the wrong place to ask for this - unsure of any other Go subreddits around - I also don't really know any other Go devs to ask myself.

---

评论：

elagergren:

Sure. What I noticed right off the bat:


IsAlbum uses regexp.MatchString which calls regexp.Compile each invocation. It'd make sense to create an unexported global to prevent the compilation each time.

You panic a lot. Don't do this. Instead, return an error. If it's an error people should be able to check (e.g., io.EOF), 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., fmt.Errorf) and bubble it up.

You don't need to create a new http.Client for each invocation of GetAlbumImageLinks. I'll come back to this one in a minute.

Don't forget to close Response.Body after calling Do.

It's a *nix convention to not print out anything to stdout except necessary output. Logging (typically) goes to stderr. 

if x == true is logically equivalent to if x.

CleanUrl should probably be CleanURL since Go's naming conventions say to use all-caps for acronyms (i.e., HTTP not Http). (There are other instances of this as well.)

ReplaceAllString in CleanUrl could just use strings.Replacer instead of regular expressions.

CreateFolder doesn't return an error, but MkdirAlldoes.

In ParseAlbumJson you pass a pointer to a pointer to ImgurAlbumApiResponse to json.Unmarshal. This probably wasn't intentional. Also, consider using := in lieu of var x = ....

WRT the comment about http.Client, a lot of the library could be methods on some sort of struct. This means you could have type Client struct { c http.Client, ... } and let users determine whether they want to use http.DefaultClient or not. (I'd pass that parameter in via some sort of ClientOption type, e.g. type ClientOption func(c Client))

bustyLaserCannon:

Awesome stuff - I'll look to fix it up in the next few days.

Thanks for your help

bustyLaserCannon:

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'd be a negligible boost? 

elagergren:

Only one way to find out :-) There's no real way to know without testing it.

bustyLaserCannon:

:)

shovelpost:

I believe your package naming follows some anti patterns. I also think that you should rename go-get-im.go to main.go.

You are also missing a License, documentation and tests. See this report.

The code looks ok-ish but I believe it could be structured better. There are also some missing error checks, like here and most importantly here. Also I don't believe you should be using panic so freely. Better replace your panics with errors.

You can also simplify this line to if album {

I recommend using gometalinter which will help you finding some of those problems easier.

bustyLaserCannon:

Thanks for pointing those out, i'll watch that video too

allhatenocattle:

I agree with what /u/elagergren said. 

In addition:
There is no check that an argument (username) is passed, so you get the following user experience:

panic: runtime error: index out of range

not great.  Checking that one argument is passed len(os.Args) == 2 and if not, print a help message would be a good way to handle it.

If you end up needing additional options, look at the flags package.

utils is not a good package name

https://github.com/chrisgreg/go-get-im/blob/master/utils/imgur.go#L51  

Is that intended, or left over from debugging?