You don’t need separate client and models packages. You should have two packages: main (which has only one line) and everything else (call it package xkcd).
Your main continues even after it finds an error in fetching the comic. This is because you can’t return err in main and you didn’t use panic/log.Fatal. You also don’t exit with a non-zero code on error.
The solution to all of this is the one line main function. Main should look something like os.Exit(xkcd.Run(os.Args[1:])). I have a helper package at github.com/carlmjohnson/exitcode so you can return an error with an associated exit code, but you can also be simple and just return an int.
Separate flag gathering/processing from execution. You’re not using global flags, which is a good step, but you should go further and make an appEnv struct that consolidates what you’re taking in from the flags. See here: https://play.golang.org/p/-gs5nqXBSuB
Your client package basically doesn’t need to exist. What you want are some simple convenience wrappers around the http package and a URL builder for XKCD. Make something generic for HTTP and you can copypasta it into your future projects. Basically, it just needs to be httphelper.GetJSON(cl ∗http.Client, url string, data interface{}) error. Saving to disk is a separate idea that you’re conflating with downloading. Make something like jsonhelper.SaveToDisk(path string, data interface{}) error. The timeout stuff you’re doing is overblown. Either just use context.WithTimeout or put a ∗http.Client in your appEnv and have ParseArgs set the default timeout on that.
The stuff with the base URL is unnecessary. Obviously you know the XKCD base URL is a constant and will never change. But don’t you need to set it in an XKCDClient struct for testing purposes? The answer is no. If you take in a ∗http.Client, that can set a different http.RoundTripper for testing purposes and the test RoundTripper can read from memory or do whatever you want. (You can see this principle at work in Google’s Go http libraries. Once you realize how powerful ∗http.Client is, it makes a lot of the hoops other libraries jump through see like a waste of time. It can do all your auth stuff, caching stuff, everything. It’s great.)
The model package should just be a file in your xkcd package. I find the names Comic vs. ComicResponse confusing. Do you need Comic at all? Maybe just add some nice helper methods onto ComicResponse. Don’t do cr.FormattedDate() string. Do cr.Date() time.Time and let the output layers handler formatting, not the model layer. The c.JSON() string method doesn’t need to exist. With Go, you can run into this problem of trying to make things more convenient by adding helpers but you end up with methods that just run two commands and don’t actually make things more convenient on net. Is this really a model level concern or should it just be in the xkcd.Run() function?
Anyway, not to be overly negative. For such a small app, none of this really matters. I’ve been making a lot of Go CLIs for a long time[1] and my experience is that the most important thing is to separate flag stuff from execution, and everything else is not a big deal to let evolve over time. The main challenge is avoiding create abstractions that don’t actually pay for themselves in setup time vs. time saved in extension.
I really appreciate you effort in pointing out the correct way to do things.
In my defence I would like to point that this post was to help beginners get hands on experience writing Go code, adding design patterns or organising the code base to make things "correct" will only confuse a person who has just started to learn Go.
And to be frank, I too don't have much practical knowledge about it. If you don't mind can you point me in the right direction?
You don’t need separate client and models packages. You should have two packages: main (which has only one line) and everything else (call it package xkcd).
Your main continues even after it finds an error in fetching the comic. This is because you can’t return err in main and you didn’t use panic/log.Fatal. You also don’t exit with a non-zero code on error.
The solution to all of this is the one line main function. Main should look something like os.Exit(xkcd.Run(os.Args[1:])). I have a helper package at github.com/carlmjohnson/exitcode so you can return an error with an associated exit code, but you can also be simple and just return an int.
Separate flag gathering/processing from execution. You’re not using global flags, which is a good step, but you should go further and make an appEnv struct that consolidates what you’re taking in from the flags. See here: https://play.golang.org/p/-gs5nqXBSuB
Your client package basically doesn’t need to exist. What you want are some simple convenience wrappers around the http package and a URL builder for XKCD. Make something generic for HTTP and you can copypasta it into your future projects. Basically, it just needs to be httphelper.GetJSON(cl ∗http.Client, url string, data interface{}) error. Saving to disk is a separate idea that you’re conflating with downloading. Make something like jsonhelper.SaveToDisk(path string, data interface{}) error. The timeout stuff you’re doing is overblown. Either just use context.WithTimeout or put a ∗http.Client in your appEnv and have ParseArgs set the default timeout on that.
The stuff with the base URL is unnecessary. Obviously you know the XKCD base URL is a constant and will never change. But don’t you need to set it in an XKCDClient struct for testing purposes? The answer is no. If you take in a ∗http.Client, that can set a different http.RoundTripper for testing purposes and the test RoundTripper can read from memory or do whatever you want. (You can see this principle at work in Google’s Go http libraries. Once you realize how powerful ∗http.Client is, it makes a lot of the hoops other libraries jump through see like a waste of time. It can do all your auth stuff, caching stuff, everything. It’s great.)
The model package should just be a file in your xkcd package. I find the names Comic vs. ComicResponse confusing. Do you need Comic at all? Maybe just add some nice helper methods onto ComicResponse. Don’t do cr.FormattedDate() string. Do cr.Date() time.Time and let the output layers handler formatting, not the model layer. The c.JSON() string method doesn’t need to exist. With Go, you can run into this problem of trying to make things more convenient by adding helpers but you end up with methods that just run two commands and don’t actually make things more convenient on net. Is this really a model level concern or should it just be in the xkcd.Run() function?
Anyway, not to be overly negative. For such a small app, none of this really matters. I’ve been making a lot of Go CLIs for a long time[1] and my experience is that the most important thing is to separate flag stuff from execution, and everything else is not a big deal to let evolve over time. The main challenge is avoiding create abstractions that don’t actually pay for themselves in setup time vs. time saved in extension.
[1]: https://blog.carlmjohnson.net/post/2018/go-cli-tools/