trjhtr

I just started golang development about two weeks ago and recently finished the recommended introduction book.

I'm now working my way through Gophercises - a sort of collection of exercises to improve beginner's understanding of Golang through small projects.

This is my solution to the first project: writing a quiz (cli-)application.

The requirements are simple:

1. 
   

   Read a csv file, each line consisting of a question and an answer:

   
   
   

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

   
   



2. Print the question to the user




3. Validate if the supplied answer is correct.




4. Print the correct answers.

Here is my solution to the problem:

package main

import (
 "bufio"
 "encoding/csv"
 "flag"
 "fmt"
 "io"
 "log"
 "os"
)

type q struct 
 question, answer string


func (q q) ask() bool 
 fmt.Println(q.question, " equals: ")
 scanner := bufio.NewScanner(os.Stdin)
 scanner.Scan()
 if scanner.Err() != nil 
 log.Fatal(scanner.Err())
 
 if scanner.Text() == q.answer 
 return true
 
 return false


func quizLoop(path string, verbose bool) 
 // Loop should:
 // 1. Read records line by line
 // 2. Ask the question (i/o)
 // 3. Keep score.
 file, err := os.Open(path)
 correct, lines := 0, 0

 if err != nil 
 log.Fatal(err)
 
 defer file.Close()

 reader := csv.NewReader(file)
 for 
 record, err := reader.Read()
 if err != nil 
 if err == io.EOF 
 break
 
 log.Fatal(err)
 
 q := qquestion: record[0], answer: record[1]
 if q.ask() 
 if verbose 
 fmt.Println("Correct")
 
 correct++
 else if verbose 
 fmt.Println("Incorrect")
 
 lines++
 
 fmt.Printf("You had %d/%d correct answers!n", correct, lines)


func main() 
 // Setup flags.
 p := flag.String("path", "problems.csv", "Specify the path to the quiz questions.")
 v := flag.Bool("verbose", false, "A boolean value to check if you want the program to be verbose or not.")
 flag.Parse()

 // Invoke loop.
 quizLoop(*p, *v)

As mentioned in my introduction I am rather new to the language, and couldn't see any caveats where it could have been beneficial to use things like interfaces or go routines in this particular project.

These are the things I'm the most interested in having reviewed:

• Best Practices


• Refactoring


• How to use more advanced functionality to solve it (ie. go routines or interfaces)


• Adding unit tests to it. What can be tested?


• Overall design

Two things I noticed:

 file, err := os.Open(path)
 correct, lines := 0, 0

 if err != nil 
 log.Fatal(err)
 
 defer file.Close()

1. 
   

   Handle errors immediately, don't do other things first:

   
   
   

   file, err := os.Open(path)
   if err != nil 
    log.Fatal(err)
   
   defer file.Close()
   
   correct, lines := 0, 0
   

   
   
   

   Don't leave an opportunity to add more code later before the error handling by separating os.Open from if err != nil. Sooner or later you'll use file where it may be nil.

   
   


2. 
   

   Creating a new scanner for each answer is slightly wasteful. Since you asked about potential tests, you can clearly test one or two question/answer cycles. To do that, you'll want to pass two io.Readers to quizLoop instead of a filename

   
   
   

   func quizLoop(questions io.Reader, answers io.Reader, verbose bool)
   

   
   
   

   You can pass the file and os.Stdin from main, and in tests you can pass bytes.Buffers, for instance, to supply test data.

   
   

I don't see an opportunity to leverage Go routines here. This program is inherently sequential. I'm sure you'll get an excercise for that later.

If you really want to use goroutines and channels, you could read the csv file in a goroutine:

questions := make(chan q)
go pushQuestions(questions) // type: func(chan<- questions)
 // it closes the channel when all questions are red

for q := range questions 
 q.ask()
 ...

Design

The q structure has two roles:

• Container of a question-answer pair


• Handle user interaction

It would be better to separate these responsibilities:
the structure should not have the ask() function.

Somewhat related to this,
there's no good reason to recreate a scanner in the ask function.

One option could be to create a new ask(...) function that takes as parameters a scanner, a question, and an answer.
With this approach the q struct becomes pointless.

To make the q struct legitimate,
you could write a func qreader(file *os.File, qs chan q) that reads the CSV and pushes q instances to the channel.
This qreader could run in a goroutine,
while the main thread reads from the channel and handles user interaction.

Don't ignore return values

The program ignores the return value of scanner.Scan().
It could be useful to save an unnecessary check for scanner.Err().

It's good to develop the habit to look suspiciously at statements that don't return value (must be mutating state), or non-void statements whose return value is ignored.

Naming

Go encourages short names, but I think q is too short and meaningless for a struct. At the minimum qa would have been better, to capture the notion of a question-answer pair. I would call it questionAnswer.