Skip to content

London | 25-SDC-NOV | Jesus del Moral | Sprint 2 | Improve with Caches#138

Open
delmorallopez wants to merge 2 commits intoCodeYourFuture:mainfrom
delmorallopez:Sprint2-improve-with-caches
Open

London | 25-SDC-NOV | Jesus del Moral | Sprint 2 | Improve with Caches#138
delmorallopez wants to merge 2 commits intoCodeYourFuture:mainfrom
delmorallopez:Sprint2-improve-with-caches

Conversation

@delmorallopez
Copy link

Fibonacci

Making Change

@delmorallopez delmorallopez added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 1, 2026
Comment on lines +19 to +40
@@ -26,7 +33,13 @@ def ways_to_make_change_helper(total: int, coins: List[int]) -> int:
if total_from_coins == total:
ways += 1
else:
intermediate = ways_to_make_change_helper(total - total_from_coins, coins=coins[coin_index+1:])
intermediate = ways_to_make_change_helper(
total - total_from_coins,
coins=coins[coin_index + 1:],
cache=cache
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array and tuple creations are relatively costly operations.

From lines 9 and 38, we know coins can only be one of the following 9 arrays:

[200, 100, 50, 20, 10, 5, 2, 1]
[100, 50, 20, 10, 5, 2, 1]
[50, 20, 10, 5, 2, 1]
...
[1]
[]

We could further improve the performance if we can

  • avoid repeatedly creating the same sub-arrays at line 38 (e.g. use another cache), and
  • create key as (total, a_unique_integer_identifying_the_subarray) instead of as (total, tuple of coins)
    • There are only a small number of different subarrays. We can easily assign each subarray a unique integer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point @cjyuan

You’re absolutely right that repeatedly slicing the coins list and converting it to a tuple for the cache key introduces unnecessary overhead. Even though there are only 9 possible subarrays, my implementation was recreating them many times during recursion, which adds avoidable allocation cost.

I’ve refactored the solution to:

  • Keep a single global COINS list

  • Pass a coin_index instead of slicing the list

  • Use (total, coin_index) as the cache key instead of (total, tuple(coins))

Since the coin subarrays are deterministic (they’re just suffixes of the original list), the index uniquely identifies each one. This removes repeated list creation and tuple construction, and reduces the constant factor of the algorithm while keeping the same overall time complexity.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 2, 2026
@delmorallopez delmorallopez added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 2, 2026
@cjyuan
Copy link

cjyuan commented Mar 2, 2026

Changes look good.

Challenge: The code could be further optimized but it is not related to cache. There is a much quicker way to determine number of ways when there is only one coin to consider.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Mar 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants