There's quite a lot of code to go through, so I'll revisit this review, and periodically update. For now, some small things that stand out in bullet-points: * Use `gofmt` & co. Although it might sound like an insignificant pedantic comment, it immediately shows that you're not using `gofmt`, `goimports`, and `golint`. The order of your `import`'s is inconsistent (recommended order is: standard packages, local packages belonging to the same project, third party packages, in alphabetical order)<br> Each of the three groups should be separated by a blank line. [cf golang code review comments](https://github.com/golang/go/wiki/CodeReviewComments#imports) * You have a few `if something { return } else {return}` at the _end_ of functions. The `else` is completely redundant, just omit it in favour of the more readable `if something { return } return` * Avoid things like `x := make([]type, 0)` Either use `var x []type` (which doesn't allocate immediately), or create literals (which requires less typing): `x := []type{}` * Unlike other languages, constants in go are usually not written in `ALL_CAPS`, but mixed caps like any other name.<br> Package constants and global variables are preferably grouped, too. Rather than writing `const ...\n const ...`, just write `const ( /* constants here */)` It makes code more readable IMO * I noticed a fair bit of error-string comparisons. Especially this: `err.Error() != REDIS_NIL`. Rather then calling `Error()` every time, it might be better just to have `RedisNil` as a package variable of type `error`: `var RedisNil error = errors.New("error message")`. In the same way that you check `err == io.EOF` when reading files in go. Having `RedisNil` in your package is also wrong. It's an error returned by a different package. If that package changes, you have to update 2 packages. It's best to declare that variable/constant in the redis package itself, That allows you to write the cleaner `err == rcl.RedisNil`. * `RedisNil` is acutually not the best name. Going back to the golang code review comments link I included earlier: package aliasing is not recommended. What is also advised against is duplication. You have a redisclient package. Reading something like `redisclient.RedisNil` is a bit silly. I'd simply call the package `redis`, and the error `Nil`, so I can write: `err == redis.Nil`. It's just as communicative. * Although I haven't seen an explicit _"ban"_ on it, I notice you're using variable names starting with an underscore (like `_name,_ := gm.GetPlayerName(gameName, _pid)`). It's something that I've rarely (not to say never) seen in go. To me, it looks messy. I initially thought you had 3 return values, and were only interested in the second one. It interrupts the person who's reading through your code. For that reason alone, I wanted to point it out as something I consider to be code smell. ### Now a bit more specific stuff: You're logging in goroutines - Your code is littered with `go log.Printf` calls. I would advise against this use of goroutines. As we all know, things can (and do) go wrong from time to time. That's why we log stuff. Below is a small code snippet that demonstrates that spawning a goroutine that attempts to log things can in fact cause you to lose data you're trying to log: func someLogicFunc() { rand.Seed(123) // or whatever defer func() { if _, err := doSomething(rand.Int()); err != nil { panic("Evil panic") } }() for i := 0; i < 10 ; i++ { nv, err := doSomething((i + rand.Int()) * (i + 1)) if err { os.Exit(1) // no recovery, immediate termination } } } func doSomething(n int) (int, error) { go log.Printf("Call to doSomething with argument %v", n) if n%2 == 0 { go log.Printf("Returned error because of even number") return 0, errors.New("Only odd numbers allowed") } return n*3 + 1, nil } func main() { someLogicFunc() doSomething(123) doSomething(2) } Looking at this code, are you 100% confident that every call to `log.Printf` will in fact show up in the logs? I'm not, in fact the last call in the `main` function will almost certainly return before both log routines have written the data. And when the last `doSomething` call returns, the program terminates, without waiting for non-main routines to terminate, as described in [the spec](https://golang.org/ref/spec#Program_execution) > Program execution begins by initializing the main package and then invoking the function main. When that function invocation returns, the program exits. It does not wait for other (non-main) goroutines to complete. One more thing I noticed was this flat out ***evil*** bit of code that actively _hides_ the fact that something went wrong: b, jerr := json.Marshal(playeScores) if jerr == nil { return string(b) } else { return "json error" } For a start, it's an example of something I mentioned earlier, the `else` can be omitted, but the true problem here is that `json.Marshal` returned an error, and you're returning a string. You're relying on the caller to know the return value needs to be checked for a specific string that indicates an error occurred. Why are you not returning the error value? I would urge you to write this instead. b, jerr := json.Marshal(playeScores) if jerr != nil { return "", jerr } return string(b), nil At the very least, the caller can then log the _actual_ error value, making it easier to work out what actually went wrong. As it stands, even if the caller is checking the return value, and knows what to check, all it that can be logged is the rather obscure message _"json error"_. That's not enough. ---- A bit late, but here's the first of some more specific things (lunch break review): func New() (gm * GameManager) { return &GameManager{ rc: rcl.New(), } } I fail to see the point in having named return values. They're useful in cases where a _naked return_ actually helps improve readability. In this case, your code is equivalent to: func New() *GameManager { return &GameManager{ rc: rcl.New(), } } Which is shorter, and just as readable. A naked return/named returns can be useful when you do something like this: func New(params ...interface{}) (g *GameManager, err error) { g, err = nonExposedNew(params...) if err != nil { log.Debug("Log something") return // returns g (nil ptr) and err } err = g.someSetupFunc() if err != nil { log.Error("failed to setup GameManager") g.unload() // cleanup if needed g = nil // do not return return // returns nil, setup error } return // returns g, nil on success }