Redis Client: Go based Game server

Question

Here is my Go code to interact with a Redis server

package redisclient
import (
 "time"
 "gopkg.in/redis.v5"
)
type RedisClient struct {
 client *redis.Client
}
func New() (rc * RedisClient) {
 return &RedisClient{
 client: redis.NewClient(&redis.Options{
 Addr: "localhost:6379",
 DialTimeout: 10 * time.Second,
 ReadTimeout: 30 * time.Second,
 WriteTimeout: 30 * time.Second,
 PoolSize: 10000,
 PoolTimeout: 30 * time.Second,
 }),
 }
}
func (rc *RedisClient) SetClient(){
 if rc.client != nil{
 return
 }
 rc.client = redis.NewClient(&redis.Options{
 Addr: "localhost:6379",
 DialTimeout: 10 * time.Second,
 ReadTimeout: 30 * time.Second,
 WriteTimeout: 30 * time.Second,
 PoolSize: 10000,
 PoolTimeout: 30 * time.Second,
 })
}
// dur = int64 nanosecond count.
func (rc *RedisClient) SaveKeyValTemporary(key string, val interface{}, dur time.Duration) error{
 rc.SetClient()
 err := rc.client.Set(key, val, dur).Err()
 if err != nil {
 return err
 }
 return nil
}
// 
func (rc *RedisClient) SaveKeyValForever(key string, val interface{}) error{
 rc.SetClient()
 return rc.SaveKeyValTemporary(key, val, 0)
}
// 
func (rc *RedisClient) DelKey(key string) (int64, error){
 rc.SetClient()
 return rc.client.Del(key).Result()
}
// 
func (rc *RedisClient) KeyExists(key string) (bool, error){
 rc.SetClient()
 return rc.client.Exists(key).Result()
}
// 
func (rc *RedisClient) GetVal(key string) (string, error){
 rc.SetClient()
 return rc.client.Get(key).Result()
}
func (rc *RedisClient) AddToSet(setName string, Score float64, Member interface{}) (int64, error){
 rc.SetClient()
 return rc.client.ZAdd(setName, redis.Z{Score, Member}).Result()
}
 // returns ([]Z, error)
func (rc *RedisClient) GetTop(setName string, topAmount int64) (interface{}, error){
 rc.SetClient()
 if topAmount <= 0 {
 topAmount = 1
 }
 return rc.client.ZRevRangeWithScores(setName, 0, topAmount-1).Result()
}
// Rank starts from 0
func (rc *RedisClient) GetRank(setName string, key string) (int64, error){
 rc.SetClient()
 return rc.client.ZRevRank(setName, key).Result()
}
func (rc *RedisClient) GetScore(setName string, key string) (float64, error){
 rc.SetClient()
 return rc.client.ZScore(setName, key).Result()
}
func (rc *RedisClient) RemScore(setName string, key string) (int64, error){
 rc.SetClient()
 return rc.client.ZRem(setName, key).Result()
}

And this is the package to test RedisClient

