In my previous question I asked how could the Redis client be improved. Here I will ask how can the Game data Client can be improved. Game Data client sits on top of Redis Client and uses datatypes from datape package
Here is the code:
package gamedatamanager
import (
"log"
"reflect"
"encoding/json"
"sort"
"gogameserver/util"
dt "gogameserver/datatypes"
rcl "gogameserver/redisclient"
)
const REDIS_NIL string = "redis: nil"
type GameManager struct {
rc *rcl.RedisClient
}
func New() (gm * GameManager) {
return &GameManager{
rc: rcl.New(),
}
}
func (gm * GameManager) DelKey(key string) (int64, bool) {
redisRet, err := gm.rc.DelKey(key)
if err != nil && err.Error() != REDIS_NIL {
go log.Printf("\nERROR:DelKey| Key: %s not found, err:%v", key, err)
return -1, false
} else {
return redisRet, true
}
}
func (gm * GameManager) GetPlayerData(gameName string, playerId string) (string, bool) {
gameName = gameName+playerId
playerDataStr, err := gm.rc.GetVal(gameName)
if err != nil && err.Error() == REDIS_NIL {
go log.Printf("\nERROR:GetPlayerData: Game %s, playerId %s not found, err:%v", gameName, playerId, err)
return "player not found", false
} else {
return playerDataStr, true
}
}
func (gm * GameManager) DelPlayerData(gameName string, playerId string) (int64, bool) {
gm.DelPlayerName(gameName, playerId)
gameName = gameName+playerId
redisRet, err := gm.rc.DelKey(gameName)
if err != nil && err.Error() != REDIS_NIL {
go log.Printf("\nERROR:DelPlayerData: Game %s, playerId %s not found, err:%v", gameName, playerId, err)
return -1, false
} else {
return redisRet, true
}
}
// store player data
func (gm * GameManager) StorePlayerData(gameName string, playerData dt.PlayerData) (bool){
gm.StorePlayerName(gameName, playerData.N, playerData.I)
gameName = gameName+playerData.I
err := gm.rc.SaveKeyValForever(gameName, dt.Str(playerData))
if err != nil && err.Error() != REDIS_NIL {
go log.Printf("\nERROR:StorePlayerData: Game %s, playerData %v, err:%v", gameName, playerData, err)
return false
} else {
go log.Printf("\nInfo: Success StorePlayerData: Game %s, playerData %v", gameName, playerData)
return true
}
}
// store player new score
func (gm * GameManager) StorePlayerScore(gameName string, score float64, playerId string) (bool){
currHiScore, err := gm.GetPlayerHighScore(gameName, playerId)
if err != nil && err.Error() != REDIS_NIL {
go log.Printf("ERROR:StorePlayerScore: Game %s, playerId %s not found. err:'%s'", gameName, playerId, err.Error() )
return false
} else {
hiScoretoday, _ := gm.GetPlayerScoreOnDay(gameName, playerId , 0)
if hiScoretoday < score {
gm.StorePlayerScoreDaily(gameName , score , playerId)
}
if currHiScore < score {
pDataStr, found := gm.GetPlayerData(gameName, playerId)
if !found {
return false
}
pData := dt.JsonFromStr(pDataStr)
pData.A = score
// a go routine to update
gm.StorePlayerData(gameName, pData)
redisRet, redisErr := gm.rc.AddToSet(gameName, score, playerId)
if redisErr != nil && redisErr.Error() != REDIS_NIL {
go log.Printf("Error:AddToSet: SUCESS gameName:%s, score:%f, playerId:%s, redisErr:%v", gameName, score, playerId, redisErr)
return false
}
go log.Printf("Info:StorePlayerScore: SUCESS currHiScore:%.6f, newScore:%.6f, Game %s, playerId %s, retcode:%d", currHiScore, score, gameName, playerId, redisRet)
} else {
go log.Printf("Info:StorePlayerScore: Already high currHiScore:%.6f, newScore:%.6f, Game %s, playerId %s", currHiScore, score, gameName, playerId)
}
return true
}
}
func getKeyForName(gameName string, playerId string) string {
return gameName+playerId+"Name"
}
// store player new score
func (gm * GameManager) StorePlayerName(gameName string, playerName string, playerId string) (bool){
key := getKeyForName(gameName, playerId)
err := gm.rc.SaveKeyValForever(key, playerName)
if err != nil && err.Error() != REDIS_NIL {
go log.Printf("\nERROR:StorePlayerName: Game %s, playerName %s, err:%v", gameName, playerName, err)
return false
} else {
go log.Printf("\nInfo: Success StorePlayerName: Game %s, playerName %s", gameName, playerName)
return true
}
}
// store player new score
func (gm * GameManager) GetPlayerName(gameName string, playerId string) (string, bool){
key := getKeyForName(gameName, playerId)
playerName, err := gm.rc.GetVal(key)
if err != nil && err.Error() == REDIS_NIL {
go log.Printf("\nERROR:GetPlayerName: Game %s, playerId %s not found, err:%v", gameName, playerId, err)
return "playerName not found", false
} else {
return playerName, true
}
}
// store player new score
func (gm * GameManager) DelPlayerName(gameName string, playerId string) (int64, bool){
key := getKeyForName(gameName, playerId)
redisRet, err := gm.rc.DelKey(key)
if err != nil && err.Error() != REDIS_NIL {
go log.Printf("\nERROR:DelPlayerName: Game %s, playerId %s not found, err:%v", gameName, playerId, err)
return -1, false
} else {
return redisRet, true
}
}
// delete player score
func (gm * GameManager) DeletePlayerScore(gameName string, playerId string) (bool){
redisRet, redisErr := gm.rc.RemScore(gameName, playerId)
if redisErr != nil && redisErr.Error() != REDIS_NIL {
go log.Printf("Error:DeletePlayerScore: SUCESS gameName:%s, playerId:%s, redisErr:%v", gameName, playerId, redisErr)
return false
} else {
go log.Printf("Info :DeletePlayerScore: SUCESS gameName:%s, playerId:%s, redisRet:%d", gameName, playerId, redisRet)
}
return true
}
// get player rank
func (gm * GameManager) GetPlayerRank(gameName string, playerId string) (int64) {
rank, err := gm.rc.GetRank(gameName, playerId)
if err != nil && err.Error() != REDIS_NIL {
go log.Printf("Error:GetPlayerRank: Game %s, playerId %s", gameName, playerId)
return -1;
} else {
return rank+1;
}
}
// get top x
func (gm * GameManager) GetTopPlayers(gameName string, topCount int64) (string) {
playeScores := make([]dt.PlayerScore, 0)
topResults, err := gm.rc.GetTop(gameName, topCount)
if err != nil && err.Error() != REDIS_NIL{
go log.Printf("Error:GetTopPlayers: Game %s", gameName)
return "json error"
}
topResultsVal := reflect.ValueOf(topResults)
resultCount := topResultsVal.Len()
for i:=0; i<resultCount; i++ {
_score,_ := topResultsVal.Index(i).Field(0).Interface().(float64)
_pid,_ := topResultsVal.Index(i).Field(1).Interface().(string)
_name,_ := gm.GetPlayerName(gameName, _pid)
playeScores = append(playeScores, dt.PlayerScore{_name, _score})
}
sort.Sort(dt.ByScoreRev(playeScores))
if topCount < int64(len(playeScores)) {
playeScores = playeScores[:topCount]
}
go log.Printf("Info: GetTopPlayers: top %d playeScores: %v", topCount, playeScores)
b, jerr := json.Marshal(playeScores)
if jerr == nil {
return string(b)
} else {
return "json error"
}
}
// get score
func (gm * GameManager) GetPlayerHighScore(gameName string, playerId string) (float64, error) {
return gm.rc.GetScore(gameName, playerId)
}
// store player new score for a week
func (gm * GameManager) StorePlayerScoreOnADay(gameName string, score float64, playerId string, numOfDaysOld int) {
gm.rc.AddToSet(gameName+util.GetDate(numOfDaysOld), score, playerId)
}
func (gm * GameManager) DeletePlayerScoreOnADay(gameName string, playerId string, numOfDaysOld int) (bool){
gameName = gameName+util.GetDate(numOfDaysOld)
redisRet, redisErr := gm.rc.RemScore(gameName, playerId)
if redisErr != nil && redisErr.Error() != REDIS_NIL {
go log.Printf("Error:DeletePlayerScoreOnADay: SUCESS gameName:%s, playerId:%s, redisErr:%v", gameName, playerId, redisErr)
return false
} else {
go log.Printf("Info :DeletePlayerScoreOnADay: SUCESS gameName:%s, playerId:%s, redisRet:%d", gameName, playerId, redisRet)
}
return true
}
// store player new score for a week
func (gm * GameManager) StorePlayerScoreDaily(gameName string, score float64, playerId string) {
gm.rc.AddToSet(gameName+util.CurrentDate(), score, playerId)
}
// store player new score for a week
func (gm * GameManager) GetPlayerScoreOnDay(gameName string, playerId string, numOfDaysOld int) (float64, error) {
return gm.GetPlayerHighScore(gameName+util.GetDate(numOfDaysOld), playerId)
}
// get top weekly 1000
func (gm * GameManager) GetTopPlayersOnDay(gameName string, topCount int64, numOfDaysOld int) (string) {
if numOfDaysOld > 6 {
return "";
}
playeScores := make([]dt.PlayerScore, 0)
dateGameName := gameName+util.GetDate(numOfDaysOld)
topResults, err := gm.rc.GetTop(dateGameName, topCount)
if err != nil && err.Error() != REDIS_NIL{
go log.Printf("Error:GetTopPlayersOnDay: dateGameName %s", dateGameName)
return "json error"
}
topResultsVal := reflect.ValueOf(topResults)
resultCount := topResultsVal.Len()
for i:=0; i<resultCount; i++ {
_score,_ := topResultsVal.Index(i).Field(0).Interface().(float64)
_pid,_ := topResultsVal.Index(i).Field(1).Interface().(string)
_name,_ := gm.GetPlayerName(gameName, _pid)
playeScores = append(playeScores, dt.PlayerScore{_name, _score})
}
sort.Sort(dt.ByScoreRev(playeScores))
if topCount < int64(len(playeScores)) {
playeScores = playeScores[:topCount]
}
go log.Printf("Info: GetTopPlayersOnDay: top %d playeScores: %v", topCount, playeScores)
b, jerr := json.Marshal(playeScores)
if jerr == nil {
return string(b)
} else {
return "json error"
}
}
func (gm * GameManager) GetTopPlayersThisWeek(gameName string, topCount int64) (string) {
playeScores := make([]dt.PlayerScore, 0)
donePersons := make(map[string] bool)
for i:=6; i>=0; i--{
topResults, err := gm.rc.GetTop(gameName+util.GetDate(i), topCount)
if err != nil && err.Error() != REDIS_NIL{
go log.Printf("Error:GetTopPlayersThisWeek: Game %s", gameName)
continue
}
topResultsVal := reflect.ValueOf(topResults)
resultCount := topResultsVal.Len()
for i:=0; i<resultCount; i++ {
_score,_ := topResultsVal.Index(i).Field(0).Interface().(float64)
_pid,_ := topResultsVal.Index(i).Field(1).Interface().(string)
_name,_ := gm.GetPlayerName(gameName, _pid)
playeScores = append(playeScores, dt.PlayerScore{_name, _score})
}
}
sort.Sort(dt.ByScoreRev(playeScores))
for i:=0; i<len(playeScores) && i<int(topCount); i++ {
if _,ok := donePersons[playeScores[i].N]; !ok {
donePersons[playeScores[i].N] = true
} else {
playeScores = append(playeScores[:i], playeScores[i+1:]...) // perfectly fine if i is the last element https://play.golang.org/p/hcUEguyiTC
i--
}
}
if topCount < int64(len(playeScores)) {
playeScores = playeScores[:topCount]
}
go log.Printf("Info: GetTopPlayersThisWeek: top %d playeScores: %v", topCount, playeScores)
b, jerr := json.Marshal(playeScores)
if jerr == nil {
return string(b)
} else {
return "json error"
}
}
// get rank among friends
func (gm * GameManager) GetScoreOfFriends(gameName string, playerId string, friendIds []string) (string) {
var topPlayersWithScores dt.ResponseData
playerScore, err := gm.GetPlayerHighScore(gameName, playerId)
if err != nil && err.Error() != REDIS_NIL {
go log.Printf("Error:GetScoreOfFriends: Error: %v", err)
return ""
}
totalCount := len(friendIds) + 1
topPlayersWithScores.PlayerIds = make([]string, totalCount)
topPlayersWithScores.Scores = make([]float64, totalCount)
for i:=1; i<totalCount; i++ {
topPlayersWithScores.PlayerIds[i] = friendIds[i-1]
topPlayersWithScores.Scores[i] = -1
score, err1 := gm.GetPlayerHighScore(gameName, friendIds[i-1])
if err1 != nil && err1.Error() != REDIS_NIL {
go log.Printf("Error:GetScoreOfFriends: Game %s, %v", gameName, err1)
} else {
topPlayersWithScores.Scores[i] = score
}
}
topPlayersWithScores.PlayerIds[0] = playerId
topPlayersWithScores.Scores[0] =playerScore
b, jerr := json.Marshal(topPlayersWithScores)
if jerr == nil {
return string(b)
} else {
return "json error"
}
}
Here is the test code:
package gamedatamanager_test
import (
dt "gogameserver/datatypes"
gdm "gogameserver/gamedatamanager"
util "gogameserver/util"
"testing"
"math/rand"
"time"
"strconv"
"encoding/json"
"sort"
)
const GAMENAME string = "00dummygame"
const PLAYERID string = "00playerid"
var PLAYERIDS = [] string{"00NeverAddThispid0", "00NeverAddThispid1", "00NeverAddThispid3", "00NeverAddThispid4", "00NeverAddThispid5"}
func TestStorePlayerData(t *testing.T) {
gm := gdm.New()
playerData := dt.NewWithId(PLAYERID)
done := gm.StorePlayerData(GAMENAME, playerData)
if !done {
t.Errorf("Data shall have been stored\n")
}
gm.DelPlayerData(GAMENAME, PLAYERID)
}
func TestGetPlayerData(t *testing.T) {
gm := gdm.New()
playerData := dt.NewWithId(PLAYERID)
done := gm.StorePlayerData(GAMENAME, playerData)
if !done {
t.Errorf("Error: Data shall have been stored\n")
}
playerDataStr := dt.Str(playerData)
playerDataStrFromDB, found := gm.GetPlayerData(GAMENAME, PLAYERID)
if !found {
t.Errorf("Error: GetPlayerData: Data shall have been found\n")
} else {
if playerDataStrFromDB != playerDataStr {
t.Errorf("Error: playerDataStr from redis: %s\n\tplayerDataStr shall have been %s\n", playerDataStrFromDB, playerDataStr)
}
}
gm.DelPlayerData(GAMENAME, PLAYERID)
}
func TestStorePlayerScore(t *testing.T) {
gm := gdm.New()
playerData := dt.NewWithId(PLAYERID)
playerData.A = 10.0
newScore := 10000.0
gm.DeletePlayerScore(GAMENAME, PLAYERID)
gm.StorePlayerData(GAMENAME, playerData)
success := gm.StorePlayerScore(GAMENAME, newScore, PLAYERID)
if !success {
t.Errorf("Error: StorePlayerScore: Data shall have been found\n")
} else {
newHiScore, err := gm.GetPlayerHighScore(GAMENAME, PLAYERID)
if err != nil {
t.Errorf("TestStorePlayerScore Error: GetPlayerHighScore, Error: %v\n", err)
} else {
if newHiScore != newScore {
t.Errorf("TestStorePlayerScore Error: newHiScore=%d. Added newScore=%d\n", newHiScore, newScore)
}
playerDataStrFromDB, success := gm.GetPlayerData(GAMENAME, PLAYERID)
if !success {
t.Errorf("TestStorePlayerScore Error: GetPlayerData\n")
} else {
playerData.A = newScore
playerDataStr := dt.Str(playerData)
if playerDataStrFromDB != playerDataStr {
t.Errorf("TestStorePlayerScore Error: playerDataStr from redis: %s\n\tplayerDataStr shall have been %s\n", playerDataStrFromDB, playerDataStr)
}
}
}
}
gm.DelPlayerData(GAMENAME, PLAYERID)
gm.DeletePlayerScore(GAMENAME, PLAYERID)
}
func TestGetPlayerRank(t *testing.T) {
gm := gdm.New()
scores := []float64{2,1,7,4, 3}
for i:=0; i<5; i++ {
gm.DeletePlayerScore(GAMENAME, PLAYERIDS[i])
playerData := dt.NewWithId(PLAYERIDS[i])
playerData.A = scores[i]
gm.StorePlayerData(GAMENAME, playerData)
gm.StorePlayerScore(GAMENAME, scores[i], PLAYERIDS[i])
}
rank := gm.GetPlayerRank(GAMENAME, PLAYERIDS[2])
if rank != 1{
t.Errorf("TestGetPlayerRank Error: Player rank should have been 1 but is : %d\n", rank)
}
for i:=0; i<5; i++ {
gm.DelPlayerData(GAMENAME, PLAYERIDS[i])
gm.DeletePlayerScore(GAMENAME, PLAYERIDS[i])
}
}
func TestGetScoreOfFriends(t *testing.T) {
gm := gdm.New()
scores := []float64{2,1,7,4, 3}
for i:=0; i<5; i++ {
gm.DeletePlayerScore(GAMENAME, PLAYERIDS[i])
playerData := dt.NewWithId(PLAYERIDS[i])
playerData.A = scores[i]
gm.StorePlayerData(GAMENAME, playerData)
gm.StorePlayerScore(GAMENAME, scores[i], PLAYERIDS[i])
}
scoresOfFriends := gm.GetScoreOfFriends(GAMENAME, PLAYERIDS[0], PLAYERIDS[2:5])
scoresOfFriendsExpectedStr := "{\"PlayerIds\":[\"00NeverAddThispid0\",\"00NeverAddThispid3\",\"00NeverAddThispid4\",\"00NeverAddThispid5\"],\"Scores\":[2,7,4,3]}"
if scoresOfFriends != scoresOfFriendsExpectedStr{
t.Errorf("TestGetScoreOfFriends Error: scoresOfFriends str is : %s\n", scoresOfFriends)
}
for i:=0; i<5; i++ {
gm.DelPlayerData(GAMENAME, PLAYERIDS[i])
gm.DeletePlayerScore(GAMENAME, PLAYERIDS[i])
}
}
func TestGetTopPlayers(t *testing.T) {
gm := gdm.New()
scores := []float64{2,1,7,4, 3}
names := make([] string, 5)
playeScores := make([]dt.PlayerScore, 0)
gm.DelKey(GAMENAME)
for i:=0; i<5; i++ {
names[i] = util.RandStringRunes(5)
gm.DeletePlayerScore(GAMENAME, PLAYERIDS[i])
playerData := dt.NewWithId(PLAYERIDS[i])
playerData.A = scores[i]
playerData.N = names[i]
gm.StorePlayerData(GAMENAME, playerData)
gm.StorePlayerScore(GAMENAME, scores[i], PLAYERIDS[i])
playeScores = append(playeScores, dt.PlayerScore{names[i], scores[i]})
}
sort.Sort(dt.ByScoreRev(playeScores))
topCount := 3
top3 := gm.GetTopPlayers(GAMENAME, int64(topCount) )
var top3Scorers []dt.PlayerScore
json.Unmarshal([]byte(top3), &top3Scorers)
for i:=0; i<topCount; i++{
if playeScores[i].N != top3Scorers[i].N || playeScores[i].S != top3Scorers[i].S {
t.Errorf("Error: TestGetTopPlayers, playeScores[i] : %v\n, top3Scorers[i]: %v\n", playeScores[i], top3Scorers[i])
}
}
for i:=0; i<5; i++ {
gm.DelPlayerData(GAMENAME, PLAYERIDS[i])
gm.DeletePlayerScore(GAMENAME, PLAYERIDS[i])
}
}
func TestGetTopPlayersThisWeek(t *testing.T) {
gm := gdm.New()
r := rand.New(rand.NewSource(time.Now().UnixNano()))
scores := make([] float64, 0)
names := make([] string, 10)
id := make([] string, 10)
for i:=0; i<10; i++ {
names[i] = util.RandStringRunes(5)
id[i] = strconv.Itoa(i)
playerData := dt.NewWithId(id[i])
playerData.N = names[i]
gm.StorePlayerData(GAMENAME, playerData)
}
playerMaxScore := make(map[string] float64)
for i:=0; i<10; i++ {
playerMaxScore[names[i]] = 0
}
// for 7 days
for d:=0; d<7; d++ {
gm.DelKey(GAMENAME+util.GetDate(d))
for i:=0; i<10; i++ {
score := r.Float64()*100
scores = append(scores, score)
if playerMaxScore[names[i]] < score {
playerMaxScore[names[i]] = score
}
gm.StorePlayerScoreOnADay(GAMENAME, score, id[i], d)
}
}
playeScores := make([]dt.PlayerScore, 0)
for k,v := range playerMaxScore {
playeScores = append(playeScores, dt.PlayerScore{k, v})
}
sort.Sort(dt.ByScoreRev(playeScores))
topCount := 5
topWeeklyScorersJson := gm.GetTopPlayersThisWeek(GAMENAME, int64(topCount) )
var topWeeklyScorers []dt.PlayerScore
json.Unmarshal([]byte(topWeeklyScorersJson), &topWeeklyScorers)
for i:=0; i<topCount; i++{
if playeScores[i].N != topWeeklyScorers[i].N || playeScores[i].S != topWeeklyScorers[i].S {
t.Errorf("Error: TestGetTopPlayersThisWeek, playeScores[i] : %v\n, topWeeklyScorers[i]: %v\n", playeScores[i], topWeeklyScorers[i])
}
}
for i:=0; i<10; i++ {
gm.DelPlayerData(GAMENAME, id[i])
}
for d:=0; d<7; d++ {
for i:=0; i<10; i++ {
gm.DeletePlayerScoreOnADay(GAMENAME , id[i], d)
}
}
}
Here is the complete GoGameServer: https://github.com/ediston/gogameserver
Please review it and tell me how can I write better code.
1 Answer 1
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 usinggofmt
,goimports
, andgolint
. The order of yourimport
'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. Theelse
is completely redundant, just omit it in favour of the more readableif something { return } return
- Avoid things like
x := make([]type, 0)
Either usevar 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 writingconst ...\n const ...
, just writeconst ( /* 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 callingError()
every time, it might be better just to haveRedisNil
as a package variable of typeerror
:var RedisNil error = errors.New("error message")
. In the same way that you checkerr == io.EOF
when reading files in go. HavingRedisNil
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 cleanererr == 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 likeredisclient.RedisNil
is a bit silly. I'd simply call the packageredis
, and the errorNil
, 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.
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
}