Ports - Margaret#14
Conversation
mmcknett
left a comment
mmcknett
left a comment
There was a problem hiding this comment.
Good job! This solution looks great overall, minus some minor nits.
Checklist
- Clean, working code
- Works just fine!
- The minor formatting issues and the variable naming are detracting from the cleanliness of this solution.
- Efficient code - give feedback as you would give to a peer on your team
- A detailed explanation of time and space complexity (explains what n stands for, explains why it would be a specific complexity, etc.)
- Be sure to briefly explain your time and space complexity analyses.
- All test cases for the assignment should be passing.
| # Time complexity: ? | ||
| # Space complexity: ? | ||
| # Time complexity: Linear or O(n) where n is the number input | ||
| # Space complexity: Constant or O(1), will have the same number of variables no matter the size of the input |
There was a problem hiding this comment.
I like this concise explanation for the space complexity. :)
|
|
||
| def factorial(number) | ||
| raise NotImplementedError | ||
| return 1 if number == 0 |
There was a problem hiding this comment.
[nit] You should definitely indent all the lines between def factorial(number) and the final end for readability.
| end | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
[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.
| raise ArgumentError, "Enter an integer" | ||
| end | ||
| a = 1 | ||
| b = number |
There was a problem hiding this comment.
[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?
| a = 1 | ||
| b = number | ||
| while a < number | ||
| b = b * (number - a) |
There was a problem hiding this comment.
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.
| # 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 |
2 participants
No description provided.