-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Added Bernoulli sample #3554
Added Bernoulli sample #3554
Conversation
Hello @MarquisdeGeek, thank you for your contribution. In order to solve the formating error reported by Travis, can you reformat your code and push it again? Simply run See more details in CONTRIBUTING.md and the docs |
end | ||
|
||
|
||
def calculateBernoulli(bern) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crystal uses snake_case
convention, so calculate_bernoulli
would fit better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -67,7 +60,6 @@ end | |||
|
|||
# Int32's are only big enough to calculate to 17 Bernoulli numbers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use Int64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hadn't seen the syntax for Int64 literals. Now found and fixed.
end | ||
|
||
def reduce | ||
gcd = gcd(@numerator, @denominator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same variable name and method name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What alternative name would you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think it's fine, its clear what's a variable and what's a method call because its not an argless call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there's already Int#gcd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RX14 is probably correct. What is 'best practices' for Crystal? If avoiding the same name for a method and a local variable is desireable [my preference] then, maybe use something like 'div', 'divisor', 'deno', or 'denominator' for the variable since it is internal to the 'Fraction#reduce' method? The '#gcd' method has different params than 'Int#gcd', but the purpose is basically the same, so the method name is probably fine as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asterite, were you suggesting that the method name is fine or were you suggesting a conflict w/ Int#gcd
? What is your recommendation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No conflict, bu we already have a gcd method for the
Int` type, that you could use, instead of reimplementing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep using the alternative gcd impl for examples sake. Make a note that Int#gcd
This is not a good example of Crystal code because it does not properly use Crystal idioms such as |
Closing because even though the sample is nice, it doesn't demonstrate any Crystal feature in particular. |
No description provided.