Some random notes:
- Your use of
notFoundHandler().ServeHTTP(w, r) is weird. Why do you have a function returning this if it doesn't take any params? You'd probably be better off just having a function writeNotFound(w, r). This mirrors the approach of net.http, which already provides functions like this.
- From a RESTful approach, you shouldn't be returning the
StatusCode in the JSON. That should be the HTTP status code.
- This is minor, but what is an
Enrol? Enrollment?
if r.Method != http.MethodGet, you should be returning a 405 Method Not Allowed instead of a 404 Not Found- If
json.Marshal fails, you should return a 500 Internal Server Error instead of an unauthorized
- Be careful with variables shadowing a package (
store *store.Store). That can have unexpected consequences. Ideally you package name and struct should form a complete thought like http.Server (instead of server.Server). Unfortunately, in some scenarios this is difficult.
- Your store functions don't follow Go conventions. If you are returning
(X, error) then you should only check err != nil. Either err != nil (and you ignore the first thing) or err = nil and the first thing is valid. Don't do assessments == nil || err != nil. This should just be err != nil. You may need to change how the store functions behave to conform to this pattern.
The overarching point
Repetitive error handling is an unfortunate consequence of some of the design decisions of Go (namely, a lack of parametric polymorphism). In this case however, even if you had generics (to pull out the repetitive error handling logic), you'd still need some way to bubble the return up to the http.HandlerFunc. And that would require something like:
if !tryQuery(func() (User, error) { return store.GetUserByEmail(email) }) {
return
}
Where tryQuery returns false if store.GetUserByEmail(email) returned an error. This is certainly shorter, but it's dubious that this is an improvement. You also couldn't write such a tryQuery currently in go because the first return type User would need to be generic. Sure, you could cast through interface{}, but that's a world of ugliness that you don't want to deal with. It would require you to do lots of casts, which incur a runtime cost and are (programmer) error prone.
So what to do? Well, any refactoring you embark on will be a tradeoff. The go ethos often prefers more verbose, repetitive code because it's easier to follow. But this comes at a cost. Eventually code becomes hard to reason about and harder to test because units have many responsibilities.
That said, I think there are some patterns you could use that would really help clear this code up. In the end, the code should be easier to read and easier to test.
First, checks like "is the user authorized" is often better suited for middleware. This pattern will allow you write the authorization check once and then wrap every handler in this middleware that needs it:
func needsAuthorization(handler http.HandlerFunc) http.HandlerFunc {
return http.HandlerFunc(func (w http.ResponseWriter, r *http.Request) {
if !request.IsAuthorized(r) {
log.Println("Unauthorized")
writeUnauthorized(w, r)
} else {
handler(w, r)
}
})
}
Then you can use it like so:
func stateHandler(store *store.Store) http.HandlerFunc {
return needsAuthorization(func(w http.ResponseWriter, r *http.Request) {
// This is only called if user is authorized
// Check email, get assessments, etc.
})
}
This is appropriate for checks like authorization (and maybe checking the request method), but shouldn't be used for everything.
Another option here (although sometimes frowned upon by the purists) is to use an existing HTTP framework that handles routing (including method) and maybe already has utilities for injecting middleware. This is somewhat in conflict with Go's obvious philosophy as there is some magic introduced. As with refactoring , there are tradeoffs here. Frameworks abstract away some common patterns to enable more succinct business logic. For example, with a buffalo router you can do:
a.GET("/state", stateHandler(store))
The meat of your problem still exists. All of the interaction with your models and constructing the response still is a lot of repetitive error handling. The ideal approach here would be an Applicative pattern which is capable of constructing data up until the point of an error. In Haskell, this would look something like:
user <- getUserByEmail =<< getParam "email"
return $ StateResponse <$> user <*> getEnrollments user <*> getAssessments user
<*> getSubmissions user
The beauty here is that all of these steps (user, getEnrollments user, etc.) can fail. And when they do, the error is returned instead. But until one occurs, we just continue to construct the data. Note that this looks almost like how you build the StateResponse struct in Go, except it also does error handling.
In Go, you'd have to duplicate some (trivial) code to achieve something like this. And I'm not sure it would be worth it. You'd end up with something like:
state := StateResponseBuilder{}
state.SetEmail(func() (string, bool) { return r.Url.Query()["email"] })
state.SetUser(func() (store.User, error) { return store.GetUserByEmail(state.Email) })
state.SetEnrollments(func() ([]store.Enrollment, error) { return store.GetEnrollmentsByUser(state.User.UID) })
state.SetAssessments(func() ([]store.Assessment, error) { return store.GetAssessmentForUser(state.User.UID) })
state.SetSubmissions(func() ([]store.Submission, error) { return store.GetSubmissionsForUser(state.User.UID) })
respBytes, err := state.Build()
if err != nil {
// return error
} else {
w.Write(respBytes)
}
The idea here is that StateResponseBuilder somewhat follows the Pike technique. Each SetX looks like this:
type StateResponseBuilder struct {
Err error
X X
}
func (s *StateResponseBuilder) SetX(f func() (X, error)) {
if s.Err != nil {
return
}
if x, err := f(); err != nil {
self.Err = err
} else {
self.X = x
}
}
Your error short circuiting is handled by checking if s.Err != nil before running f(). s.Err is the first error we encountered or nil if everything so far as succeeded.
You'll note this requires a lot of code repetition in the SetX methods. Furthermore, the resulting code is probably less obvious even though it lacks all the error checking.
That said, such a pattern can be useful in certain contexts.
A better approach
A more suitable pattern, in my opinion, would be to separate concerns and use some indirection to make errors responsible for reporting themselves (instead of making your handler responsible for knowing how to report them). The big idea here is that currently your handler has many jobs. This is what is making it slightly unwieldy. You can separate its main jobs into two categories:
- HTTP related (specifically, translating errors/data to the appropriate constructs in HTTP)
- Interacting with
store
If you separated these two, you could de-duplicate some of your repetitive HTTP error handling code. Further, this would abstract your controller logic away from HTTP so if you wanted to provide another kind of API (maybe some sort of custom protocol over TCP), you could do this fairly easily. Doing something like this would make your handler look like:
func stateHandler(store *store.Store) http.HandlerFunc {
return needsAuthorization(onlyGet((http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
var email string
emails := r.URL.Query()["email"]
if len(emails) == 1 {
email = emails[0]
}
response, err := buildStateResponse(store, email)
if err != nil {
err.WriteHTTP(w)
} else {
w.Write(response)
}
})))
}
We've done several things here. First, all the HTTP error handling has been reduced to one location. This should make logging much easier. All HTTP-related things are located just in the handler. Note that we did push some of this into the errors themselves. This kind of indirection allows us to not have to handle every case in the HTTP handler. The advantage here is if another error condition arises when you're working on store, you don't have to go through all of your HTTP handlers and handle outputting the error each place a store function is used. Doing this would require you to create a custom error interface that has WriteHTTP, but you could just as easily make this a function that takes an error as its first parameter. The simplicity of the pure controller logic speaks for itself:
// You may want to return the `StateResponse` here and handle serialization in the HTTP handler
func buildStateRespones(store *store.Store, email string) ([]byte, error) {
user, err := store.GetUserByEmail(email)
if err != nil {
return nil, err
}
enrolled, err := store.GetUserEnrollments(user.UID)
if err != nil {
return nil, err
}
assessments, err := store.GetUserAssessments(user.UID)
if err != nil {
return nil, err
}
submissions, err := store.GetUserSubmissions(user.UID)
if err != nil {
return nil, err
}
return json.Marshal(StateResponse{
User: *user,
Enrolled: enrollments,
Assessment: assessments,
Submissions: submissions,
})
}
You still need the error checks, but now you aren't trying to do so much inside them. This code should be much easier to follow. What's more, if the new error check proposal lands, this code will immediately be convertible to this much more readable form:
func buildStateRespones(store *store.Store, email string) (*StateResponse, error) {
user := check store.GetUserByEmail(email)
enrolled := check store.GetUserEnrollments(user.UID)
assessments := check store.GetUserAssessments(user.UID)
submissions := check store.GetUserSubmissions(user.UID)
return &StateResponse{
User: *user,
Enrolled: enrollments,
Assessment: assessments,
Submissions: submissions,
}, nil
}
Here we've separated business logic from protocol specific "presentation" logic. The result is that each separated part is now easier to reason about (and easier to unit test!).
