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

Ports - Ngoc #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions lib/fibonacci.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,23 @@
# ....
# e.g. 6th fibonacci number is 8

# Time complexity: ?
# Space complexity: ?
# Time complexity: O(n), n is the number of computations this calculation needs to run to compute the nth fibonacci number

Choose a reason for hiding this comment

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

"the number of computations this calculation needs to run" is sort of what you're trying to define by explaining the "time complexity" of a method, so you don't want to use that in the definition. "n" as you're using it here is actually the value of the input variable n. The time complexity is n because you're iteratively calculating each fibonacci number up to n.

# Space complexity: O(1), space complexity is constant because this method only require a storage for 3 variables (p,p1,p2) regardless of number of operations

Choose a reason for hiding this comment

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

Nice.

def fibonacci(n)
raise NotImplementedError
if n.nil? || n < 0
raise ArgumentError
end
if n == 0
return 0
else
p1 = 0

Choose a reason for hiding this comment

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

[nit] Why p? (Not that you should change it, it's just not obvious to me.)

Copy link
Author

Choose a reason for hiding this comment

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

You have a good point. Do you think naming it p3 would make it easy to understand, since it is the number subsequent to p2 ?

Choose a reason for hiding this comment

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

I was actually curious why you were using p1 and p2 and p3. What's the "p" for?

p2 = 1

(n - 1).times do
p = p1 + p2
p1 = p2
p2 = p
end
return p2
end
end