Improving my solution to the Quiz project from Gophercises in Golang

Question

I'm new to Go and I completed the first challenge from Gophercises. Any feedback would be highly appreciated.

There are some things that I am unsure of if they are made in an optimal way.

Github repo

1. Goroutines and channels in the quizList loop

2. Better error logging for the cli portion

3. The Shuffle function only works for string, how can I make it work for other types as well?

4. How should I approach writing test cases?

5. How should the folder structure be structured?

    package main
    import (
    "encoding/csv"
    "flag"
    "fmt"
    "log"
    "math/rand"
    "os"
    "strings"
    "time"
    )
    type question struct {
    prompt, answer string
    }
    func main() {
    var (
    csvFile = flag.String("csv", "problems.csv", "a csv file in the format of 'question,answer'")
    timeLimit = flag.Int("limit", 3, "the time limit for the quiz in seconds")
    isShuffle = flag.Bool("shuffle", false, "shuffle the quiz order each time it is run")
    )
    flag.Parse()
    if *csvFile == "" {
    fmt.Printf("%s does not exist, please choose another file", *csvFile)
    } 
    if *timeLimit == 0 {
    fmt.Printf("Not a valid limit, please choose another limit")
    }
    score := 0
    file, err := os.Open(*csvFile)
    if err != nil {
    log.Fatal(err)
    }
    r := csv.NewReader(file)
    problems, err := r.ReadAll()
    if err != nil {
    log.Fatal(err)
    }
    if *isShuffle {
    Shuffle(problems)
    }
    quizList := populateQuiz(problems)
    timer := time.NewTimer(time.Duration(*timeLimit) * time.Second)
    isTimerDone := true
    ch := make(chan string)
    for _, quiz := range quizList {
    fmt.Printf(quiz.prompt)
    var recipientAnswer string
    go func() {
    fmt.Scanln(&recipientAnswer)
    ch <-recipientAnswer
    }()
    if isTimerDone {
    select {
    case <-ch:
    if recipientAnswer == quiz.answer {
    score += 1
    }
    case <-timer.C:
    isTimerDone = false
    fmt.Println()
    return
    }
    } else {
    break
    }
    }
    fmt.Printf("\nYou scored %d out of %d.\n", score, len(problems))
    }
    func Shuffle(l [][]string) {
    rand.Seed(time.Now().UnixNano())
    rand.Shuffle(len(l), func(i, j int) {
    l[i], l[j] = l[j], l[i]
    })
    }
    func populateQuiz(problems [][]string) []question {
    var questions []question 
    for index, record := range problems {
    prompt := fmt.Sprintf("Problem: #%d: %s = ", index + 1, record[0])
    answer := record[1]
    questions = append(questions, question{
    prompt: prompt, 
    answer: strings.TrimSpace(answer),
    })
    }
    return questions
    }
   

Answer 1

A well-written program has a well-organized structure.

In Go, interfaces describe behavior, and are a key abstraction. A primary Go abstraction for a stream of data is the io.Reader interface. It allows us to substitute any type that satisfies the io.Reader interface, for example, os.File, bytes.Buffer, bytes.Reader, strings.Reader, bufio.Reader, and so on.

Consider reading the Gophercises Quiz Game problems.csv.

A problem is

type problem struct {
 question, answer string
}

For the io.Reader interface we have

func readProblems(r io.Reader) ([]problem, error) {
 rv := csv.NewReader(r)
 rv.FieldsPerRecord = 2
 rv.ReuseRecord = true
 var problems []problem
 for {
 rec, err := rv.Read()
 if err != nil {
 if err == io.EOF {
 break
 }
 return nil, err
 }
 problems = append(problems,
 problem{
 question: rec[0],
 answer: rec[1],
 },
 )
 }
 return problems[:len(problems):len(problems)], nil
}

For reading a file, we use os.File to satisfy the io.Reader interface.

func loadProblems(filename string) ([]problem, error) {
 f, err := os.Open(filename)
 if err != nil {
 return nil, err
 }
 defer f.Close()
 problems, err := readProblems(f)
 if err != nil {
 return nil, err
 }
 return problems, nil
}

For testing, we use bytes.Reader to satisfy the io.Reader interface.

