Wrote a package in Go. What did I do wrong? What did I do right?

Question

I wrote a Player package in Go. It builds correctly, but I would love a review on what I did and did not do "the Go way". I'm new to coding in the language, although I've gotten about halfway through Ivo Balbaert's "The Way To Go".

The code consists of a struct and a bunch of setters and getters for the struct.

package Player

import (
    "math/rand"
    "time"
)

type Player struct {
    name   string
    race   string
    id     int64
    pos    Position
    facing int64
    health int64
    wpn_id int64   // Weapon id
    is_buf bool    // Has buff
    buf_mt float64 // Buff multiplier
}

type Position map[int64]int64

func (P *Player) SetPlayer(name string, race string) *Player {
    rand := rand.New(rand.NewSource(time.Now().UnixNano()))

    P.name = name
    P.race = race
    P.id = rand.Int63()
    P.pos = Position{0: 0}
    P.facing = 0
    P.health = 100
    P.wpn_id = 0
    P.is_buf = false
    P.buf_mt = 0.0

    return P
}

func (P *Player) GetPlayer() *Player {
    return P
}

func (P *Player) SetPosition(x int64, y int64) {
    P.pos = Position{x: y}
}

func (P *Player) GetPosition() Position {
    return P.pos
}

func (P *Player) SetFacing(direction int64) {
    P.facing = direction
}

func (P *Player) GetFacing() int64 {
    return P.facing
}

func (P *Player) SetHealth(amount int64, direction string) {
    if direction == "down" {
        P.health = P.health - amount
    } else {
        P.health = P.health + amount
    }
}

func (P *Player) GetHealth() int64 {
    return P.health
}

func (P *Player) SetWeaponId(weapon_id int64) {
    P.wpn_id = weapon_id
}

func (P *Player) GetWeaponId() int64 {
    return P.wpn_id
}

func (P *Player) SetIsBuffed(is_buff bool) {
    P.is_buf = is_buff
}

func (P *Player) GetIsBuffed() bool {
    return P.is_buf
}

func (P *Player) SetBuffMultiplier(multiplier float64) {
    P.buf_mt = multiplier
}

func (P *Player) GetBuffMultiplier() float64 {
    return P.buf_mt
}

Answer 1

This is fairly bare-bones as far as packages go, but there are thing that can be improved here.

Firstly, to create a player object, someone is going to need to do the equivalent of:

player := new(Player)
player.SetPlayer(...)

This is a bit annoying. I'd much rather create a function that constructs a fully initialised Player:

func NewPlayer(name string, race string) *Player {
    ...
}

We can also use some syntactic sugar to shorten the actual initialisation:

func NewPlayer(name_ string, race_ string) *Player {
    rand := rand.New(rand.NewSource(time.Now().UnixNano()))
    return &Player{name: name_, race: race_, id: rand.Int63(), pos: Position{0: 0}, 
                   facing: 0, health: 100, wpn_id: 0, is_buf: false, buf_mt: 0.0}
}

I don't really understand what the point of the function:

func (P *Player) GetPlayer() *Player {
    return P
}

is supposed to be. It takes a player, and returns the same player - a no-op.

I'm also not sure why you use a map for position, when 2 integers will do:

type Position struct {
    x int64
    y int64
}

In the following function:

func (P *Player) SetHealth(amount int64, direction string) {
    if direction == "down" {
        P.health = P.health - amount
    } else {
        P.health = P.health + amount
    }
}

direction could be an enum-like type:

type Direction uint8

const (
    DOWN Direction = iota,
    UP   Direction = iota
)

func (P *Player) SetHealth(amount int64, direction Direction) {
    switch direction {
    case DOWN: P.health -= amount
    case UP: P.health += amount
    default: panic("Unknown direction type!")
    }
}

Although this is overkill here, and I'd go with @WinstonEwert's suggestion and just pass in either a positive or negative number that can be used to modify the player health.

A type with a lot of getters and setters doesn't really fit with the Go "mantra". In fact, I'd consider this a bit of an anti-pattern in most languages - if you're going to write both Get and Set functions, without really checking arguments or enforcing constraints, you may as well just give direct access to the underlying variable. This is debatable, however, and I know some people will disagree, so I'll won't belabour the point.

Answer 2

func (P *Player) SetPlayer(name string, race string) *Player {
    rand := rand.New(rand.NewSource(time.Now().UnixNano()))

You should do this once at the beginning your program, and pass it around to where you need the random number generator. For almost all applications, you should only seed the random number generator once, not each time you use it.

Answer 3

Switch

package Player

to

package player

Effective Go's recommendations are: "By convention, packages are given lower case, single-word names; there should be no need for underscores or mixedCaps". http://golang.org/doc/effective_go.html#package-names

Switch

func NewPlayer(name string, race string) *Player 

to

func New(name string, race string) *Player 

Any code referencing your package already will be using player. in front, so having player.New(...) is a little tighter than player.NewPlayer(...). If your package had several constructors it would make sense to keep it NewPlayer, but for a single purpose package, switching to New is preferred. http://golang.org/doc/effective_go.html#package-names