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.

Sign up for GitHub

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

Jump to bottom

Ports - Margaret #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Ports - Margaret #14

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f010889
created factorial method
msfinnan Apr 9, 2019
8a567f1
removed commented out code from earlier attempt
msfinnan Apr 9, 2019
577de3a
worked out new method
msfinnan Apr 10, 2019
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
54 changes: 51 additions & 3 deletions lib/factorial.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,54 @@
# Computes factorial of the input number and returns it
# Time complexity: ?
# Space complexity: ?
# Time complexity: Linear or O(n) where n is the number input
Copy link

Choose a reason for hiding this comment

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

Please also briefly explain why.

# Space complexity: Constant or O(1), will have the same number of variables no matter the size of the input
Copy link

Choose a reason for hiding this comment

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

I like this concise explanation for the space complexity. :)


def factorial(number)
raise NotImplementedError
return 1 if number == 0
Copy link

Choose a reason for hiding this comment

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

[nit] You should definitely indent all the lines between def factorial(number) and the final end for readability.

if number == nil
raise ArgumentError, "Enter an integer"
end
a = 1
b = number
Copy link

Choose a reason for hiding this comment

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

[nit] I had to run the tests and stare at this to prove to myself that factorial(1) returns the right thing. In other cases I'd say it's duplicative to add && number == 1 to your return 1 if statement up at the top, but in this case I think it might actually benefit readability. Not something you need to change; I only note it to show you how unpredictable "readability" can be as a problem. Humans, am I right?

while a < number
b = b * (number - a)
Copy link

Choose a reason for hiding this comment

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

a and b aren't particularly descriptive variable names. For b, I'd suggest calling it product -- you're using it to accumulate the overall product of every number between 1 and number. a is a little harder to name, but you could decrement number (i.e. do number -= 1 every loop) in order to not need a at all.

a += 1
end
return b
end



Copy link

Choose a reason for hiding this comment

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

[nit] It's good to get in the habit now of deleting lots of empty lines at the end of code files. You'll definitely get nit-picky feedback on these sorts of formatting things on the job.