Add catalan_numbers.py #4455
Add catalan_numbers.py #4455
Conversation
ghost
added
awaiting reviews
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
Kommandat
force-pushed
the
lakshay/add_catalan_numbers
branch
from
c018648 to
f071d62
Compare
ghost
removed
the
require type hints
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
Kommandat
force-pushed
the
lakshay/add_catalan_numbers
branch
from
f071d62 to
cf9a272
Compare
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
|
|
||
|
|
||
| class Catalan: | ||
| def __init__(self, N: int) -> None: |
There was a problem hiding this comment.
Please provide descriptive name for the parameter: N
There was a problem hiding this comment.
I added a comment explaining N's function below this line and I don't think a more descriptive variable name is necessary
Kommandat
force-pushed
the
lakshay/add_catalan_numbers
branch
from
cf9a272 to
c694ed1
Compare
ghost
removed
the
require descriptive names
|
I don't like the unneeded OOP. |
There was a problem hiding this comment.
Hello. Thanks for your contribution. I performed a review. If you disagree with the changes I suggested, feel free to explain your disagreement by commenting on the review.
Mostly, I suggested changes to avoid unwanted side-effects. These changes are based on the following contribution guideline:
have minimal side effects (Ex. print(), plot(), read(), write()).
Also, when the user enters erroneous input, an exception should be raised, instead of it simply being printed (this applies only to the algorithm itself, not in the if __name__ == "__main__" block).
raise Python exceptions (ValueError, etc.) on erroneous input values
| ) | ||
| elif upper_limit == 0: | ||
| self.catalan_numbers[0] = 1 | ||
| print(self.catalan_numbers) |
There was a problem hiding this comment.
I would avoid print statements in the algorithm itself as they produce unwanted side-effects.
| """ | ||
| if sequence_no is not None: | ||
| if sequence_no < len(self.catalan_numbers): | ||
| return print(self.catalan_numbers[: sequence_no + 1]) |
There was a problem hiding this comment.
This should return the value directly for the same reason described above. This is where the algorithm produces its output, so the output should be directly returned.
| if sequence_no < len(self.catalan_numbers): | ||
| return print(self.catalan_numbers[: sequence_no + 1]) | ||
| else: | ||
| print("Out of bound.") |
There was a problem hiding this comment.
This is an error, so an exception should be raised here.
| else: | ||
| print("Out of bound.") | ||
| else: | ||
| print("Please specify a value") |
There was a problem hiding this comment.
Again, this should be refactored as an exception.
ghost
added
awaiting changes
ghost
added
require tests
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
ghost
removed
require tests
Kommandat
force-pushed
the
lakshay/add_catalan_numbers
branch
3 times, most recently
from
7e15dd5 to
bb62754
Compare
Kommandat
force-pushed
the
lakshay/add_catalan_numbers
branch
from
bb62754 to
7f5578d
Compare
@mrmaxguns Thanks for the review! I agree with all your comments and made the changes you recommended. I requested a re-review, please take another look when you get a chance. As an aside, I based much of my code on the Fibonacci algorithm in the same folder thinking an existing algorithm already followed the guidelines but I was mistaken. It would nice to add some examples to the contribution guidelines that demonstrate best practices so others don't do what I did. Once again, thanks! |
There was a problem hiding this comment.
Thank you for your contribution! Yes, indeed, some code in this repository does not follow the contribution guidelines, as they may have changed or the code was improperly reviewed. If you notice such code, feel free to open a pull request addressing the changes.
Regarding the code, it looks good. Thanks for applying the suggestions! This should be merged once tests are run and I double check the code for any issues I might not have caught now.
mrmaxguns
added
awaiting merge
|
Thank you for your contribution! 💯 |
Reviewed by @mrmaxguns. This is an implementation of Catalan Numbers.
Describe your change:
Checklist:
Fixes: #{$ISSUE_NO}.Additional Notes
Excited to make my first contribution to The Algorithms! Catalan numbers are a sequence in mathematics that represent answers to many combinatorics problems. They can be calculated using a recurrence relation and we can use tabulation (similarly to how we can for the Fibonacci sequence). I thought this best fit in the "dynamic_programming" folder given we're using tabulation to compute the sequence.
All work is my own, but I did take a look at the existing C++ Catalan number algorithm and Python Fibonacci algorithm for inspiration and best practices. If this is approved, I hope to contribute to Algorithms-Explanation as well.
Please let me know if you have any feedback!
Sources:
[1] https://brilliant.org/wiki/catalan-numbers/
[2] https://en.wikipedia.org/wiki/Catalan_number