Skip to content

Conversation

@hellosputnik
Copy link
Contributor

No description provided.

@hellosputnik
Copy link
Contributor Author

Since I'm new to Go, I would really appreciate an honest code review :)

@joncalhoun
Copy link
Contributor

Happy to review but it is going to be a few days until I have time. Mind waiting on me?

@hellosputnik
Copy link
Contributor Author

Yeah, of course. Thank you for creating these exercises!

fin := bufio.NewScanner(file)

// Read the problems from the comma-separated values file.
for fin.Scan() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid this using the package "encoding/csv" in the standard library

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hellosputnik Generally speaking when dealing with a CSV trying to do it on your own is dangerous. Eg what happens if someone gives you the input:

"Yo, what is 2+2?", 4

From the looks of your code, this would probably end up causing issues.

I'm guessing you didn't know about the encoding/csv package so it's a completely understandable (and common) mistake.

Copy link
Contributor

@joncalhoun joncalhoun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is an awesome stab at the exercise. I left a lot of comments, but that doesn't mean what you wrote is bad. I'm just trying to give you as much feedback as possible 😄

fin := bufio.NewScanner(file)

// Read the problems from the comma-separated values file.
for fin.Scan() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hellosputnik Generally speaking when dealing with a CSV trying to do it on your own is dangerous. Eg what happens if someone gives you the input:

"Yo, what is 2+2?", 4

From the looks of your code, this would probably end up causing issues.

I'm guessing you didn't know about the encoding/csv package so it's a completely understandable (and common) mistake.


type Quiz struct {
problems []Problem
score int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it doesn't matter, but when putting things like problems and score in the same struct try to think about how they might be used.

Eg if we planned on giving the same quiz to 10 students then perhaps it makes more sense to have two types - one that is the "quiz" and stores the problems, another that is the answer sheet and maybe has responses plus a total score. You can kinda think of it like when your teacher hands you a quiz and says, "Don't write on the quiz, write on this separate answer sheet".

That said, if you expect your quizes to be unique for each student (eg maybe you mix up the problem order for each student) then maybe in those cases it makes more sense to store it like you have.

Again, this doesn't matter given the scope of this exercise, but just something I wanted to point out so you think about it in the future as you design your struct types 😄


func main() {
quiz := Quiz{}
settings := Settings{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a personal preference, but when creating the zero value of a type I prefer to do:

var settings Settings

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bonus points for

var (
	quiz Quiz
	settings Settings
)

settings := Settings{}

settings.filename = flag.String("csv", "problems.csv", "a csv file in the format of 'question,answer'")
settings.timeLimit = flag.Int("limit", 30, "the time limit for the quiz in seconds")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanted you could use flag.IntVar and flag.StringVar here. Eg:

type Settings struct {
	filename  string
	timeLimit int
}

func main() {
	var settings Settings
	flag.StringVar(&settings.filename, "csv", "problems.csv", "a csv file in the format of 'question,answer'")
	flag.IntVar(&settings.timeLimit, "limit", 30, "the time limit for the quiz in seconds")
	flag.Parse()
	fmt.Println(settings)
}

The primary upside is not needing to mess with string and integer pointers from here onwards for those values.

if err != nil {
panic(err)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a personal preference, but for code like os.Open(); ... if err ... defer file.Close() I prefer to group it together without spaces. For me it just makes it easier when things are "grouped" and it makes it obvious that I didn't forget to defer closing the file.

defer timer.Stop()

go func() {
<-timer.C
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will run as soon as the time limit is up, but it won't terminate your program. I cover a few ways to do this in the videos that go w/ this exercise, but one way to quickly add it to your code would be to add something like below to the end of this goroutine closure:

os.Exit(0)

os.Exit will cause your entire application to exit and return a status code. You can read more about it here - https://golang.org/pkg/os/#Exit

var input string
fmt.Scan(&input)

if input == problem.answer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want some more practice using Go you could look at ways to trim both the user in put and the problem answers of any extra whitespace, or even try to convert everything to upper or lowercase to avoid any case sensitivity issues.

@joncalhoun joncalhoun added the in review PR/Issue is currently undergoing code review label Mar 6, 2018
@hellosputnik
Copy link
Contributor Author

I appreciate the feedback. Thanks, Jon!

@joncalhoun joncalhoun merged commit 617e1ce into gophercises:master Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in review PR/Issue is currently undergoing code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants