first Go program: Tic Tac Toe

Question

^{If I formatted my question wrong, feel free to notify me or update, this is my first question on this subforum}

This is literally my first ever Go project, decided to make a Tic Tac Toe (ironically I came up with that idea myself, turns out a lot of people use this as a beginner project heheh), requiring me to use User input and some slice/array/struc.

I would like some feedback :) Not perse the logic (like that there might be a more efficient way to check a match), but more the usage of struct, funcs, pointers, etc. Specific Go related feedback :)

If it matters I'm not new to programming, just expanding my skills from the web (Advanced PHP). I'm not really used to a program that keeps running until we hit an exit somewhere. I'm getting used to the super short var names, I'm not sure whether that's something I want to be more strict in, but time will tell.
I had some difficulty with returning matches in a proper/useful way.

package main

import (
    "bufio"
    "fmt"
    "os"
    "strconv"
    "strings"
)

func getInputAsInt(r *bufio.Reader) int {
    text, _ := r.ReadString('\n')
    text = strings.Replace(text, "\n", "", -1)
    textAsInt, _ := strconv.Atoi(text)

    return textAsInt
}

func main() {
    b := createNewBoard()
    reader := bufio.NewReader(os.Stdin)
    active := 1 // current player, 1=X, 2=O
    for {
        fmt.Print("\n\n")
        b.print()
        if active == 1 {
            fmt.Print("X -> ")
        } else {
            fmt.Print("O  -> ")
        }

        textAsInt := getInputAsInt(reader)
        err := b.updateBoard(textAsInt, active)
        if err != nil {
            fmt.Println(err)
        } else {
            msg := hasAWinner(b)
            if msg != "" {
                b.print()
                fmt.Println(msg)
                os.Exit(0)
            }

            if active == 1 {
                active = 2
            } else {
                active = 1
            }
        }
    }
}

package main

import (
    "errors"
    "fmt"
    "strings"
)

type board [3][3]int

func createNewBoard() board {
    return board{}
}

// Convert the data to a human format
func (b board) print() {
    var boardRows []string
    for _, row := range b {
        // convert slice of ints to a joined string
        boardRows = append(boardRows, strings.Trim(strings.Join(strings.Fields(fmt.Sprint(row)), "|"), "[]"))
    }
    textBoard := strings.Join(boardRows, "\n---┼---┼---\n")
    textBoard = strings.Replace(textBoard, "0", " · ", -1)
    textBoard = strings.Replace(textBoard, "1", " X ", -1)
    textBoard = strings.Replace(textBoard, "2", " O ", -1)

    fmt.Println(textBoard)
}

// Takes the input and tries to update the board
func (b *board) updateBoard(pos int, char int) error {
    keymap := [9][2]int{
        {2, 0}, // 1
        {2, 1}, // 2
        {2, 2}, // 3
        {1, 0}, // 4
        {1, 1}, // 5
        {1, 2}, // 6
        {0, 0}, // 7
        {0, 1}, // 8
        {0, 2}, // 9
    }
    mp := keymap[pos-1] // Mapped Pos
    square := b[mp[0]][mp[1]]
    if square != 0 {
        return errors.New("this square is already chosen dumdum, try again")
    }

    b[mp[0]][mp[1]] = char

    return nil
}

package main

import "fmt"

// Tests whether there is a possible win
func hasAWinner(b board) string {
    hasMatch, winner, pos := hasAnyHorizontalMatch(b)
    if hasMatch {
        return fmt.Sprintf("%v wins! (horizontal row %d)", winner, pos)
    }

    hasMatch, winner, pos = hasAnyVerticalMatch(b)
    if hasMatch {
        return fmt.Sprintf("%v wins! (vertical row %d)", winner, pos)
    }

    hasMatch, winner = hasAnyDiagonalMatch(b)
    if hasMatch {
        return fmt.Sprintf(" wins! (diagonal), winner")
    }

    return ""
}

func hasAnyHorizontalMatch(b board) (bool, int, int) {
    for index, row := range b {
        if hasHorizontalMatch(row, 1) {
            return true, 1, index + 1
        }
        if hasHorizontalMatch(row, 2) {
            return true, 2, index + 1
        }
    }
    return false, -1, -1
}

