London | 25-SDC-July | Andrei Filippov | Sprint 2 | Improve code with caches#30
London | 25-SDC-July | Andrei Filippov | Sprint 2 | Improve code with caches#30Droid-An wants to merge 5 commits intoCodeYourFuture:mainfrom
Conversation
Droid-An
added
the
Needs Review
OracPrime
added
the
Review in progress
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
Fibonacci is fine, but coins needs a little more thought
| if n not in cache: | ||
| cache[n] = fibonacci(n - 1) + fibonacci(n - 2) | ||
| return cache[n] | ||
| else: |
There was a problem hiding this comment.
It's personal taste as to which way you do it, but the code would be more concise without lines 9 and 10, and with 11 outside the if.
| else: | ||
| intermediate = ways_to_make_change_helper(total - total_from_coins, coins=coins[coin_index+1:]) | ||
| key = (total - total_from_coins, tuple(coins[coin_index + 1 :])) | ||
| if key not in cache: |
There was a problem hiding this comment.
converting the list to a tuple every time is going to be expensive. Also at each stage you're creating a new list, which will also be expensive. The new lists are always the same list, but starting later. Can you think of a way you could refactor the code so that it always uses the same tuple, and the recursion is achieved by passing a parameter indicating where the first usable entry in the tuple is? In that situation, how much can you simplify the key to the cache as well?
There was a problem hiding this comment.
yes, I can do this. In that case I only need to store the amount and index where to start as key
OracPrime
added
Reviewed
Droid-An
added
Needs Review
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
Good corrections (not least because the result is not just faster, but less code!)
OracPrime
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Improved code by adding cache caches
Questions
no questions