Lab1 - Generate Fibonacci Series #625

jimbosoft · 2019-09-03T00:41:58Z

Fixes #630

Review of colleague's PR #514

Changes proposed in this PR:

Submit lab1 Fibonacci from the go course

-

codecov · 2019-09-03T00:53:55Z

Codecov Report

Merging #625 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #625   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         209    210    +1     
  Lines        3952   3969   +17     
=====================================
+ Hits         3952   3969   +17

Impacted Files	Coverage Δ
01_fib/jimbotech/ex-fibonacci.go	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa5b659...56f27df. Read the comment docs.

codecov · 2019-09-03T00:53:55Z

Codecov Report

Merging #625 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #625    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files         239    235     -4     
  Lines        4757   4637   -120     
======================================
- Hits         4757   4637   -120

Impacted Files	Coverage Δ
01_fib/jimbotech/ex_fibonacci.go	`100% <100%> (ø)`
10_rest/kasunfdo/pkg/puppy/store/mapstore.go
10_rest/kasunfdo/pkg/puppy/store/syncstore.go
10_rest/kasunfdo/pkg/puppy/error.go
10_rest/kasunfdo/pkg/puppy/puppy.go
10_rest/kasunfdo/pkg/puppy/rest.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32d6a28...dbf2201. Read the comment docs.

willshen8

Apart from the comments below, I think the filename needs to be snakecase to align with convention:
https://stackoverflow.com/questions/35694137/file-name-convention-for-compound-words

willshen8 · 2019-09-03T23:17:22Z

01_fib/jimbotech/.gitignore