func hasAnyVerticalMatch(b board) (bool, int, int) {
    i := 0
    for i < len(b[0]) {
        if hasVerticalMatch(b, i, 1) {
            return true, 1, i + 1
        }
        if hasVerticalMatch(b, i, 2) {
            return true, 2, i + 1
        }
        i++
    }

    return false, -1, -1
}
func hasAnyDiagonalMatch(b board) (bool, int) {
    if hasDiagonalMatch(b, 1) {
        return true, 1
    }
    if hasDiagonalMatch(b, 2) {
        return true, 2
    }

    return false, -1
}

func hasHorizontalMatch(line [3]int, char int) bool {
    return line[0] == char && line[1] == char && line[2] == char
}
func hasVerticalMatch(b board, vline int, char int) bool {
    return b[0][vline] == char && b[1][vline] == char && b[2][vline] == char
}
func hasDiagonalMatch(b board, char int) bool {
    return (b[0][0] == char && b[1][1] == char && b[2][2] == char) || (b[0][2] == char && b[1][1] == char && b[2][0] == char)
}

Questions:

• I have char int in several places, short for the character, the currently active player. I wanted to make that more obvious by making a type player int which enables me to use func(p player) instead. Shorter var, more obvious code. However, then the board starts erroring because it is a grid of INT, not of the player. Is there something nice about that? I'm running into the type-strictness here. I could make it a grid of player, but Im not sure how to create a new player with value 1 or 2.

Answer 1

Update one:

Had a brainfart, realized that I dont need to check everything if it matches X or O, just the last move. If the last move has been done by X, O will never win, so checking for that is a waste of compute as it reduces checks by 50%. The new version has functions like this, reducing the complexity regarding returns. I no longer need to return which player won:

func hasAnyHorizontalMatch(b board, player int) (bool, int) {
    for index, row := range b {
        if hasHorizontalMatch(row, player) {
            return true, index + 1
        }
    }
    return false, -1
}

There is another improvement possible here: I dont need to check the whole board everytime, only the rows affected. If someone places an X top left, I only need to check line 0 vertical and horizontal. And I need to check wether it is one of the two diagonal. I'm not going to do this, as that is more of a code thing than a Go thing.

Answer 2

