7
\$\begingroup\$

This is my first Go program. I would like to know what could be improved, what is done wrong, and anything else that I should know. The CSV contains 10 questions with the 10 answers separated by a comma. For example:

10+10,20
4+5,32

If you want the full context this is from gophercises which are some exercises to practice Go.

package main

import (
    "encoding/csv"
    "flag"
    "fmt"
    "math/rand"
    "os"
    "strings"
    "time"
)


var points int

func main() {
    fPtr := flag.String("csv", "problems.csv", "FileName in csv (question, answer)")
    tPtr := flag.Int("time", 10, "Time in seconds")
    sPtr := flag.Bool("shuffle", true, "shuffle your questions order")
    flag.Parse()
    questions := readCSV(*fPtr)

    fmt.Print("Press enter to start you have ", *tPtr, " seconds")
    fmt.Scanln()

    askQuestions(&questions, *tPtr, *sPtr)

    fmt.Println("You got ", points, " points")
}

func askQuestions(questions *[][]string, time int, s bool) {

    go startTimer(time)
    for _, i := range shuffle(s, len(*questions)) {
        fmt.Println("Question ", (*questions)[i][0], ":")
        var a string
        fmt.Scan(&a)
        a = strings.ToLower(strings.Trim(a, " "))
        if a == (*questions)[i][1] {
            points++
        }
    }
}

func shuffle(shuffle bool, sliceLen int) []int {
    var r []int
    for i := 0; i < sliceLen; i++ {
        r = append(r, i)
    }
    if shuffle {
        rand.Seed(time.Now().Unix())
        rand.Shuffle(len(r), func(i, j int) {
            r[i], r[j] = r[j], r[i]
        })
    }

    return r
}

func readCSV(s string) (questions [][]string) {
    f, err1 := os.Open(s)
    records, err2 := csv.NewReader(f).ReadAll()
    for err1 != nil || err2 != nil {
        fmt.Println("Error: ", err1, err2)
        fmt.Println("Please re-enter the name of the CSV file: ")
        fmt.Scan(&s)
        f, err1 = os.Open(s)
        records, err2 = csv.NewReader(f).ReadAll()
    }
    return records
}

func startTimer(seconds int) {
    time.Sleep(time.Duration(seconds) * time.Second)
    fmt.Println("Time is up! You got ", points, " points")
    os.Exit(0)
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for sharing the exercises resource! Looks nice and useful \$\endgroup\$
    – user97059
    Apr 21, 2019 at 9:03

1 Answer 1

5
\$\begingroup\$

startTimer()

The time package provides timers natively. So rather than using a call to time.Sleep(), you can use time.NewTimer().

Use fmt.Printf() rather than fmt.Println() for printing formatted strings.

func startTimer(seconds int) {
    time.Sleep(time.Duration(seconds) * time.Second)
    fmt.Println("Time is up! You got ", points, " points")
    os.Exit(0)
}

Becomes:

func startTimer(sec int) {
    timer := time.NewTimer(time.Duration(sec) * time.Second)
    <-timer.C
    fmt.Printf("Time is up! You got %d points\n", points)
    os.Exit(0)
}

readCSV()

In readCSV(), you have multiple error variables. You can instead use a single error variable as you go along.

I advise against adding file retry logic. It complicates the code, and most users can simply re-run the command upon error. Instead, you should return an error value.

Using a named return (questions) means you should actually use the questions variable. Because I switch to multiple return values, I removed the named parameter.

Also be sure to close the file after you're done reading from it.

func readCSV(s string) (questions [][]string) {
    f, err1 := os.Open(s)
    records, err2 := csv.NewReader(f).ReadAll()
    for err1 != nil || err2 != nil {
        fmt.Println("Error: ", err1, err2)
        fmt.Println("Please re-enter the name of the CSV file: ")
        fmt.Scan(&s)
        f, err1 = os.Open(s)
        records, err2 = csv.NewReader(f).ReadAll()
    }
    return records
}

Becomes:

func readCSV(name string) ([][]string, error) {
    f, err := os.Open(name)

    if err != nil {
        return nil, err
    }

    qs, err := csv.NewReader(f).ReadAll()

    if err != nil {
        return nil, err
    }

    return qs, f.Close()
}

askQuestions()

Again, here you should use fmt.Printf() to print formatted strings. You also use the value i as an index, but with the range keyword you can access the currently indexed value, see here.

You should check the return of fmt.Scan().

Rather than starting the timer within askQuestions(), I opted to move it to main(). This means we can remove the time argument.

main()

Naming variables like fPtr and sPtr is just extra typing. Documenting the type in the name itself is not very useful.

  • fPtr becomes name
  • tPtr becomes time
  • sPtr becomes shuffle

From your updated code with shuffling:

Rather than keeping a second copy of the shuffled questions, just modify the array in-place. You also don't need to seed rand (see this example from the Go docs).

Conclusion

Here is the code I ended up with:

package main

import (
    "encoding/csv"
    "flag"
    "fmt"
    "log"
    "math/rand"
    "os"
    "time"
)

var points int

func main() {
    name := flag.String("csv", "problems.csv",
        "filename in csv (question, answer)")
    time := flag.Int("time", 10, "time in seconds")
    shuffle := flag.Bool("shuffle", true, "shuffle your question order")

    flag.Parse()

    questions, err := readCSV(*name)

    if err != nil {
        log.Fatal(err)
    }

    if *shuffle {
        rand.Shuffle(len(questions), func(i, j int) {
            questions[i], questions[j] = questions[j], questions[i]
        })
    }

    go startTimer(*time)

    if err := askQuestions(&questions); err != nil {
        log.Fatal(err)
    }

    fmt.Printf("You got %d points\n", points)
}

func readCSV(name string) ([][]string, error) {
    f, err := os.Open(name)

    if err != nil {
        return nil, err
    }

    qs, err := csv.NewReader(f).ReadAll()

    if err != nil {
        return nil, err
    }

    return qs, f.Close()
}

func askQuestions(questions *[][]string) error {
    for i, q := range *questions {
        fmt.Printf("Question %d: %s\n", i+1, q[0])

        var a string
        _, err := fmt.Scan(&a)

        if err != nil {
            return err
        }

        if a == q[1] {
            points++
        }
    }

    return nil
}

func startTimer(sec int) {
    timer := time.NewTimer(time.Duration(sec) * time.Second)
    <-timer.C
    fmt.Printf("\nTime is up! You got %d points\n", points)
    os.Exit(0)
}

Hope this helps!

\$\endgroup\$
3
  • \$\begingroup\$ Added some notes about your updated shuffling code. \$\endgroup\$
    – esote
    Mar 26, 2019 at 18:29
  • \$\begingroup\$ Thanks for your help, will make the changes. for the range loop in askQuestions I chose i not to make a copy, wouldn't it be faster? Also do you know any go exercise to practice with go concurrency routines channels? \$\endgroup\$
    – lucarlig
    Mar 26, 2019 at 18:58
  • 1
    \$\begingroup\$ @lucarlig The performance difference will be negligible, so I would prefer the most readable code. One resource is the Go tour of concurrency. You can read and page through with live code examples. \$\endgroup\$
    – esote
    Mar 26, 2019 at 19:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.