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 & go. 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)
Each of the three groups should be separated by a blank line. cf golang code review comments
- 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.
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
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.
