Don't hold back #2
Conversation
Release 1.0
| package main | ||
|
|
||
| import ( | ||
| "geoffrey/commands" |
There was a problem hiding this comment.
I'm not sure how this is working. I don't see a package named geoffrey. On my IDE it is happy when I do ./commands but thats still not proper form.
go project structures love the long form that you see in repository urls. So github.com/mmoghaddam385/GeoffreyBot/commands would be the proper way.
Of course for this to work properly on your machine, or any machine that is running this, the GOPATH would need to be set and this code would need to live in $GOPATH/github.com/mmoghaddam385/
| import ( | ||
| "geoffrey/commands" | ||
|
|
||
| "os" |
There was a problem hiding this comment.
look into goimports for info on how go likes its stuff imported.
The rule is top group is stdlib so stuff like os. Second group is third party. Last group is stuff thats in this project.
|
|
||
| const CommandNotFoundCode = -100 | ||
|
|
||
| var commands []types.ConsoleCommand |
There was a problem hiding this comment.
first glance I dont think having this being package level is desired. I can't think of an alternative at the moment but I just know this makes it harder to scale. Let's say in the future you're Main.go spins up multiple geoffrey bots in concurrent. Then they are sharing one commands.
Also its in name conflict with the package name which isn't good practice
There was a problem hiding this comment.
for the name maybe consoleCommands
There was a problem hiding this comment.
The more I think about it the more it seems like this would be nice as a map. Since it is a registry of sorts you can key by command Name and value by the struct. That way your RunCommand can do a lookup instead of O(n) search.
Still thinking about the scope and rn seems fine at package level. If you did have multiple concurrent workers then you could just mutex lock it
| fmt.Println("Registering Endpoints...") | ||
|
|
||
| for path, handler := range endpoints.GetAllEndpoints() { | ||
| fmt.Printf("Registering Handler for %v\n", path) |
There was a problem hiding this comment.
This seems like something that should be logged instead of printed to stdout. The other prints I've seen make sense since they are interactive to the user of the CLI. But this is informational and is mostly helpful to a dev and not a user.
If you wanna be fancy look into struct logging
|
Overall comment: golang file naming convention is short names and if needed for compound names then |
|
|
||
| const CommandNotFoundCode = -100 | ||
|
|
||
| var commands []types.ConsoleCommand |
There was a problem hiding this comment.
The more I think about it the more it seems like this would be nice as a map. Since it is a registry of sorts you can key by command Name and value by the struct. That way your RunCommand can do a lookup instead of O(n) search.
Still thinking about the scope and rn seems fine at package level. If you did have multiple concurrent workers then you could just mutex lock it
| func (*ServerCommand) Execute(args []string) int { | ||
| addr, err := determineListenAddress() | ||
| if err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
oh you are logging.. im still reading but maybe revisit when you print vs log and see if the uses are consistent
| if fact, exists := body[catFactFactKey]; exists { | ||
| return sanitizeCatFact(fact), nil | ||
| } else { | ||
| return "", errors.New("Could not parse fact out of response body!") |
There was a problem hiding this comment.
errors shouldn't start with a capital letter
https://github.com/golang/go/wiki/CodeReviewComments#error-strings
| func sanitizeCatFact(fact string) string { | ||
| fact = strings.Replace(fact, "\"", "'", -1) // Remove quote marks | ||
| return strings.Replace(fact, "\\", "", -1) // Remove backslashes | ||
| } |
There was a problem hiding this comment.
This is more of an opinion piece: what would you think of doing this instead.
func removeQuotes(old string) (new string) {
return strings.replace(old, "\"", "'", -1)
}
func removeBackslashes(old string) (new string) {
return strings.Replace(old, "\\", "", -1)
}
func sanitzeCatFact(fact string) string {
return removeBackSlashes(removeQuotes(fact))
}
// or
func sanitzeCatFact(fact string) (sanitizedFact string) {
sanitizedFact = removeQuotes(fact)
return removeBackSlashes(sanitizedFact)
}
I consider it since you have a comment explaining what the two lines are doing and im under the opinion that breaking out little things for clarity is worth it, even if its more lines of code.
Also its very clear what is happening given the named return values (new string)
the different sanitizeCatFacts are up to you. I think idiomatic go would prefer the second version Obvious code is important. What you can do in one line you should do in three
| return "", err | ||
| } | ||
|
|
||
| defer resp.Body.Close() |
There was a problem hiding this comment.
something I started seeing at work is people putting typical defered functions in a wrapper that catches errors for them. Since Close() can return an error yet its typically defered it causes room for missed bugs. So what i've seen some people do is
func logError(f func () error) {
err := f()
if err != nil {
log15.Error("something happened!", "error", err)
}
}
then instead of defer resp.Body.Close() they do defer logError(resp.Body.Close)
There was a problem hiding this comment.
Just want your opinion on it. I think it does nothing but help, even if you don't have all the context you normally would have when logging errors
|
|
||
| bodyMap := make(map[string] string) | ||
|
|
||
| json.Unmarshal(bodyBytes, &bodyMap) |
There was a problem hiding this comment.
anything that returns an error should be handled. it gets annoying cause its so boiler plate but having
err := f()
if err != nil {
// do something
// maybe just return err
}
is proper because it provides for more robust code.
theres been interesting conversation relating to error handling in go and some are proposals for Go 2
| } | ||
|
|
||
| // getBodyJson takes an http.Response pointer and reads all of its data into a map | ||
| func getBodyJson(response *http.Response) (map[string] string, error) { |
There was a problem hiding this comment.
when i better understand the overall project I'll have better input. But I have a feeling there is a better way to represent the body than map[string]string. A lot of times people make structs with json tags and when you throw that into json.Unmarshal it works easy. Then you have your own custom representation that you can cater towards readability. ill find a link
There was a problem hiding this comment.
I think this is the right link. Its for marshal but works both ways
https://golang.org/pkg/encoding/json/#Marshal
|
also noticed that the package |
| botId = os.Getenv(botIdEnvironmentVar) | ||
| } | ||
|
|
||
| return botId |
There was a problem hiding this comment.
this is familiar. I forget why its familiar but this is a pattern ive seen before. Go likes sourcing all environment variables up front and in one spot if possible.
Practically this is very useful because you dont want to find out youre missing an env var only when x happens.
You do want to find out when you deploy, or practice deploying. cause if you fail a deploy you can rollback and try again later. if you fail only when x happens then you have a bug.
Having it in one place makes it clear to someone else what is required. Thats why people either source env vars in main.go or in an init() per package. theres a link somewhere to what init() does
There was a problem hiding this comment.
Nah actually dont use init. declare them all in the top level var block and either set a default or exit if it doesnt exist. At work we have utils.GetEnv{type}(envName string, default {type} and utils.MustGetEnv{type}(envName string) which does os.Exit(1) if envName points to an empty value
| @@ -0,0 +1,32 @@ | |||
| package types | |||
There was a problem hiding this comment.
generally not desired to have a package for common, base, or utils and this feels like it fits in that rules. If you have to use one of these packages then it might mean your packages are too thin and this is just avoiding a dependency cycle. If not then cool put them where they need to be, but it could be a good way to help organize things.
Try and think in terms of "containment" rather than "category" when packaging stuff
Rip my code a new one