Right, there's a lot to get through, so I'll most likely update this answer over time (I'm writing some review stuff in between work). As per usual, I'll review the code sequentially, and leave comments/suggestions along the way...

Starting with the getInputAsInt function. In Golang, names tend to be as concise as possible. Something like getInputAsInt is a bad name. The AsInt bit doesn't add any relevant information: the return type of the function alone communicates in what format you'll get something, so the function name merely tells you it's getting input. Great! But input for what? What does this input refer to? It's clear that it's the field the player wants to write to, but as your code-base grows, this function name becomes rather generic and unclear.

Getting user input is something I'd assume can go wrong (and it can). Depending on the user input, or the error, I might want to handle the error differently (e.g. I want my program to retry N times, before finally terminating). At the very least, I'd change this function to handle empty input or invalid input and provide the user with a meaningful message (and I'd rename the function):

func getCoordinate(r *bufio.Reader) int {
    in, err := r.ReadString('\n')
    if err != nil {
        fmt.Printf("Could not read input (%v). Please enter a value between 1 and 9", err)
        return getCoordinate(r) // recursive call for retry
    }
    if len(in) == 0 {
        fmt.Println("Empty input, should be value between 1-9")
        return getCoordinate(r) // recursive again
    }
    coord, err := strconv.Atoi(in)
    if err != nil {
        fmt.Printf("Invalid input %s, could not convert to int (%v), please enter value between 1-9\n", in, err)
        return getCoordinate(r)
    }
    if coord < 1 || coord > 9 {
       fmt.Printf("%d is out of the valid/expected range (1-9)\n", coord)
       return getCoordinate(r)
    }
    // all good
    return coord
}

Now the user is given infinite retries to input a valid number. This is a lot better, but it does make the function a lot more specific. A good compromise, is to write a function that can limit the number of retries, and return raw input, which can then be called by (or passed to) a function that converts the raw input to a desired type/value with additional validation and specific error handling:

var (
    ErrEmptyInput = errors.New("user input empty")
)

func readInput(r *bufio.Reader, retries int) (string, error) {
    in, err := r.ReadString('\n')
    if err != nil {
        fmt.Printf("Could not read input: %v", err)
        retries--
        if retries == 0 {
            fmt.Println(" no retries left\n")
            os.Exit(1) // exit with error code
        }
        fmt.Printf(". %d retries left", retries)
        return readInput(r, retries)
    }
    if len(in) == 0 {
        return in, ErrEmptyInput // the caller can determine whether this input is valid or not
    }
    return in, nil // non-empty input
}

Now we have a function that can be used to read any type of single-line user input. Because we're returning a specific error value if the input is empty (we could trim spaces before checking len(in) here), the function can be used in cases where we require a non-empty input or a default value (empty) is acceptable. It's up to the caller to handle that error. To get an int from this, you need a function like this:

func getCoordinate(r *bufio.Reader, retries, min, max int) (int, error) {
    in, err := getInput(r, retries)
    if err != nil {
        if err == ErrEmptyInput && min == 0 { // accept empty as 0
            return min, nil
        }
        // other errors
        return min-1, err // pass on error, return value out of range
    }
    for retries >= 0 {
        i, err := strconv.Atoi(in)
        if err == nil && i >= min && i <= max {
            return i, nil // all good
        }
        retries--
        fmt.Printf("Conversion to int error (%v), or value %d out of range [%d-%d]\n", err, i, min, max)
    }
    return min-1, ErrEmptyInput // or specific error indicating invalid int input
}

Now this is a lot more specific, adds the ability to specify a number of retries, some basic validation, etc... Still, this function is still quite rigid. Depending on how many different types of input you expect to be handling, and how often, you could go all-out with some variadic arguments and callbacks:

type IntValidation func(int) error

func ValidIntRange(min, max int) IntValidation {
    return func (i int) error {
        if i < min || i > max {
            return fmt.Errorf("%d out of valid range [%d-%d]", i, min, max)
        }
        return nil
    }
}

func ValidPositive(zeroValid bool) IntValidation {
    return func (i int) error {
        if i > 0 || (zeroValid && i == 0) {
            return nil
        }
        fmt.Errorf("value not positive")
    }
}

func getNumInput(r *bufio.Reader, validators ...IntValidation) (int, error) {
    // get input same as before
    i, err := strconv.Atoi(in)
    // handle error
    for _, v := range validators {
        if err := v(i); err != nil {
            return i, err
        }
    }
    return i, nil
}

---

Alright, now the main question you had: char int. I would definitely change this to a more descriptive type:

type Player int

Not only would I do this, because you're relying on strings.Replace to replace the player numeric values to their respective X or O characters, I'd just implement the fmt.Stringer interface on this type. The fmt package, when passing any type that implements this interface will just call String() to convert the type to string before printing.

The interface is defined as

type Stringer interface {
    String() string
}

So in our case, that'd just be:

type Player int

const (
    Player0 Player = iota // player 0 is an available square
    Player1
    Player2
)

func (p Player) String() string {
    switch p {
    case Player0:
        return " . "
    case Player1:
        return " X "
    case Player2:
        return " O "
    }
    return fmt.Sprintf("%d", int(p)) // this shouldn't happen
}

Now your board can be defined as:

type board [3][3]Player

And printing each row becomes rather trivial:

func (b board) Print() {
    fmt.Println("|---|---|---|")
    for _, row := range board {
        fmt.Printf("|%s|%s|%s|\n", row[0], row[1], row[2])
    }
    fmt.Println("|---|---|---|")
}

I've created this demo on the golang playground.

Adding another method on the Player type can clean up some of the other stuff in your main function:

func (p Player) Other() Player {
    if p == Player1 {
        return Player2
    }
    return Player1 // if Player0 || Player2, so we don't have to initialise
}

This allows us to clean your main for loop:

var active Player
for {
    fmt.Println("\n")
    b.print()
    active.Switch() //  switch player if active was not initialised, this will move to player 1, after that it'll switch between P2 and P1
    fmt.Printf("%s-> ", active)

    coord := getCoordinate(reader) // infinite retry version
    if err := b.updateBoard(coord, active); err != nil {
        fmt.Println(err)
        os.Exit(1) // something went wrong, the state of board (and thus the game/app is no longer reliable, exit)
    }
    if msg := hasAWinner(b); len(msg) != 0 {
        b.print()
        fmt.Println(msg)
        break // instead of os.Exit, in case the main func has to clean up some stuff
    }
}

Functionally, nothing has changed, we've just managed to do the same thing with a lot less code and we've been able to remove the second if-else for the active variable