The current support for encoding "request" structs as URL query strings is weak (because I hacked it out). The easiest way to solve this is to use github.com/google/go-querystring/query. I've evaluated it and decided that it would solve many problems at once:

1. Proper support for encoding embedded structs
2. Better struct tags for fields that should vs shouldn't be encoded (currently just using json, which is inaccurate)
3. Unification of "query" structs. Some methods require a query struct like BeerChangeRequest for adding new beers or updating existing ones, when really they should just take a Beer struct. This requires omitting some fields in the Beer struct during encoding, e.g. ID, CreateDate, etc.

(For example) I don't think the EventService should allow retrieving Events using List and Get when adding new ones or updating/deleting existing ones requires AddEvent, UpdateEvent, or DeleteEvent, respectively. The EventService also has methods like ListBeers, AddBeer, UpdateBrewery, etc.

Should the methods called List and Get be renamed to include the type of object being retrieved, or should the Add..., Update..., and Delete... methods have the object type removed from their name for each service?

Option 1:

client.Event.List -> client.Event.ListEvents
client.Event.Get  -> client.Event.GetEvent

or Option 2:

client.Event.AddEvent    -> client.Event.Add
client.Event.UpdateEvent -> client.Event.Update
client.Event.DeleteEvent -> client.Event.Delete

For reference, the BeerService currently looks like option 2.

Many endpoints return a "list" of objects, usually paginated. The current API has three different "list" types:

• normal slice of objects
• a thin struct wrapper containing a slice of objects, one for each "page"
• a struct wrapper that supports iteration with built-in pagination using a First and Next method

Obviously the most convenient is a slice of all the objects. This is possible for all collections that are not paginated. For paginated collections, the current First, Next idiom, e.g.

c := NewCollection(...)
for v, err := c.First(); v != nil; v, err = c.Next() {
    if err != nil {
        // handle err and break
    }
    // do something with v
}

is convenient because it handles pagination under the hood and allows for clean iteration over every object. It has a few problems though:

1. A user has no control over pagination (and therefore the number of API requests they make)
2. If First returns a nil pointer to an object, and an error, say a critical error, the error isn't caught. Sure you could declare err before the loop and check it after the loop, but that's a terrible idea.
3. Much of the API returns objects not pointers, where this iteration API requires pointers because it uses nil as the loop terminator.

The thin wrapper with manual pagination is simple. It'd be nice to have some optional API on top it to allow for iteration, perhaps using a channel (since they are iterable using range).

The "search" endpoint on BreweryDB allows for searching breweries, beers, guilds and events but since objects of each type are returned in one JSON response, the Go implementation just has 4 separate methods for searching each type. This makes the code simple, but it also means a user must make more requests to search for multiple types of objects, e.g. searching for both breweries and beers with a specific term requires two method calls and two HTTP requests.

In the case that there are JSON fields in a response that aren't defined in the corresponding Go struct, it would be nice if they were unmarshalled into some extra space. Apparently other JSON libraries are capable of doing this. At the very least it would help develop/debug this library as there are a few discrepancies in the BreweryDB API documentation.

Every endpoint has a lot of duplicate code for:

• creating the request with url-encoded values
• making the request and checking errors
• checking the HTTP response
• decoding JSON and checking errors

It'd be nice to consolidate this logic.

Some services have only one or two methods, e.g. Heartbeat.Heartbeat. The "Service" is probably unnecessary and the Heartbeat method could just move to the Client.

All Add/Update endpoints accept a pointer to a brewerydb type which then gets encoded using go-querystring. Each of these methods should check that said pointer is not nil before creating an *http.Request.

The entire package needs unit tests.

Each method's comment block should contain a link to the endpoint's corresponding documentation on the BreweryDB website. This makes it easier for users to see how to use the endpoint and help keep the library in sync with the actual web API.

As of now, if there is an HTTP error during a request, the error code is caught and swallowed and a very useless message is returned in its place.

Google's Github Go API actually returns an entire Response object for each endpoint, along with the result "object" and an error. This makes sense, but I don't know that it's not overkill. Regardless, something must be done.

Ideally, all "Y/N" fields should be of type bool in the Go API. I guess this would require special JSON decoding/encoding.

On this note, it would be nice if all the "year" and "postalCode" fields could be ints in Go but still encode to strings in JSON.