var readProblemsTests = []struct {
 line string
 want problem
}{
 {
 line: "5+5,10\n",
 want: problem{question: "5+5", answer: "10"},
 },
 // ...
}
func TestReadProblems(t *testing.T) {
 var file []byte
 for _, tt := range readProblemsTests {
 file = append(file, tt.line...)
 }
 r := bytes.NewReader(file)
 got, err := readProblems(r)
 if err != nil {
 t.Errorf("readProblems: got %v, want %v", err, nil)
 }
 // ...
}

The following are sample data, program, and test to illustrate the usage of the readProblems function for the Gophercises Quiz Game.

readcsv.csv:

5+5,10
1+1,2
8+3,11
1+2,3
8+6,14
3+1,4
1+4,5

readcsv.go:

package main
import (
 "encoding/csv"
 "fmt"
 "io"
 "log"
 "math/rand"
 "os"
 "time"
)
type problem struct {
 question, answer string
}
func (p problem) String() string {
 return fmt.Sprintf(
 "{question: %q, answer: %q}",
 p.question, p.answer,
 )
}
func readProblems(r io.Reader) ([]problem, error) {
 rv := csv.NewReader(r)
 rv.FieldsPerRecord = 2
 rv.ReuseRecord = true
 var problems []problem
 for {
 rec, err := rv.Read()
 if err != nil {
 if err == io.EOF {
 break
 }
 return nil, err
 }
 problems = append(problems,
 problem{
 question: rec[0],
 answer: rec[1],
 },
 )
 }
 return problems[:len(problems):len(problems)], nil
}
func loadProblems(filename string) ([]problem, error) {
 f, err := os.Open(filename)
 if err != nil {
 return nil, err
 }
 defer f.Close()
 problems, err := readProblems(f)
 if err != nil {
 return nil, err
 }
 return problems, nil
}
func shuffleProblems(l []problem) {
 rand.Seed(time.Now().UnixNano())
 rand.Shuffle(len(l),
 func(i, j int) {
 l[i], l[j] = l[j], l[i]
 },
 )
}
func printProblems(problems []problem) {
 fmt.Println(len(problems), "problems")
 for _, p := range problems {
 fmt.Println(p.String())
 }
}
func main() {
 // TODO: use flag
 filename := `readcsv.csv`
 problems, err := loadProblems(filename)
 if err != nil {
 log.Fatal(err)
 }
 // TODO: use flag
 shuffle := true
 if shuffle {
 shuffleProblems(problems)
 }
 printProblems(problems)
}

Output:

$ go run readcsv.go
7 problems
{question: "1+4", answer: "5"}
{question: "5+5", answer: "10"}
{question: "8+6", answer: "14"}
{question: "1+1", answer: "2"}
{question: "3+1", answer: "4"}
{question: "1+2", answer: "3"}
{question: "8+3", answer: "11"}
$ 

readcsv_test.go:

package main
import (
 "bytes"
 "testing"
)
var readProblemsTests = []struct {
 line string
 want problem
}{
 {
 line: "5+5,10\n",
 want: problem{question: "5+5", answer: "10"},
 },
 {
 line: "1+1,2\n",
 want: problem{question: "1+1", answer: "2"},
 },
}
func TestReadProblems(t *testing.T) {
 var file []byte
 for _, tt := range readProblemsTests {
 file = append(file, tt.line...)
 }
 r := bytes.NewReader(file)
 got, err := readProblems(r)
 if err != nil {
 t.Errorf("readProblems: got %v, want %v", err, nil)
 }
 if len(got) != len(readProblemsTests) {
 t.Errorf("problems: got %v, want %v", len(got), len(readProblemsTests))
 }
 for i, tt := range readProblemsTests {
 if got[i] != tt.want {
 t.Errorf("problem: got %v, want %v", got[i], tt.want)
 }
 }
}

Output:

$ go test readcsv_test.go readcsv.go -v
=== RUN TestReadProblems
--- PASS: TestReadProblems (0.00s)
PASS
$

---

ADDENDUM

Comment: Regarding the readcsv.go file, in the readProblems function why did you return problems[:len(problems):len(problems)] instead of problems? Also, when closing the file in the loadProblems function, does the defer wait until the loadProblems function has returned or until the main function is done? – Anthony Gedeon

---

The Go Programming Language Specification

