solve it #12
solve it #12
Conversation
|
Just FYI I'm going to try to look this over this week - do you want a full review or just to have it added to the repo? |
joncalhoun
left a comment
joncalhoun
left a comment
There was a problem hiding this comment.
Overall you did a great job 🙌
One thing I will mention is that your solution appears to reset the time limit after each individual question. If you want some more practice, try to figure out how you would refactor this code to support a global time limit of say 30s for the entire quiz. With your code I don't think it would be that much work, and I'm happy to help out if you have questions.
| func ReadStringWithLimitTime(limit int) (string, error) { | ||
| timer := time.NewTimer(time.Duration(limit) * time.Second).C | ||
| doneChan := make(chan bool) | ||
| answer, err := "", error(nil) |
There was a problem hiding this comment.
I'm not a huge fan of this line here. What you are doing isn't wrong by any means, so this is a readability/style comment.
If it were me, I'd instead replace this with:
var answer string
var err error
// or
var (
answer string
err error
)Or even as a third alternative, you could use named return variables (I like this one less personally, but it is readable):
func ReadStringWithLimitTime(limit int) (answer string, err error) {Yes, the first two options I presented are more code but I think it becomes clearer that you are just initializing them with zero values.
Building on that, you never actually use the err value. I mean, you return it on line 35:
return answer, errBut this is the only place you return it, and we know for certain that it is going to be nil at this point (at least I do after looking at the earlier code - it isn't as clear when reading line 35).
You could probably replace this line with var answer string then on line 35 do:
return answer, nilNow it is clear you are returning a nil error in this case and you don't need that err variable anymore 😺
| "time" | ||
| ) | ||
|
|
||
| type recordtype struct { |
There was a problem hiding this comment.
nitpick: I believe this should be recordType.
|
|
||
| // ParseLines - parse lines from array of array of string to array of recordtype | ||
| func ParseLines(lines [][]string) []recordtype { | ||
| ret := make([]recordtype, len(lines)) |
There was a problem hiding this comment.
👍 Nice job allocating exactly how much memory you use. Its minor, but really cool to get in the habit of it when you know that info.
| for i, line := range lines { | ||
| ret[i] = recordtype{ | ||
| question: line[0], | ||
| answer: strings.TrimSpace(line[1]), |
There was a problem hiding this comment.
I like that you trimmed up these strings 😄
| println("Time expire!") | ||
| break | ||
| } | ||
| if strings.ToLower(strings.Trim(answer, "\n ")) == p.answer { |
There was a problem hiding this comment.
I don't think you need the \n part of this, and you may want to check out TrimSpace. What you have works, but TrimSpace is just a nice helper to get all kinds of whitespace.
On another note - you do ToLower here, but you DO NOT do that to input from the CSV. I would consider adding a call to ToLower in your ParseLines function to guarantee that both user input and csv input are both in the same case.
|
|
||
| answer, err := ReadStringWithLimitTime(*limit) | ||
| if err != nil { | ||
| println("Time expire!") |
There was a problem hiding this comment.
Stick to fmt.Println instead of println. The latter was added to the language as part of the bootstrapping process, but may be removed from the language at some point breaking code like this, so the former (fmt.Println) is preferred. Source: https://golang.org/ref/spec#Bootstrapping
2 participants
It is my solution