Gin framework project with an endpoint to return a list of users based on their score

Question

My relevant work experience is with Java (mostly web development with Dropwizard/Spring) so I can not avoid using the same practices as I do in a Java project. The project is there but I am going to copy it here:

Project

A very simple project with Gin framework with an endpoint to return a list of users based on their score.

Structure

liveops-tool
-- user
---- model
------ user.go
---- service
------ userService.go
------ userDirectory.go (mock the db)
-- main.go

main.go

r.GET("/competition/users", func(c *gin.Context) {
        numUsers, err := strconv.Atoi(c.DefaultQuery("numusers", "6"))
        minScore, err := strconv.Atoi(c.DefaultQuery("minscore", "0"))
        maxScore, err := strconv.Atoi(c.DefaultQuery("maxscore", "2000"))
        fmt.Printf("numUsers: %d , minScore: %d , maxScore: %d \n", numUsers, minScore, maxScore)

        if err == nil {
            var res = userService.GenerateUserListByScore(numUsers, minScore, maxScore)
            c.JSON(http.StatusOK, gin.H{"users": res})
        } else {
            c.JSON(http.StatusBadRequest, gin.H{"users": "no valid"})
        }

    })

user.go

package user

type User struct {
    ID         string `json:"id"`
    Name       string `json:"name"`
    Country    string `json:"country"`
    TotalScore int    `json:"totalScore"`
}

userService.go

package user

import (
    user "liveops-tool/user/model"
)

// GenerateUserListByScore returns a list of users for a tournament
func GenerateUserListByScore(numUsers, minScore, maxScore int) []user.User {
    return searchUsers(numUsers, minScore, maxScore)
}

userDirectory.go

package user

import (
    "fmt"
    userModel "liveops-tool/user/model"
)

var users = []userModel.User{}

func init() {
    initUsers()
}

func searchUsers(numUsers, minScore, maxScore int) []userModel.User {
    return filterUsersByScore(numUsers, minScore, maxScore)
}

func filterUsersByScore(numUsers, minScore, maxScore int) []userModel.User {
    var filteredUsers = []userModel.User{}
    var countUsers = 0
    if len(users) < numUsers {
        numUsers = len(users)
    }
    for i := range users {
        if users[i].TotalScore >= minScore && users[i].TotalScore <= maxScore {
            filteredUsers = append(filteredUsers, users[i])
            countUsers++
        }

        if countUsers >= numUsers {
            break
        }
    }
    return filteredUsers
}

func initUsers() {
    user1 := userModel.User{ID: "1", Name: "John", Country: "UK", TotalScore: 500}
    user2 := userModel.User{ID: "2", Name: "Frank", Country: "ES", TotalScore: 1500}
    user3 := userModel.User{ID: "3", Name: "Bob", Country: "UK", TotalScore: 2000}
    user4 := userModel.User{ID: "4", Name: "Anna", Country: "FR", TotalScore: 3000}
    users = append(users, user1)
    users = append(users, user2)
    users = append(users, user3)
    users = append(users, user4)
    fmt.Printf("users: %+v\n", users)
}

Questions

• I am mocking the user list and I would like the method initUsers would not be accesible from userService. Golang doesn't have public and private methods because the visibility depends on the package. The same for the function filterUsersByScore. How can I improve it?

• User (model): I need to put the attributes with the first letter in uppercase to make it exportable when I return the JSON. Is it possible to have the attributes as private to access them through getters and setters and also return them in the JSON response?

Any other advice about the code is well received.

Answer 1

Let's start by addressing your 2 main questions:

1. Visibility (exporting) of functions

Yes, there's a very easy way of doing this, by using receiver functions (methods), and interfaces. Golang interfaces are incredibly powerful if you understand where they actually differ from "traditional" languages with a full-blown OOP model. you could have a directory type, and expose an interface to the service, which only declares the functions you want to use. Suppose you want your service to access GetUsers, but not a FilterByX, then just pass this directory type in as an interface type limiting how you can use it:

// in service
type Directory interface {
    GetUsers() []User
}

type service struct {
    dir Directory // interface type
}

func New(dir Directory) *service {
    return &service{
        dir: dir,
    }
}

The interface is declared in the package that uses it, not in the package implementing said interface. This makes unit-testing a lot easier. There's quite a few articles about this around. It's a bit odd at first, if you're used to OO, but it makes sense quite quickly. Don't try to make go look like C++, C#, or Java... it's its own thing, and though not perfect, it's got a lot of good things going for it.