Appending to and copying slices

The variadic function append appends zero or more values x to s of type S, which must be a slice type, and returns the resulting slice, also of type S.

If the capacity of s is not large enough to fit the additional values, append allocates a new, sufficiently large underlying array that fits both the existing slice elements and the additional values. Otherwise, append re-uses the underlying array.

Slice expressions

Full slice expressions

For an array, pointer to array, or slice a (but not a string), the primary expression

a[low : high : max]

constructs a slice of the same type, and with the same length and elements as the simple slice expression a[low : high]. Additionally, it controls the resulting slice's capacity by setting it to max - low. Only the first index may be omitted; it defaults to 0.

---

Hyrum's Law

An observation on Software Engineering

Put succinctly, the observation is this:

With a sufficient number of users of an API,
it does not matter what you promise in the contract:
all observable behaviors of your system
will be depended on by somebody. 

Obligatory XKCD: https://xkcd.com/1172/

---

We should avoid exposing a particular API (function or method) implementation. This particular implementation of readProblems uses the append built-in function, which may create excess capacity. Other implementations may return the exact capacity. By Hyrum's Law, we ensure that this implementation, like other implementations, returns the exact capacity:

problems[:len(problems):len(problems)]

---

The Go Programming Language Specification

Defer statements

A "defer" statement invokes a function whose execution is deferred to the moment the surrounding function returns, either because the surrounding function executed a return statement, reached the end of its function body, or because the corresponding goroutine is panicking.

---

It's important to ensure that scarce resources, like file handles, are released as soon as they are no longer needed. The defer statement to close the file in the loadProblems function is deferred to the moment the loadProblems function returns.

• Anthony Gedeon: Regarding the readcsv.go file, in the readProblems function why did you return problems[:len(problems):len(problems)] instead of problems? Also, when closing the file in the loadProblems function, does the defer wait until the loadProblems function has returned or until the main function is done?
• peterSO: @AnthonyGedeon: See the addendum in my revised answer.

Answer 2

from what I saw there are a few changes that could be done to the program

1. while parsing command-line arguments I think it's better to put them inside a function cause it makes the code look clean and simple and as it is returned as a pointer you would have to keep getting the value as * but you could return it as the specific type and have a simpler handling

    csvFile, timeLimit, isShuffle := parseCMD() 
    func parseCMD() (string, int, bool) {
    csvFile := flag.String("csv", "problems.csv", "a csv file in the format of 'question,answer'")
    timeLimit := flag.Int("limit", 3, "the time limit for the quiz in seconds")
    isShuffle := flag.Bool("shuffle", false, "shuffle the quiz order each time it is run")
    flag.Parse()
    return *csvFile, *timeLimit, *isShuffle
   

   }

2. var ( csvFile = flag.String("csv", "problems.csv", "a csv file in the format of 'question,answer'") timeLimit = flag.Int("limit", 3, "the time limit for the quiz in seconds") isShuffle = flag.Bool("shuffle", false, "shuffle the quiz order each time it is run") ) var can be used on a variable that doesn't have any initialization or if it's gonna be a global score or... if in a case outside an if statement or a for loop (a situation where:= won't work).. as these are being declared inside a function and have a default value a short variable declaration would be a more golang kinda way. (there is not a lot of difference between using var and:= in your code but in a situation like this:= would be more golang-ish) https://stackoverflow.com/questions/53404305/when-to-use-var-or-in-go/53404332 https://stackoverflow.com/questions/21657446/var-vs-in-go

3.  timer := time.NewTimer(time.Duration(*timeLimit) * time.Second)
   

   while defining a ticker add a "defer ticker.stop()" to release any resource related to as it wont be garbage collected (https://golang.org/src/time/tick.go)

4. if isTimerDone { select { case <-ch: if recipientAnswer == quiz.answer { score += 1 } case <-timer.C: isTimerDone = false fmt.Println() return } } else { break }

In the above case the if check (isTimerDone) won't actually hit as by flow you always go into the select case and if you get an answer you continue the loop and if the timer hits you return.. so a better way to write it would be to remove the IF-ELSE check..

from what I understand yours is a score counting program so assuming you don't answer and the timer hit.. you could continue the loop and score based on the total answer or break the loop if he doesn't answer (this is just a suggestion :) )