patterngoMinor
Wrote a package in Go. What did I do wrong? What did I do right?
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.
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:
This is a bit annoying. I'd much rather create a function that constructs a fully initialised
We can also use some syntactic sugar to shorten the actual initialisation:
I don't really understand what the point of the function:
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
In the following function:
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
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.