As for the initUsers: that function is completely pointless. You could move the entire body of that function into your init function from the get-go. But why stop there? You can simply initialise your variable straight up, rather than creating 2 function calls to do the same thing:

var users = []User{
    {
        ID:         "1",
        Name:       "John",
        Country:    "UK",
        TotalScore: 500,
    },
    {
        ID:         "2",
        Name:       "Frank",
        Country:    "ES",
        TotalScore: 1500,
    },
    {
        ID:         "3",
        Name:       "Bob",
        Country:    "UK",
        TotalScore: 2000,
    },
    {
        ID:         "4",
        Name:       "Anna",
        Country:    "FR",
        TotalScore: 3000,
    },
}

That's it, all done.

2. Marshalling unexported fields.

Yes, this is possible, but I'd strongly advise against it. However, here's a couple of ways to do it. In both cases, we're relying on the fact that golang supports a JSON Marshaller and Unmarshaller interface out of the box.

type User struct {
    id      string
    name    string
    country string
    score   int
}

// manually creating a map
func (u User) MarshalJSON() ([]byte, error) {
    data := map[string]interface{}{
        "id":      u.id,
        "name":    u.name,
        "country": u.country,
        "score":   u.score,
    }
    return json.Marshal(data)
}

// now with some pointer magic... mimics the omitempty tag
func (u User) MarshalJSON() ([]byte, error) {
    data := map[string]interface{}{
        "id":      &u.id,
        "name":    &u.name,
        "country": &u.country,
        "score":   &u.score,
    }
    return json.Marshal(data)
}

Now for unmarshalling:

// note a pointer receiver is required here, we're changing the receiver state
func (u *User) UnmarshalJSON(v []byte) error {
    data := map[string]interface{}{} // empty map
    if err := json.Unmarshal(v, &data); err != nil {
        return err
    }
    if id, ok := data["id"]; ok {
        // technically we need to check the type assert, I'll do it once:
        strId, ok := id.(string)
        if !ok {
            return errors.New("id value is not a string!")
        }
        u.id = strId
    }
    if name, ok := data["name"]; ok {
        u.name = name.(string)
    }
    if country, ok := data["country"]; ok {
        u.country = country.(string)
    }
    if score, ok := data["score"]; ok {
        u.score = score.(int)
    }
    return nil
}

As you can probably see now, each field you add will add more code that needs to be written, changing the names of fields is a right PITA... it's better to just not bother IMO.

You could use the pointer trick from the marshaller here to avoid the type assertions, though:

func (u *User) UnmarshalJSON(v []byte) error {
    data := map[string]interface{}{
        "id":      &u.id,
        "name":    &u.name,
        "country": &u.country,
        "score":   &u.score,
    }
    // unmarshals into map[string]<pointer to fields on struct>
    return json.Unmarshal(v, &data)
}

Still, this doesn't look nice to me. I'd just stick with exported fields + json tags. If you really don't want people to access fields in certain places, just use interfaces as explained above.

---

Something you really ought not to do, is your over-use of package aliases. You may, or may not have noticed that I've always used the User type, without a package name. You've got aliases like:

user "liveops-tool/user/model"
// and the even worse
userModel "liveops-tool/user/model"

This is what sometimes is referred to as stuttering names. Read this aloud and ask yourself whether your package name is giving the user of your code any clearer understanding of what the User type represents:

u := user.User{} // u is an instance of User from the user package

What's in the user package? What is this User? I don't know. I'm just annoyed at having to type user twice.

u := userModel.User{} // u us a user from the userModel package

OK, now I know User is probably some kind of data structure. Good, but did I really need to know that I'm getting a User form the userModel package? I'm still annoyed at having to type user twice. The file itself is in the directory called model (perhaps models would be better). That's the bit of information that I want, so why not just:

import (
    "liveops-tool/user/model"
)

func foo() {
    u := model.User{} // u is a user model
}

That's a lot better, isn't it? You may want to have a look at the golang code review comments page. There's a lot of stuff in there that makes your code nicer. It's mainly sensible stuff: keep package aliases to a minimum, keep package names short, but communicative. Avoid name stutter, etc...

A particular bug-bear of mine is when people have a package like handler, and have a New function in there like this:

package handler

func NewHandler() (*h, error) {
    return &h{}, nil
}

Forcing me to write handler.NewHandler(). There's a reason why people find Java overly verbose (SomeFooBarImpl foobar = new SomeFooBarImpl();). What else do you expect handler.New() to return, other than a new handler?

/rant...

Might revisit this one later on, I'm heading home for the day now :P