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

Sockets - Evelynn #23

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

Conversation

evelynnkaplan
Copy link

…e complexity

@evelynnkaplan evelynnkaplan changed the title Wrote method to pass tests, added question in comments, time and spac… Sockets - Evelynn Apr 11, 2019
Copy link

@eric-andeen eric-andeen left a comment

Choose a reason for hiding this comment

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

Reviewed with comments.

# Time complexity: O(n), where n is the value of number. The loop will run number - 2 times, so the
# amount of time the loop runs varies linearly based on the value of the input number.
# Space complexity: O(1), because the method will store the same amount of data no matter
# the value of the input number. Question - initially I wrote this method creating a new variable on line 19,

Choose a reason for hiding this comment

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

Code comments are not a great place for this kind of conversation. Remove before final commit.

raise ArgumentError, "Please enter an integer, you've entered nil."
elsif number == 0
return 1
elsif number < 0

Choose a reason for hiding this comment

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

Put error cases (number < 0) before normal cases (number == 0). Also, you could include (number == 1) here if you want. The loop below handles it, but calling it out separately may make the loop simpler and more efficient.

raise ArgumentError, "Please enter a positive number."
end

lesser_number = number - 1

Choose a reason for hiding this comment

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

lesser_number is not a good variable name. It doesn't give the reader any idea what it does.

# above lesser_number. The variable was called total, and the value was set to the value of the number. Instead of the loop
# multiplying number by lesser_number and constantly changing the value of number to store the factorial value, total stored
# this value. Would this be a better solution, so as not to change the value of number? Or is eliminating the use of another
# variable more space-efficient?

Choose a reason for hiding this comment

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

It's totally fair game (in any language, not just Ruby) to modify the input number. It's passed by value, not by reference, so any changes you make only affect the copy of the variable in this function.

You need two local variables - one to accumulate the result, and one to track the factors you're multiplying into the result. You can use the input number as one of those (either one), so you only need one additional local. You could call that 'total' or 'result' and accumulate the value into that local, or you could call it 'factor' or something and use the input number to accumulate the value. Either choice is fine, though accumulating into a new variable like 'total' is probably more readable.

end

lesser_number = number - 1
(number - 2).times do

Choose a reason for hiding this comment

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

Good insight that you don't need to multiply by 1 at the end of the loop.

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