Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lab 1: Fibonacci #514

Closed
wants to merge 7 commits into from
Closed

Lab 1: Fibonacci #514

wants to merge 7 commits into from

Conversation

krnicolson
Copy link
Collaborator

@krnicolson krnicolson commented Jun 29, 2019

Fixes #539

Review of colleague's PR #379

Changes proposed in this PR:

  • fib(n) function to print fibonacci series up to n
  • call to fib() from main with supplied input
  • test main and fib

Copy link
Member

@anzboi anzboi left a comment

Choose a reason for hiding this comment

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

Looking good so far.

  1. Please write a proper PR description.
  2. Review someone elses PR
  3. Fix CI failure (run gofmt and goimports, you can usually configure your IDE to run these automatically)

fmt.Fprintln(out, sum)

// update variables for next iteration
previous = current
Copy link
Member

Choose a reason for hiding this comment

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

You can use multiple assignment here without worrying about overwriting

previous, current = current, sum

You can actually use this before the print to remove the sum variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Neat! Thanks Dan 👍


fib(-1)

expected := strconv.Quote("fib(n) doesn't accept negative integers\n")
Copy link
Member

Choose a reason for hiding this comment

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

👍 for testing your implementation.

@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #514   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         107    108    +1     
  Lines        1861   1876   +15     
=====================================
+ Hits         1861   1876   +15
Impacted Files Coverage Δ
01_fib/nicholsk/main.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 40114a7...9834b4b. Read the comment docs.

@krnicolson krnicolson changed the title add fibonacci main and test WIP: add fibonacci main and test Jul 4, 2019
@krnicolson krnicolson changed the title WIP: add fibonacci main and test Lab 1: Fibonacci Jul 7, 2019
@krnicolson krnicolson requested a review from samuong as a code owner July 21, 2019 06:12
@krnicolson krnicolson requested a review from anzboi July 21, 2019 06:12
Copy link
Contributor

@juliaogris juliaogris left a comment

Choose a reason for hiding this comment

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

Please read through the Collected Code Review Comments and fix commit messages with git rebase -i HASH.

}

func fib(n int) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary blank line.
No need for blank lines at beginning or end of block.
Use blank lines between type, method and function definitions and within a block for logical separation.
Please fix throughout codebase.


current, previous := 1, 0

// print first value which is always 1
Copy link
Contributor

Choose a reason for hiding this comment

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

No really needed if you rearrange printing within your loop:

	n1, n2 := 1, 1
	for i := 0; i < n; i++ {
		fmt.Println(n1)
		n1, n2 = n2, n1+n2
	}

@juliaogris
Copy link
Contributor

Sorry it took so long to get back to this PR - feel free to reach out to me personally and I will get right to it if/when your update is ready.

Copy link
Collaborator

@jimbosoft jimbosoft left a comment

Choose a reason for hiding this comment

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

Looks good to me. Through testing, neat code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lab 1 - Fibonacci
7 participants