HiveBrain v1.2.0
Get Started
← Back to all entries
patterngoMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
packagewhatdidwrotewrongright

Problem

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
}

Solution

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.

Code Snippets

player := new(Player)
player.SetPlayer(...)
func NewPlayer(name string, race string) *Player {
    ...
}
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}
}
func (P *Player) GetPlayer() *Player {
    return P
}
type Position struct {
    x int64
    y int64
}

Context

StackExchange Code Review Q#48012, answer score: 4

Revisions (0)

No revisions yet.