Lab1 fib first submission #610
Conversation
BaiJieCN
requested review from
anzboi,
anzdaddy,
camscale,
joshcarp,
juliaogris,
linuxmonk,
pentaphobe,
rokane and
samuong
as code owners
01_fib/baijie/fib_test.go
Outdated
| {input: 1, | ||
| expected: strconv.Quote("1\n")}, | ||
| {input: -1, | ||
| expected: strconv.Quote("1\n")}, |
There was a problem hiding this comment.
File is not goimports-ed (from goimports)
| expected: strconv.Quote("1\n")}, | |
| expected: strconv.Quote("1\n")}, |
Codecov Report
@@ Coverage Diff @@
## master #610 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 209 210 +1
Lines 3952 3966 +14
=====================================
+ Hits 3952 3966 +14
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Please read through the Collected Code Review Comments
Please fix commit messages with git rebase -i HASH
Do not comment the obvious (pretty much all comments in this code base) - save comments to explain what cannot be derived from the code in one glance
01_fib/baijie/fib_test.go
Outdated
| actual := strconv.Quote(buf.String()) | ||
|
|
||
| if expected != actual { | ||
| t.Errorf("Unexpected output in main()\nexpected: %q\nactual: %q", expected, actual) |
There was a problem hiding this comment.
Just a couple of minor things, looks mostly good.
Also, please fix commit messages with git rebase -i HASH
01_fib/baijie/fib_test.go
Outdated
|
|
||
| tests := []test{ | ||
| {input: 0, | ||
| expected: strconv.Quote("")}, |
There was a problem hiding this comment.
to me it would be more readable if you put it on one line.
01_fib/baijie/fib_test.go
Outdated
| expected: strconv.Quote("1\n")}, | ||
| {input: 7, | ||
| expected: strconv.Quote(`1 | ||
| 1 |
There was a problem hiding this comment.
sorry should have said this early: consider writing this as: strings.Join([]string{"1","1","2","3"}, "\n") or similar.
01_fib/baijie/fib.go
Outdated
|
|
||
| var out io.Writer = os.Stdout | ||
|
|
||
| // fib function to generate fibonacci sequence |
There was a problem hiding this comment.
This comment is a bit redundant - it's easy to see what the function does without it. If you feel that you must comment, a suggestion would be
| // fib function to generate fibonacci sequence | |
| // fib generates a fibonacci sequence for _n_. It expects the existence of a | |
| // variable named _out_ of type _io.Writer_, and will output the sequence | |
| // to that variable; this can be used for eg. printing on screen or testing. |
Personally I'd just leave it out for simplicity.
01_fib/baijie/fib.go
Outdated
|
|
||
| // fib function to generate fibonacci sequence | ||
| func fib(i int) { | ||
| // initial values |
There was a problem hiding this comment.
This is positively a comment that can be left out, as in my opinion it does not add value to the code.
| // initial values |
01_fib/baijie/fib.go
Outdated
| m, n = n, m | ||
| fmt.Fprintln(out, m) | ||
| } | ||
| // negative case |
There was a problem hiding this comment.
Another comment that I don't think adds much value. If you must, I suggest aligning it with the upper comment for legibility. Personally, I would leave both out.
01_fib/baijie/fib.go
Outdated
|
|
||
| // main function | ||
| func main() { | ||
| fib(1) |
There was a problem hiding this comment.
Please correct this as the specs say fib(7), not fib(1): https://github.com/anz-bank/go-course/blob/master/01_fib/README.md
01_fib/baijie/fib_test.go
Outdated
| out = &buf | ||
|
|
||
| main() | ||
| expected := strconv.Quote("1\n") |
There was a problem hiding this comment.
You also want to change this to test for the output on the spec.
01_fib/baijie/fib_test.go
Outdated
|
|
||
| main() | ||
| expected := strconv.Quote("1\n") | ||
| actual := strconv.Quote(buf.String()) |
There was a problem hiding this comment.
Let me know if I'm wrong, but I don't think using strconv.Quote here and above adds value.
01_fib/baijie/fib_test.go
Outdated
| {input: -1, expected: strconv.Quote("1\n")}, | ||
| {input: 7, expected: strconv.Quote(strings.Join(s1, "\n") + "\n")}, | ||
| {input: -7, expected: strconv.Quote(strings.Join(s2, "\n") + "\n")}, | ||
| } |
There was a problem hiding this comment.
While I agree that the strings look much neater like this, pushing everything inside a anonymous struct literal makes it look even better. Have a look at https://dave.cheney.net/2019/05/07/prefer-table-driven-tests.
| if test.expected != actual { | ||
| t.Errorf("Unexpected output in main()\ninput: %d\nexpected: %q\nactual : %q", test.input, test.expected, actual) | ||
| } | ||
| buf.Reset() |
There was a problem hiding this comment.
You could avoid having to do a buf.Reset() if you pushed lines 24-25 inside the for loop.
There was a problem hiding this comment.
Do you mean move below lines inside the for loop?
var buf bytes.Buffer
out = &buf
There was a problem hiding this comment.
Yes. If you moved them inside the loop at the top, each iteration of the loop would start with a fresh buffer that you do not need to Reset() at the end of the loop.
01_fib/baijie/fib_test.go
Outdated
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
File is not goimports-ed (from goimports)
|
Alex, could you advise what change is required pls? |
There was a problem hiding this comment.
This is a pretty good submission for the first lap. There are a couple of minor things that could be cleaned up in the code, but there is also the problems that your linked issue is empty and your linked review does not exist.
While the issue and review are not directly related to Go, they are related to software engineering and are a requirement of this course. Please have a read of https://github.com/anz-bank/go-course/wiki/Software-Engineering-with-Go to understand a bit more of what I mean about "software engineering".
Then, please add some content to your linked issue, and perform a peer review of someone elses PR.
Thanks
| if i >= 0 { | ||
| for j := 0; j < i; j++ { | ||
| m += n | ||
| m, n = n, m |
There was a problem hiding this comment.
These two lines are confusing. You are first changing m, and then changing it again in the next line, but referencing the first change. You can write this as one line:
m, n = n, m+n
This applies to the other case of the if statement too.
| if test.expected != actual { | ||
| t.Errorf("Unexpected output in main()\ninput: %d\nexpected: %q\nactual : %q", test.input, test.expected, actual) | ||
| } | ||
| buf.Reset() |
There was a problem hiding this comment.
Yes. If you moved them inside the loop at the top, each iteration of the loop would start with a fresh buffer that you do not need to Reset() at the end of the loop.
|
The go-course is now closed. Thank you very much for participating. |
Fixes #609
Review of colleague's PR #196
Changes proposed in this PR: