A couple months back I made a go fish game in Go, just for the sake of that pun. It was pretty poorly done and made experienced Go-ers cry when they looked at it. I rewrote some of it to be more standard, but I'd still like critiques from the 95% of the world with more Go experience than me.
For those unaware, Go Fish is a children's game where each player has a hand of cards and takes turns calling out a card value, if anyone else has that card they have to give it to them, your turn ends when noone has a card that you call out, and you have to draw a card from the deck before the next person takes their turn. The game ends when there are no more cards in the deck and the winner is whoever gets the most four-of-a-kinds during the game.
package main
import (
"fmt"
"math/rand"
"sort"
"time"
)
var cards = [13]string{"2", "3", "4", "5", "6", "7", "8", "9",
"10", "J", "Q", "K", "A"}
//GoFishGame Stores the game state.
type GoFishGame struct {
hands [][]string
deck []string
turn int
scores []int
}
// checkForBooks looks for fours of a kind
// and scores them if we have them.
// If we run out of cards, we draw more.
func (gm *GoFishGame) checkForBooks() {
sort.Strings(gm.hands[gm.turn])
prev := ""
count := 1
for _, card := range gm.hands[gm.turn] {
if card == prev {
count++
if count == 4 {
fmt.Printf("Book of %s.\n", card)
gm.stealCards(card, gm.turn)
gm.scores[gm.turn]++
if gm.isHandEmpty() {
gm.drawCard()
}
}
} else {
count = 1
}
prev = card
}
}
// drawCard takes a card from the deck
// adding it to the current player's hand.
func (gm *GoFishGame) drawCard() {
if !gm.isDeckEmpty() {
card := gm.deck[0]
gm.deck = gm.deck[1:]
if gm.turn == 0 {
fmt.Printf("You drew a %s.\n", card)
}
gm.hands[gm.turn] = append(gm.hands[gm.turn], card)
//Check for books
gm.checkForBooks()
}
}
// endPly ends the current person's turn.
// It then either calls the next person's
// turn or prints a game over message.
func (gm *GoFishGame) endPly() {
gameOver := gm.isGameOver()
if gameOver {
gm.printGameOverMessage()
} else if gm.turn == 1 {
gm.playerTurn(getPickComputer)
} else {
gm.playerTurn(getPickUser)
}
}
// getPickComputer handles the computer's card choices.
// We do the moderately smart thing of pick a random
// card from our hand
func getPickComputer(gm *GoFishGame) string {
hand := gm.hands[1]
choice := "A"
if len(hand) > 0 {
choice = hand[rand.Intn(len(hand))]
}
fmt.Printf("Computer picks %s.\n", choice)
return choice
}
// getPickUser gets the user's move.
// If it's not valid, then the user just wastes
// their turn.
func getPickUser(gm *GoFishGame) string {
fmt.Println("What card do you want?")
var card string
fmt.Scanf("%s\n", &card)
return card
}
// isDeckEmpty returns if the deck is empty.
func (gm *GoFishGame) isDeckEmpty() bool {
return len(gm.deck) == 0
}
// isHandEmpty returns if the current player's hand is empty.
func (gm *GoFishGame) isHandEmpty() bool {
return len(gm.hands[gm.turn]) == 0
}
// isGameOver returns if the game is over.
// This happens when all 13 pips have been made into sets.
func (gm *GoFishGame) isGameOver() bool {
return gm.scores[0]+gm.scores[1] == 13
}
// makeDeck makes a deck.
// The deck is 52 cards with 4 of each pip.
func makeDeck() []string {
rand.Seed(time.Now().UTC().UnixNano())
deck := make([]string, 52)
perm := rand.Perm(52)
for indx := range perm {
tVal := perm[indx]
card := cards[tVal/4]
deck[indx] = card
}
return deck
}
// opponentHas returns if the opponent's hand has a card.
func (gm *GoFishGame) opponentHas(find string) bool {
for _, card := range gm.hands[(gm.turn+1)%2] {
if card == find {
return true
}
}
return false
}
// playerTurn handles the major game logic.
// It's used for both the player's and computer's turns,
// with the different behavior handled by the getPick param.
func (gm *GoFishGame) playerTurn(getPick func(*GoFishGame) string) {
opponent := (gm.turn + 1) % 2
gm.checkForBooks()
if opponent == 1 {
gm.printHand()
}
if gm.isHandEmpty() {
gm.drawCard()
}
gameOver := gm.isGameOver()
if !gameOver {
card := getPick(gm)
if gm.opponentHas(card) {
count := gm.stealCards(card, opponent)
for indx := 0; indx < count; indx++ {
gm.hands[gm.turn] = append(gm.hands[gm.turn], card)
}
gm.checkForBooks()
} else {
fmt.Println("GO FISH!")
gm.drawCard()
gm.turn = opponent
}
}
gm.endPly()
}
// printGameOverMessage prints the appropriate end message.
func (gm *GoFishGame) printGameOverMessage() {
fmt.Printf("Final score is %d to %d.\n", gm.scores[0], gm.scores[1])
if gm.scores[0] > gm.scores[1] {
fmt.Println("Player wins!")
} else if gm.scores[0] == gm.scores[1] {
fmt.Println("It's a tie.")
} else {
fmt.Println("Computer wins!")
}
}
// printHand print's the player's hand and current score.
func (gm *GoFishGame) printHand() {
sort.Strings(gm.hands[0])
fmt.Printf("You have: %s.\n", gm.hands[0])
fmt.Printf("Score is %d to %d.\n", gm.scores[0], gm.scores[1])
}
// stealCards removes all instances of a card from side's hand.
func (gm *GoFishGame) stealCards(purge string, side int) int {
count := 0
tList := gm.hands[side]
var filtered []string
for _, card := range tList {
if purge == card {
count++
} else {
filtered = append(filtered, card)
}
}
gm.hands[side] = filtered
return count
}
// main creates the deck and initial hands.
func main() {
deck := makeDeck()
playerHand := deck[0:9]
compHand := deck[9:18]
deck = deck[18:]
hands := make([][]string, 2, 2)
hands[0] = playerHand
hands[1] = compHand
scores := make([]int, 2, 2)
scores[0] = 0
scores[1] = 0
game := GoFishGame{hands, deck, 0, scores}
game.playerTurn(getPickUser)
}
I have three things that I think might be issues. First, I use 0/1 both as flags saying whose turn it is and then use them as array indexes for GoFishGame.hands and GoFishGame.score. Second, this program seems to be done assuming tail call optimization, that's not a thing in Go, but since the stack depth is reasonably bounded it doesn't seem like an issue to me. Third, I'm not sure how to name methods and structs.
1 Answer 1
The following comments come from reading the code for the first time, straight from top to bottom.
Directly after the definition for the GoFishGame type there should be a method NewGoFishGame that initializes this struct, so that the reader learns some basic assumptions of the game, such as the number of players or how many cards each hand holds.
In the moment that gm.checkForBooks calls gm.stealCards, you are iterating over the current player's hand. At the same time, the stealCards method probably removes some cards from the player's hand. Then, iteration continues.
At this point, it is hard to predict what happens. The good thing is that a player can only ever have one book at a time, so for this particular game, there is no chance of missing a second book in the hand. To be safe, you should return from the function at the end of the if count == 4 { ... } clause.
In the drawCard method, the gm.turn == 0 condition surprised me. At this point I concluded that player 0 is a human player and that players 1 to n are computer players. If so, the code is ok.
In the endPly method, the gameOver variable is not necessary. When you step through the code during debugging, you will know the returned value based on the next line the debugger steps to.
In the getPickComputer method I was surprised again. Up to now I thought that this was a game for 2 to 6 players. But since the computer is fixed to player number 1, it is only a 2 player game. That's unfortunate.
In the documentation of getPickComputer, the word we/our is confusing. I had taken the perspective of the human player, so I thought the computer player would spy on my (the human player's) hand. That would have been mean.
In the makeDeck function, don't seed the random number generator, since it will be predictable. Rather, seed it when starting the whole program. Sure, this program is just a children's game, not a crypto library, but who knows what you will develop next.
At the first look, the makeDeck function didn't seem to shuffle the deck, since in the for loop, you are only using a single variable. The loop should be for indx, tVal := range perm { ... }.
In the opponentHas method, you are dealing with two different cards, one is the requested card and the other is the opponent's card. Neither of them should go by the simple name card. I would rename find to requestedCard and then rename card to opponentCard.
In the playerTurn method, the expression (gm.turn + 1) % 2 appears for the second time. You might consider extracting this into its own method, which will become helpful when you expand the game for 6 players later. But even if you don't extract the code, you would have to look at all the code accessing gm.turn, which would also work.
Still in the playerTurn method, the gameOver variable is not necessary, as above.
Still in the playerTurn method, after appending a card to a hand I would expect that hand to be sorted. When you do this, the checkForBooks and printHand methods don't have to do the sorting anymore, and you are guaranteeing that all hands are always sorted.
In the main function, you don't need the variables playerHand and computerHand, although it doesn't hurt. You can initialize the scores using a single line: scores := []int{0, 0}. Same for the hands. All this code should go into the NewGoFishGame function, so that the main function only creates a new game and then calls the method play on it. It sounds weird that the method playerTurn is the whole game; I would have expected it to just execute a single move.
_Handsis definitely non-idomatic, your names are better). Edit: D'oh, now I see that's your old code (that makes Go-ers cry :)). \$\endgroup\$