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 - Sopheary #29

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

Ports - Sopheary #29

wants to merge 2 commits into from

Conversation

sophearychiv
Copy link

No description provided.

raise NotImplementedError
return 1 if number == 0
return 1 if number == 1
raise ArgumentError if number == nil

Choose a reason for hiding this comment

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

If someone put in factorial("2") you'd get an exception. Consider checks such as if number.is_a?(Integer)


describe "factorial" do
describe "basic tests" do
it "factorial(5) = 120" do
factorial(5).must_equal 120
end

it "factorial(3) = 6" do

Choose a reason for hiding this comment

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

Nice addition of extra testing!

@@ -1,6 +1,16 @@
# Computes factorial of the input number and returns it
# Time complexity: ?
# Space complexity: ?
# Time complexity: O(n) where n is the number

Choose a reason for hiding this comment

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

How does time relate to the number?

# Time complexity: ?
# Space complexity: ?
# Time complexity: O(n) where n is the number
# Space complexity: O(1)

Choose a reason for hiding this comment

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

Please explain your reasoning for constant space.

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

Successfully merging this pull request may close these issues.

2 participants