@@ -0,0 +1,2 @@
+/.vscode/*


Only source files should be committed.

I usually check in the .gitignore and others also consider it normal practise as per https://stackoverflow.com/questions/5765645/should-you-commit-gitignore-into-the-git-repos, however in this context it might not add a lot, so I removed it.

01_fib/jimbotech/README.md

01_fib/jimbotech/ex-fib_test.go

01_fib/jimbotech/ex-fibonacci.go

willshen8 · 2019-09-03T23:52:01Z

Also please open an 'issue' and put the issue number inside Fixes # of the PR description. The go-course require commit messages to be squashed, so you might have to do an interactive rebase.

willshen8 · 2019-09-04T10:11:08Z

I moved the issue number up in the PR description (into the first line), I think maybe I wasn't clear. Hope you see it now.

camscale

The file name ex_fib_test.go should be called ex_fibonacci_test.go - i.e. the same as the name as the file being tested but with _test appended before the extension. This is not something that is explicitly documented but is a convention I've never seen not followed.

My general approach to these reviews is to classify my comments into two types: Go-specific ones that need to be addressed to merge, and general software engineering practice ones which are advisory.

In this review, just the comments about error strings are Go-specific. So please at least fix those up (all instances - I did not leave a comment on every one, so look at all your error strings) before submitting for re-review. It is up to you as to whether you want to address the other comments, but it is good practice to at least respond to all comments with an acknowledgement or reason for ignoring.

Finally, a couple of comments on your commits and commit messages. Have a read of https://chris.beams.io/posts/git-commit/ (also linked from the slides) on what makes a good commit message. In particular, your first commit titled "INITIAL COMMIT" does not have a good title. It is shouty and non-descriptive. The initial split into five commits is fine and well done, but it does mean you need to take more action with your subsequent fixup commits - the should be squashed with the commits where the original code being fixed was written. We do not want to see fixup commits merged into master - the place to get your code and commits into order is the pull request. When merging it should look as though a series of commits was written "perfectly" the first time. If you started with one commit and intended to only keep one commit, you could do a "squash merge" when merging the PR to remove all evidence of the fix-ups. But if you want to retain your original split, you'll need to manually apply the fix-ups with git rebase. Remember, history is written by the victors. Be a victor!

camscale · 2019-09-08T10:21:50Z

01_fib/jimbotech/ex_fibonacci.go

+
+	if n < 0 {
+		for i := 0; i > n; i-- {
+			factor := int(math.Pow(-1, float64(i*-1)))


The computation of negative fibonacci numbers really belongs where the sequence is calculated. This function is responsible for printing n fib numbers. Instead you've split the computation over two separate functions for negative numbers. For instance, if you wanted to have another function return a slice of fib numbers instead of printing them, you would have to duplicate this negative number handling. But you've kind of backed yourself into a corner with your closure implementation of fib - it has no notion of which direction to go in so it cannot compute negative numbers.

Also the computation of -1 or 1 here is a bit obtuse. i*-1 is more simply expressed as -i and using floating point calculations just to alternate between -1 and 1 seems unnecessary. If it were possible to write it as -1^-i in simple mathematical notation, then it may be worth doing. But with the casts and math.Pow instead of ^ or similar, it becomes that much harder to mentally parse and understand. An alternative is to use a variable sign initialised to 1, and each time around the loop you just do sign *= -1 - that way it alternates but is a much simpler expression.

Done ... however I feel hardly done by. I feel I am handling the negative numbers in the right place as the series is being generated there. So now I have separated the generation from the printing. Am I going to be criticised for inefficiency now? I was hoping for bonus points not bonus corrections. Secondly, as this is a exerciser I felt proud of my play with the math library and expected applause. Yes I did not like floating point either. When I re-wrote it I needed ABS, guess what, floating point again. So can I use any of the math library?

camscale · 2019-09-08T10:23:32Z

01_fib/jimbotech/ex_fibonacci.go

+			factor := int(math.Pow(-1, float64(i*-1)))
+			fmt.Fprintln(out, f()*factor)
+		}
+


nitpick: stray blank line. unnecessarily spaces out the code, which matters on real code that does not fit on one screen. blank lines to separate "paragraphs" of code are good, but do think about what your "code paragraphs" are.

camscale · 2019-09-08T10:25:22Z

01_fib/jimbotech/ex_fib_test.go

+
+	for _, val := range fibResults {
+		if r := f(); r != val {
+			t.Errorf("Returned value %v does not match %v", r, val)


error messages in Go should start with lower case letters. This is because error messages are often wrapped as the propagate causing a SeQuEnCe of lower/upper case letters: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

camscale · 2019-09-08T10:30:32Z

01_fib/jimbotech/README.md

@@ -0,0 +1,15 @@
+# Fibonacci Number Generator
+## Overview
+As part of the ANZ "go" course the first lab is to produce a piece of software that will produce the Fibonacci series for fib(7). It was also an exercise in the tour.golang.org.


nitpick: Markdown is an extension of plain old ascii documents, in that ascii markup is added so it can be prettier when rendered as markdown. But the files should still be readable as ascii documents. This means wrapping text around 80 chars (the general convention), as people use all sorts of different width terminals, and some that require side scrolling to read wider text. So please make markdown text readable as plain ascii as originally intended.

Plenty has been written about line length / text width - here's one example: https://baymard.com/blog/line-length-readability

* Implement lab9 requirements Implement ability to convert between Go in memory objects to JSON representation and vice versa. Implement ability to accept user input from CLI and read data from a file. Load data from json text file and store in an instance of storer. * Add tests to main_test.go * Update README file to reflect changes in lab9 * Fix pesky scopelint error * Fix pesky test coverage issue Create new test with totally invalid json * Fix final lint problem Lint issue: should merge variable declaration with assignment on next line * Add 2 json files to read into program * Fix PR feedback 1) add defer f.close() to close file 2) use kingpin.ExistingFile no kingpin.String 3) tried using kingpin.fatalusage() to replace my printusage() func but struggled to hit it for test coverage 4) Remove ./ at start of paths 5) separate flag parsing and loading a file

Modified version of the Excersise: Fibonacci closure from the Tour of Go It already contains some review comments from a previous attempt

* Removed unwanted blank lines * Separated the printing of the series from the generation * Removed math function that use floating point

* Renamed the file to go convention * Made sure that error messages start with a lower case letter

camscale

There is something very wrong with this PR. It has 29 files changed in it with code from other people's lab. I suggest you checkout the latest master and rebase on that, then force push. That should clean it up, but I am not sure how you got here so cannot offer precise guidance of how to get out.

I cannot re-review until this is fixed.

camscale

LGTM.

BTW, if you wanted to incorporate negative fibonacci numbers into your closure way of generating numbers:

func fibonacci(direction int) func() int {
	if direction < 0 {
		direction = -1
	} else {
		direction = 1
	}
	secondLast, last := 0, 1

	return func() int {
		result := last
		secondLast, last = last, secondLast+(last*direction)
		return result
	}
}

Then you could just call f := fibonacci(n) and iterate over the result of f() which would remove the need for the intermediate slice. Or slightly more verbose but taking care of the iteration properly:

f := fibonacci(n)
limit := n
if n < 0 {
     limit = -n
}
for i := 0; i < limit; i++ {
    fmt.Fprintln(out, f())
}

I'm not requesting this change, just pointing out what I had in mind with my previous comments.

jimbosoft requested review from anzboi, anzdaddy, camscale, joshcarp, juliaogris, linuxmonk, pentaphobe, rokane and samuong as code owners September 3, 2019 00:41

jimbosoft added Ready for review lab1 labels Sep 3, 2019

willshen8 added Under review and removed Ready for review labels Sep 3, 2019

willshen8 requested changes Sep 3, 2019

View reviewed changes

willshen8 added Changes requested and removed Under review labels Sep 3, 2019

jimbosoft added Ready for review and removed Changes requested labels Sep 4, 2019

willshen8 self-requested a review September 4, 2019 10:12

willshen8 previously approved these changes Sep 4, 2019

View reviewed changes

willshen8 added the Approved by colleague label Sep 4, 2019

camscale suggested changes Sep 8, 2019

View reviewed changes

camscale added Changes requested and removed Ready for review labels Sep 8, 2019

harirakr and others added 2 commits September 9, 2019 12:52

Add lab5 - stringer interface

ff14d30

jimbosoft dismissed willshen8’s stale review via a799821 September 9, 2019 02:55

jimbosoft force-pushed the lab1 branch 3 times, most recently from d5fddee to 9cf1af7 Compare September 9, 2019 03:40

Jim hejtmanek added 4 commits September 9, 2019 13:41

Implementation of Fibonacci Series for lab1

6c2ac60

Modified version of the Excersise: Fibonacci closure from the Tour of Go It already contains some review comments from a previous attempt

Added handling of negative numbers

ddb2210

Unit tests to verify all operations

08cd89f

Added required README for documentation

0f72e56

jimbosoft force-pushed the lab1 branch from 9cf1af7 to 0f72e56 Compare September 9, 2019 03:42

Jim hejtmanek added 3 commits September 9, 2019 16:24

Fixed issues resulting from the last PR review

5e0a7d3

* Removed unwanted blank lines * Separated the printing of the series from the generation * Removed math function that use floating point

Fixed test file as a result of the last PR review

95ed1ea

* Renamed the file to go convention * Made sure that error messages start with a lower case letter

Fixed formatting as a result of the PR review

acc8c32

jimbosoft added Ready for review and removed Changes requested labels Sep 10, 2019

Changed sign reversal

152df59

This was referenced Sep 15, 2019

Lab11 - Puppy Notification #660

Closed

Lab11 - Puppy Notifiaction #661

Merged

camscale suggested changes Sep 15, 2019

View reviewed changes

camscale added Changes requested and removed Ready for review labels Sep 15, 2019

jimbosoft and others added 2 commits September 18, 2019 15:42

Merge branch 'master' into lab1

df7df91

Removed unwanted blank lines

bd78645

jimbosoft added Ready for review and removed Changes requested labels Sep 18, 2019

camscale approved these changes Sep 21, 2019

View reviewed changes

camscale added Merge me and removed Ready for review labels Sep 21, 2019

Merge branch 'master' into lab1

dbf2201

jimbosoft merged commit 3411b10 into anz-bank:master Sep 24, 2019

jimbosoft deleted the lab1 branch September 24, 2019 05:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lab1 - Generate Fibonacci Series #625

Lab1 - Generate Fibonacci Series #625

jimbosoft commented Sep 3, 2019 •

edited by willshen8

Loading

codecov bot commented Sep 3, 2019

codecov bot commented Sep 3, 2019 •

edited

Loading

willshen8 left a comment

willshen8 Sep 3, 2019

jimbosoft Sep 4, 2019

willshen8 commented Sep 3, 2019

willshen8 commented Sep 4, 2019

camscale left a comment

camscale Sep 8, 2019

jimbosoft Sep 9, 2019

camscale Sep 8, 2019

jimbosoft Sep 9, 2019

camscale Sep 8, 2019

jimbosoft Sep 9, 2019

camscale Sep 8, 2019

jimbosoft Sep 9, 2019

camscale left a comment

camscale left a comment

Lab1 - Generate Fibonacci Series #625

Lab1 - Generate Fibonacci Series #625

Conversation

jimbosoft commented Sep 3, 2019 • edited by willshen8 Loading

Changes proposed in this PR:

-

codecov bot commented Sep 3, 2019

Codecov Report

codecov bot commented Sep 3, 2019 • edited Loading

Codecov Report

willshen8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willshen8 commented Sep 3, 2019

willshen8 commented Sep 4, 2019

camscale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camscale left a comment

Choose a reason for hiding this comment

camscale left a comment

Choose a reason for hiding this comment

jimbosoft commented Sep 3, 2019 •

edited by willshen8

Loading

codecov bot commented Sep 3, 2019 •

edited

Loading