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

Page 460: In exercise 13-5, the TODO comment for fib_sum refers to the wrong thing. #107

Closed
WraithGlade opened this issue Jan 12, 2020 · 2 comments

Comments

@WraithGlade
Copy link

@WraithGlade WraithGlade commented Jan 12, 2020

The following example code in exercise 13-5 refers to the wrong thing:

long fib_sum(size_t n) { 
    // TODO: Adapt code from Exercise 12.1
    return 0; 
}

Here is the actual contents of Exercise 12.1:

12-1. Reimplement the narrow_cast in Listing 6-6 to return a std::optional. If the cast would result in a narrowing conversion, return an empty optional rather than throwing an exception. Write a unit test that ensures your solution works.

Perhaps you instead intended to refer to the recursive version of the Fibonacci algorithm, considering that its naive implementation performance is terrible and easily noticed. Or perhaps you are only intending for the summing of the Fibonacci sequence itself to be the source of the performance overhead. The clarity here could be improved.

...

Also, why are you passing by const reference in the following example code, from that same example code block?:

long cached_fib_sum(const size_t& n) { 
    static std:: map <long, long> cache; 
    //TODO: Implement me 
    return 0; 
}

There doesn't seem to be a point to doing that, although it doesn't really harm anything either in this case.

@WraithGlade

This comment has been minimized.

Copy link
Author

@WraithGlade WraithGlade commented Jan 13, 2020

On further thought, I estimate that you were not referring to the recursive version of the Fibonacci sum, considering that the iterative version is already plenty slow when you sum it together as in this exercise.

@JLospinoso

This comment has been minimized.

Copy link
Owner

@JLospinoso JLospinoso commented Jan 21, 2020

Thanks, @WraithGlade!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.