package redisclient_test
import (
 rcl "gogameserver/redisclient" 
 "testing"
 "reflect"
 "time"
)
const tempKeyStr string = "00NeverAddThiskeytemp"
const keyStr string = "00NeverAddThiskey"
const valStr string = "00NeverAddThisVal"
const setName string = "00NeverAddThisSet"
const setKey string = "00NeverAddThisSetKey"
var tempStrs = [] string{"00NeverAddThiskey0", "00NeverAddThiskey1", "00NeverAddThiskey2", "00NeverAddThiskey3", "00NeverAddThiskey4", "00NeverAddThiskey5"}
func TestSaveKeyValTemporary(t *testing.T) {
 rc := rcl.New()
 rc.SaveKeyValTemporary(tempKeyStr, valStr, 1*time.Second) // 10 seconds 10*1000 000 000
 exists,_ := rc.KeyExists(tempKeyStr)
 if !exists {
 t.Errorf("Key should exist!")
 }
 time.Sleep(3 * time.Second)
 exists,_ = rc.KeyExists(tempKeyStr)
 if exists {
 t.Errorf("Key should be deleted!")
 rc.DelKey(tempKeyStr)
 }
}
func TestSaveKeyValForever(t *testing.T) {
 rc := rcl.New()
 rc.SaveKeyValForever(keyStr, valStr)
 exists,_ := rc.KeyExists(keyStr)
 if !exists {
 t.Errorf("Key should exist!")
 }
 rc.DelKey(keyStr)
}
func TestGetVal(t *testing.T) {
 rc := rcl.New()
 rc.SaveKeyValForever(keyStr, valStr)
 tempVal, _ := rc.GetVal(keyStr) 
 if valStr != tempVal{
 t.Errorf("Key should exist and be equal to %s!", valStr)
 }
 rc.DelKey(keyStr)
}
func TestAddToSet(t *testing.T) {
 rc := rcl.New()
 score := 12.0
 rc.AddToSet(setName, score, setKey)
 tempScore, _ := rc.GetScore(setName, setKey)
 if tempScore != score {
 t.Errorf("Stored Score is wrong!")
 }
 rc.RemScore(setName, setKey)
}
func TestGetTop(t *testing.T) {
 rc := rcl.New()
 scores := []float64{2,1,7,4, 3}
 rev_sorted_scores := []float64{7,4,3}
 for i:=0; i<5; i++ {
 rc.AddToSet(setName, scores[i], tempStrs[i])
 }
 top3,_ := rc.GetTop(setName, 3)
 s := reflect.ValueOf(top3)
 for i:=0; i<3; i++ {
 f := s.Index(i).Field(0)
 if rev_sorted_scores[i] != f.Interface() {
 t.Errorf("%d: %s = %v\n", i, f.Type(), f.Interface())
 }
 }
 for i:=0; i<5; i++ {
 rc.RemScore(setName, tempStrs[i])
 }
}
func TestGetRank(t *testing.T) {
 rc := rcl.New()
 scores := []float64{2,1,7,4, 3}
 for i:=0; i<5; i++ {
 rc.AddToSet(setName, scores[i], tempStrs[i])
 }
 rank,_ := rc.GetRank(setName, tempStrs[2])
 if rank != 0{
 t.Errorf("Rank is : %d\n", rank)
 }
 for i:=0; i<5; i++ {
 rc.RemScore(setName, tempStrs[i])
 }
}
func TestGetScore(t *testing.T) {
 rc := rcl.New()
 scores := []float64{2,1,7,4, 3}
 for i:=0; i<5; i++ {
 rc.AddToSet(setName, scores[i], tempStrs[i])
 }
 for i:=0; i<5; i++ {
 score, _ := rc.GetScore(setName, tempStrs[i])
 if score != scores[i] {
 t.Errorf("%d: Expected score: %f. Score is %f\n", i, scores[i], score)
 }
 }
 for i:=0; i<5; i++ {
 rc.RemScore(setName, tempStrs[i])
 }
}
func TestRemScore(t *testing.T) {
 rc := rcl.New()
 rc.AddToSet(setName, 2, tempStrs[0])
 rc.RemScore(setName, tempStrs[0])
 score, _ := rc.GetScore(setName, tempStrs[0])
 if score != 0 {
 t.Errorf("%s exists with score: %f\n", tempStrs[0], score)
 }
}

Can above code be done better?

Repo is here: https://github.com/ediston/gogameserver

• Simon Forsberg: You have quite a bit of code here but not much description about how it all fits together or how it works. I'd like to recommend you to follow some of the tips in how to post a good Code Review question
• Gujarat Santana: and one more thing, please remove the commented code, if you don't use it just delete it. if not it will confuse people who read it and it takes time to understand

Answer 1

Here are some thoughts on how to improve your code.

No need for a separate package

That package only contains redisclient so unless you plan on expanding drastically what the package provides, it fits in its own few files in your main project (gogameserver).

Avoid function names that are not explicit, e.g. New()

In relation with the first item, you should rename your function New() to NewRedisClient(). That way when browsing the documentation of your library, one knows from the table of contents what the function should do.

SetClient is not useful enough

Instead of calling SetClient every time, you should let the user of the library face a crash if they try to use an uninitialized object to attempt to read anything from Redis. In addition, the code seems extremely similar to what's in New. Code duplication is usually a big and resounding no in any programming language (provides issues in maintainability).

Use environment variables

Both New and SetClient have hardcoded values. So if the address of your Redis box changes, you need to recompile the code. It may not seem like a big problem at first, but if/when you start deploying several instances of your code across different machines, there's a big deployment time difference between changing an environment variable and restarting the binary, and recompiling the code and re-deploying the several megabyte binary across your datacenter.

A way I tend to do this is by checking that all the environment variables are set at the start of the program, panic if not, and then go on as if they are set (e.g. https://github.com/ChristopherRabotin/gofetch/blob/master/settings.go#L15 ).

Extend Redis' client instead of having a pointer

Since your struct only has a pointer, you just extend/embed it via an anonymous field to add your methods, cf this old blog post and this not as old blog post.

Avoid returning interface{}

Usually in go, functions accept interfaces and return concretes. I haven't checked the documentation of Redis, but if you can infer the concrete type of the interface in your GetTop function, then do so there.

Define variables in the if scope

In your tests, there are a few instances where you define a variable and then use it only in an if statement. Go allows you to define variables directly in the if statement which are only valid in that

if tempVal, _ := rc.GetVal(keyStr); valStr != tempVal{
 t.Errorf("Key should exist and be equal to %s!", valStr)
}