0
\$\begingroup\$

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.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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)
    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.


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
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.