-
Notifications
You must be signed in to change notification settings - Fork 2
fibonacci function first implementation (missing autotest code) #11
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
base: master
Are you sure you want to change the base?
Conversation
| if x < 1 || x > 45 { | ||
| // less than one is outside the domain of the mathematical function, | ||
| // and more than 45 overflows uint64 and results in infinite recursion | ||
| panic("fib() called with too small or large value!") |
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.
Panic will crash the program with a huge and ugly stack trace. It's never the best solution. If we want to quit the program with a fatal condition, log.Fatal is the way to go. Here, I think this should return an error that is then "bubbled up" to the caller and displayed to the user.
|
|
||
| ophandler{"fac", "Calculate factorial of x", 1, func(a []float64) ([]float64, int, error) { | ||
| x := uint64(a[0]) | ||
| if float64(x) != a[0] { |
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.
Instead of doing this maneuver with uint64 and float to make sure the number is an integer, maybe use if math.Floor(a) != a { ... } like elsewhere in the code. This avoids the confusing type casting.
| return []float64{float64(fact)}, 1, nil | ||
| }}, | ||
|
|
||
| ophandler{"fib", "Calculate fibonacci of x", 1, func(a []float64) ([]float64, int, error) { |
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 design fib() in a way that it can be called directly from the ophandler?
|
|
||
| ophandler{"fib", "Calculate fibonacci of x", 1, func(a []float64) ([]float64, int, error) { | ||
| x := a[0] | ||
| if x-float64(uint64(x)) != 0 { |
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.
Maybe use the same math.Floor approach here?
| if x-float64(uint64(x)) != 0 { | ||
| return nil, 1, errors.New("fibonacci requires an integer number") | ||
| } | ||
| if x < 1. { |
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.
You don't need a dot at the end in Go. if x < 1 works fine :)
| if x < 1. { | ||
| return nil, 1, errors.New("fibonacci requires a positive number") | ||
| } | ||
| if x > 45. { |
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 here (dot)
|
|
||
| ophandler{"fib", "Calculate fibonacci of x", 1, func(a []float64) ([]float64, int, error) { | ||
| x := a[0] | ||
| if x-float64(uint64(x)) != 0 { |
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.
With a bunch of ifs, we can use a switch. It's cleaner. Remember that in Go, a case is not restricted to one variable. You can do:
switch {
case any_condition:
(...)
case any_other_condition:
(...)
}
and so on.
For your appreciation. Will update with autotest code later.