Return to Answer

• 1

• 98.1k
• 17
• 219
• 419

Well, when you decide to something as a learning exercise it's normal to tackle just one problem at a time. In your program you have databases, object-relational mapping, JSON encoding, OAuth protocols, closures, REST, and more. It almost makes me a bit sad not to see slices, channels, and go-routines in there. What happened to the good-old FizzBuzz?

Your ambition here is admirable, and I understand the motivation to accomplish something meaningful (a cool slack-bot is awesome), but I worry that you'll maybe miss some details in your haste.

##General

In general, your code looks like it has been through the wringer a few times, and it has wrinkles, and a few worn spots. While it looks like you are already in the habit of running your code through the go fmt routines, you should also add go vet and golint to your tools. You have a few issues I can see off-hand:

• there's no need to rename the import net/http to http... that's what it's called already.

• the comments on team are out of date. It's no longer exported (no longer capital-T Team.

• you have mis-uses of log.Fatal(...). That call really is fatal, it calls os.Exit(1) when done. No code can run after the log.Fatal() so the following lines are redundant:

   errorMessage := fmt.Sprintf("Failed to create OAuth Token request, parameters: %v", request)
   log.Fatal(errorMessage)
   http.Error(writer, errorMessage, 501)
   return
  

The httpError(...) is never sent.

There are some places where error checking is incomplete:

database, err := gorm.Open("postgres", os.Getenv("DATABASE_URL"))
defer database.Close()

The authorizeSlackTeam method's handler function captures the body of the request, but does not use it. If you are simply trying to force the close of the body at the function end (and not the beginning), you can replace:

 body := request.Body
 defer body.Close()

with:

 defer request.Body.Close()

In that same area, you have the function declaration:

 func authorizeSlackTeam(db *gorm.DB) (handler func(writer http.ResponseWriter, request *http.Request)) {

but there's no need to name the writer and `request. It can be just:

 func authorizeSlackTeam(db *gorm.DB) (handler func(http.ResponseWriter, *http.Request)) {

but even better, you can reuse the http.HandlerFunc type to simplify it to:

func authorizeSlackTeam(db *gorm.DB) (handler http.HandlerFunc) {

Despite the above issues, in general, the code is good. The variable names are descriptive, the function names too. The code does flow quite well, and I don't feel like the functions are doing too much.

http errors

It's common in http-handling code to have a helper function to allow child methods to return an error, and then you catch that error and return a consistent, reused, error message system.

Consider a function like:

type UncheckedHandler func(http.ResponseWriter, *http.Request) error
func checkedHandler(handler UncheckedHandler) http.HandlerFunc {
 return func(writer http.ResponseWriter, request *http.Request) {
 if err := handler(writer, request) err != nil {
 log.Printf(err)
 http.Error(writer, err.Error(), 501)
 }
 }
}

Now you can have your own set of handlers that have an error return value, and you can simplify the error-handling code that you have scattered over the place. The setup routine changes from:

http.HandleFunc("/", sayHelloWorld)
http.HandleFunc("/setColor", setColor)
http.HandleFunc("/authorizeSlackTeam", authorizeSlackTeam(database))

to

http.HandleFunc("/", checkedHandler(sayHelloWorld))
http.HandleFunc("/setColor", checkedHandler(setColor))
http.HandleFunc("/authorizeSlackTeam", checkedHandler(authorizeSlackTeam(database)))