<p>I've posted a solution to a Gophercise yesterday, but even with nearly 30 views and 3 upvotes no one has reviewed it on stackexchange.</p>
<p><a href="https://codereview.stackexchange.com/questions/186067/beginner-solution-to-basic-quiz-exercise-a-la-gophercises" rel="nofollow">https://codereview.stackexchange.com/questions/186067/beginner-solution-to-basic-quiz-exercise-a-la-gophercises</a></p>
<p>Neither in the gopher slack channel...</p>
<p>Any suggestions?</p>
<hr/>**评论：**<br/><br/>dlsniper: <pre><p>FYI, during the weekend the Slack channel usually is 95% less active than during the week</p></pre>qu33ksilver: <pre><p>Your code might be easier to review if you can upload to a github repo.</p>
<p>Also, adding unit tests are always an added bonus. :)</p></pre>echophant: <pre><p>I don't know much about the Slack Channel, but I haven't really seen StackExchange used as a place to request code reviews. I've seen people ask for code reviews here on the <a href="/r/golang" rel="nofollow">/r/golang</a> subreddit, so this seems like a good place. I reviewed your code, (and <a href="https://gist.github.com/anonymous/1d4e67db9b91394b7e04498cb74e48ca" rel="nofollow">refactored it here</a>), my thoughts:</p>
<ul>
<li>The main should have as little logic as possible, mostly just set up and clean up. I moved all of the quiz functionality into a separate quiz package. The main just handles flags, loading files, and checking the results of the quiz.</li>
<li>Dependencies should be made explicit whenever possible. I added a <code>FromCSV</code> function that takes an <code>io.Reader</code>, so that it's clear we're loading a CSV, and it can come from a file, or the body of an HTTP response, and the <code>quiz</code> package doesn't need to know. I also added <code>Input</code> and <code>Output</code> fields to the <code>Quiz</code>, so that you could write to places other than a terminal if you wanted to. This way, if you wanted to test it, you could make the input with <code>strings.NewReader</code> and the output could be stored in a <code>bytes.Buffer</code></li>
<li>We don't have to <code>Read()</code> the csv records ourselves, we can just <code>ReadAll()</code> at once</li>
</ul></pre>Kimput: <pre><p>Thanks! I really appreciate that you took time out of your day / evening to help a beginner out! :)</p>
<p>I agree with most of your points. </p>
<p>But your last, about ReadAll surprise me. I'm not saying yoi are wrong, but when I was looking into how to read csv files it seemed as if reading line for line is considered more efficient and safer. <a href="https://stackoverflow.com/questions/32027590/efficient-read-and-write-csv-in-go" rel="nofollow">See this SO post for instance</a></p>
<p>Would you mind specifying why you think I should use ReadAll? Is it because of the file size, or any other particular reason?</p>
<p>Do you have any suggestion regarding how I could use go routines or interfaces for a better design? Also, suggestions on unit tests?</p></pre>echophant: <pre><p>The only reason I'm suggesting <code>ReadAll</code> is that it's less code you have to write. Since this is for a quiz program, I'm not expecting the CSV to contain thousands or tens of thousands of rows, which might be the scale at which it's worth it to stream the records from the reader instead of loading them all at once. If you think the quiz files will be megabytes in size, definitely use <code>Read</code> and stream records individually.</p>
<p>As for using goroutines and interfaces, they aren't really necessary for a project like this. There aren't a lot of opportunities for concurrency, unless you needed to do some heavyweight (file or network I/O) operations for each row in the CSV to process it, so you don't really need goroutines. If the <code>quiz</code> package were part of a larger system, you might want to create a <code>Quizzer</code> or an <code>Asker</code> interface, depending on the requirements. For this program though, we don't need that extra layer of abstraction.</p>
<p>Things I would unit test: </p>
<ul>
<li><code>FromCSV</code> - Use <code>strings.NewReader</code> to create a reader with an in-memory CSV file, confirm that processing occurs as expected and the <code>Quiz</code> contains the right number of questions</li>
<li><code>Run</code> - Make sure the output and results are correct.</li>
</ul>
<p>I wrote <a href="https://gist.github.com/anonymous/a3b6cdc71fcc0b78882005960de6b42d" rel="nofollow">some quick tests here</a>, but I had to change the way scanning works to get it to work correctly in the test, so these tests will fail with the above code.</p></pre>nstratos: <pre><blockquote>
<p>Where can I find code reviews for GoLang code?</p>
</blockquote>
<p>Beyond the ones you tried, you can also try to ask for code reviews at:</p>
<ul>
<li><a href="https://groups.google.com/forum/#!forum/golang-nuts" rel="nofollow">golang-nuts</a></li>
<li><a href="https://forum.golangbridge.org" rel="nofollow">Gobridge forum</a></li>
</ul>
<p>According to the requirements:</p>
<blockquote>
<ol>
<li>Read a csv file, each line consisting of a question and an answer:</li>
<li>Print the question to the user</li>
<li>Validate if the supplied answer is correct.</li>
<li>Print the correct answers.</li>
</ol>
</blockquote>
<p>It doesn't seem like you are asked to handle levels of verbosity. Sometimes it is better to follow the requirements strictly so that you have to write less code. The verbosity feature complicates the code a tiny bit:</p>
<blockquote>
<pre><code> if q.ask() {
if verbose { // extra if
fmt.Println("Correct")
}
correct++
} else if verbose { // extra else if
fmt.Println("Incorrect")
}
</code></pre>
</blockquote>
<p>This is okay for such a small program but on a complex application, you might end up making your life harder by adding features you have not been asked to.</p>
<p>To address some of your questions: </p>
<blockquote>
<ul>
<li>Best Practices</li>
<li>How to use more advanced functionality to solve it (ie. go routines or interfaces)</li>
</ul>
</blockquote>
<p>You shouldn't be concerned so much about using goroutines everywhere. As the Go proverb says, <a href="https://www.youtube.com/watch?v=PAAkCSZUG1c&t=14m35s" rel="nofollow">Clear is better than clever</a>. You should be thinking about writing simple, clear code and in this case you've already done that. Sometimes there's no reason to add additional complexity. Nevertheless using interfaces can make your code more general and testable which is useful even on small programs.</p>
<blockquote>
<ul>
<li>Refactoring</li>
<li>Overall design</li>
<li>Adding unit tests to it. What can be tested?</li>
</ul>
</blockquote>
<p>I have purposely kept these together because writing tests, can lead to refactoring which sometimes improves the overall design. Let's look at the current code and try to write some tests.</p>
<p>First let's look at:</p>
<pre><code>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
}
</code></pre>
<p>The method <code>ask</code> is not easily testable because it depends on <code>os.Stdin</code>. Moreover functionality such as scanning, is not a very good fit for a method. It is usually recommended to keep structs lean and not attach much logic to them if any. Also it seems like the scanning functionality could be done by <code>quizLoop</code> directly and reduce the coupling.</p>
<pre><code>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 := q{question: 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)
}
</code></pre>
<p>The function <code>quizLoop</code> is not easily testable because it depends on opening a file, which needs to have a specific format. It also seems like it tries to do too many things (opens a file, reads questions, plays the quiz). In order to be able to write good tests we will remove some responsibility from this function and let it just play the quiz. Thus we need a new function that reads using the required format and returns a slice of questions. Let's call it <code>importQuizCSV</code> to signify that it depends on the CSV format.</p>
<pre><code>func importQuizCSV(r io.Reader) ([]q, error) {
var quiz = make([]q, 0)
csvr := csv.NewReader(r)
for {
record, err := csvr.Read()
if err != nil {
if err == io.EOF {
break
}
return nil, err
}
qq := q{question: record[0], answer: record[1]}
quiz = append(quiz, qq)
}
return quiz, nil
}
</code></pre>
<p>Notice that the function does not open a file. Writing tests for functions that open files is problematic. A better practice is to abstract away the source of reading by using the common <code>io.Reader</code> interface. The input can be a simple in memory string instead of a file which is ideal for testing. Here's an example of a <a href="https://github.com/golang/go/wiki/TableDrivenTests" rel="nofollow">table driven test</a> for <code>importQuizCSV</code>:</p>
<pre><code>// in main_test.go
func Test_importQuizCSV(t *testing.T) {
tests := []struct {
input string
quiz []q
}{
{
"",
[]q{},
},
{
"2+2,4",
[]q{
{question: "2+2", answer: "4"},
},
},
{
"2+2,4\n1+2,3",
[]q{
{question: "2+2", answer: "4"},
{question: "1+2", answer: "3"},
},
},
}
for _, tt := range tests {
r := strings.NewReader(tt.input)
quiz, err := importQuizCSV(r)
if err != nil {
t.Errorf("importQuizCSV(%q) returned error: %v", tt.input, err)
}
if got, want := quiz, tt.quiz; !reflect.DeepEqual(got, want) {
t.Errorf("importQuizCSV(%q) = %v, want %v", tt.input, got, want)
}
}
}
</code></pre>
<p>So now all that remains for <code>quizLoop</code> is to play the quiz. That means that it needs to receive the quiz (slice of questions), print each question, scan the answer, compare and keep track of the correct answers. If we scan the answers from <code>os.Stdin</code> we will have the same problem which makes testing difficult. Instead we can use the same technique and read from an <code>io.Reader</code>. We can extend this technique and instead of printing the answers to <code>os.Stdout</code> we can write them to an <code>io.Writer</code>. Let's rename the function to <code>playQuiz</code>.</p>
<pre><code>// playQuiz will:
//
// 1. Write questions to w
//
// 2. Read answers from r
//
// 3. Write score to w at the end of the quiz
func playQuiz(w io.Writer, r io.Reader, quiz []q) error {
correct := 0
for _, q := range quiz {
fmt.Printf("%s equals? ", q.question)
scanner := bufio.NewScanner(r)
scanner.Scan()
if scanner.Err() != nil {
return fmt.Errorf("scanning: %v", scanner.Err())
}
input := scanner.Text()
if input != q.answer {
fmt.Fprintln(w, "> Incorrect")
continue
}
fmt.Fprintln(w, "> Correct!")
correct++
}
fmt.Fprintf(w, "You had %d/%d correct answers!\n", correct, len(quiz))
return nil
}
</code></pre>
<p>A noteworthy detail is that it is common in Go to <a href="https://golang.org/doc/effective_go.html#if" rel="nofollow">omit unnecessary elses</a> and <a href="https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88" rel="nofollow">align the "happy path" to the left</a>. Of course in this case it could be argued that using an else, like in the original function, improves readability but I wanted to showcase the idiom.</p>
<p>Using the power of <code>io.Reader</code> and <code>io.Writer</code> now it is very easy to test the function:</p>
<pre><code>// in main_test.go
func Test_startQuiz(t *testing.T) {
quiz := []q{
{question: "2+2", answer: "4"},
{question: "1+2", answer: "3"},
}
input := "4\n2"
output := "> Correct!\n> Incorrect\nYou had 1/2 correct answers!\n"
r := strings.NewReader(input)
var buf bytes.Buffer
err := playQuiz(&buf, r, quiz)
if err != nil {
t.Errorf("play quiz %v with input %q returned error: %v", quiz, input, err)
}
if got, want := buf.String(), output; got != want {
t.Errorf("play quiz %v with input %q\nhave: %q\nwant: %q", quiz, input, got, want)
}
}
</code></pre>
<p>So now all we need is someone to open the file, call <code>importQuizCSV</code> and <code>playQuiz</code>. And of course there is no better candidate for that than <code>main</code>. Nevertheless, it is a good practice to use a function like <code>run() error</code> so that we can easily return errors with context and allow the caller to do centralized logging of the errors. As an added bonus, if we later want to move the function <code>run</code> to another package, the procedure becomes trivial.</p>
<pre><code>func main() {
if err := run(); err != nil {
fmt.Fprintln(os.Stderr, "Error:", err)
os.Exit(1)
}
}
func run() error {
var (
csvFile = flag.String("csv", "problems.csv", "Specify the path to the quiz questions.")
)
flag.Parse()
file, err := os.Open(*csvFile)
if err != nil {
return fmt.Errorf("opening CSV: %v", err)
}
defer file.Close()
quiz, err := importQuizCSV(file)
if err != nil {
return fmt.Errorf("importing quiz: %v", err)
}
return playQuiz(os.Stdout, os.Stdin, quiz)
}
</code></pre>
<p>In conclusion, thinking about testing, lead to refactoring which made the functions more flexible and testable. The responsibility of each function has been reduced and so did the coupling. For example it would be trivial to move these functions into a new package <code>quiz</code>, improve the naming (e.g. <code>quiz.Play</code>) and share the code if that is desirable. </p>
<p>Last but not least, the quiz game is no longer concerned about its actual input and output. It could be reading and writing to a network connection for all it knows!</p>
<p>Playground links of the full code:</p>
<ul>
<li><a href="https://play.golang.org/p/OpSpkdWptQk" rel="nofollow">main.go</a></li>
<li><a href="https://play.golang.org/p/4QN5wamCbjW" rel="nofollow">main_test.go</a></li>
<li><a href="https://play.golang.org/p/eikk5oe7DEZ" rel="nofollow">problems.csv</a></li>
</ul></pre>Kimput: <pre><p>First off, thanks a lot for taking so much time to write this up for me!</p>
<p>I'll have to properly go through this review. As mentioned in my question on CodeReview I've got really limited experience with GoLang, and some of the things that you are mentioning are things that I've never heard of, or used during the book that I read through (An into to programming in Go). </p>
<p>That being said, the things that you are mentioning are absolutely amazing! I can't believe the time you must have spent on writing this up. </p>
<p>For instance I love how you work with buffers to make the logic more extendable, something I had no clue you could do! Obviously (as you mention) it makes it a lot easier to test the underlying code. Having got experience with more object oriented programming, this is a rather well-known pattern to me. I just had no clue how I could do it in i GoLang. </p>
<p>I suppose I need to go back to the documentation and just go through each package at a time. :)</p>
<p>I also think you are right about the added complexity of adding verbosity to the code. But it's just something I make an effort to do when I program in Python, so that I have a way of debugging the code. :)</p></pre>nstratos: <pre><p>I've found that <a href="https://dave.cheney.net/2016/08/20/solid-go-design" rel="nofollow">SOLID Go Design</a> helps a lot with getting that "aha" moment when thinking about interfaces in Go. I wanted to mention it before but I ran out of words. :)</p>
<p>There's also "the bible": <a href="https://golang.org/doc/effective_go.html" rel="nofollow">Effective Go</a></p>
<p>And if you want something concise: <a href="https://github.com/golang/go/wiki/CodeReviewComments" rel="nofollow">CodeReviewComments</a></p></pre>iv0n: <pre><p>Very useful links. Thanks!!</p></pre>
