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

Fire - Noor #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fire - Noor #37

wants to merge 1 commit into from

Conversation

hn4ever
Copy link

@hn4ever hn4ever commented Nov 3, 2020

No description provided.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Some issues here, and other methods are well done. Take a look at my comments and let me know what questions you have via Slack.

Comment on lines +3 to 5
# Time complexity: O(n)
# Space complexity: O(n)
def factorial(n)

Choose a reason for hiding this comment

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

👍

Comment on lines +11 to 13
# Time complexity: O(n)
# Space complexity: O(n)
def reverse(s)

Choose a reason for hiding this comment

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

⚠️ This isn't recursive!!!

Comment on lines +27 to 29
# Time complexity: O(n)
# Space complexity: O(n)
def reverse_inplace(s)

Choose a reason for hiding this comment

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

⚠️ This isn't done in-place!!! It's also O(n^2) for both space/time complexity!

Think about if we changed the method signature to the following

Suggested change
# Time complexity: O(n)
# Space complexity: O(n)
def reverse_inplace(s)
# Time complexity: O(n)
# Space complexity: O(n)
def reverse_inplace(s, low = 0, high= s.length - 1)

Can you see a way to do this with O(n) space/time complexity now?

Comment on lines +38 to 40
# Time complexity: O(n)
# Space complexity: O(n)
def bunny(n)

Choose a reason for hiding this comment

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

👍

Comment on lines +50 to +51
if s[i+1] == "(" && s[-i] == ")"
return true

Choose a reason for hiding this comment

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

Suggested change
if s[i+1] == "(" && s[-i] == ")"
return true
if s[i-1] == "(" && s[-i] == ")"
return nested(s, i + 1)

Comment on lines +59 to +61
# Time complexity: O(n)
# Space complexity: O(n)
def search(array, value, i = 0)

Choose a reason for hiding this comment

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

👍

Comment on lines +68 to 70
# Time complexity: O(n)
# Space complexity: O(n)
def is_palindrome(s)

Choose a reason for hiding this comment

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

👍 Because of the slice you are doing with each recursive call, it's O(n^2)

Comment on lines +81 to +87
if n[i] == m[i]
return count + 1
else
return count
end

return digit_match(n, m, i + 1, count)

Choose a reason for hiding this comment

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

Suggested change
if n[i] == m[i]
return count + 1
else
return count
end
return digit_match(n, m, i + 1, count)
if n[i] == m[i]
count += 1
end
return digit_match(n, m, i + 1, count)

Comment on lines +78 to +79
n.to_s
m.to_s

Choose a reason for hiding this comment

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

Suggested change
n.to_s
m.to_s
n = n.to_s
m = m.to_s

Comment on lines +75 to +77
# Time complexity: O(n)
# Space complexity: O(n)
def digit_match(n, m, i = 0, count = 0)

Choose a reason for hiding this comment

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

⚠️ Not quite working, check the suggestions below.

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