Skip to content

London | 25-SDC-NOV | Jesus del Moral | Sprint 2 | Improve with Precomputing#139

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

London | 25-SDC-NOV | Jesus del Moral | Sprint 2 | Improve with Precomputing#139
delmorallopez wants to merge 2 commits intoCodeYourFuture:mainfrom
delmorallopez:improve-with-precomputing

Conversation

@delmorallopez
Copy link

Common prefix

Count letters

@delmorallopez delmorallopez added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 1, 2026
Copy link

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Can you use complexity to explain how the new implementation is better than the original implementation?

Comment on lines +23 to +24
@lru_cache(maxsize=None)
def cached_find_common_prefix(left: str, right: str) -> str:
Copy link

Choose a reason for hiding this comment

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

Can you justify the use of @lru_cache? Do you expect it to always improve the performance?

Copy link
Author

Choose a reason for hiding this comment

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

In the original version, we compare every string with every other string. If there are a lot of strings, the number of comparisons grows very quickly because it’s roughly proportional to the square of the number of strings. So doubling the number of strings makes the work grow by about four times.

In the new version, we first sort the strings. Sorting does add some work, but after sorting we only compare neighbouring strings instead of every possible pair. That reduces the number of prefix comparisons dramatically, from “compare everything with everything” to just “compare each string with the next one”.

As a result, the work now grows much more slowly as the number of strings increases. For large inputs, this makes a very noticeable difference in performance compared to the original approach.

So overall, the new implementation scales much better as the list gets bigger.

In this version, @lru_cache stores the result of cached_find_common_prefix(left, right) so that if the same pair of strings is compared again, we can return the result immediately instead of recomputing it.
In this specific implementation, we only compare each adjacent pair once after sorting. That means the same pair of strings is not normally recomputed. So in practice, the cache is unlikely to provide much benefit here.
So I have removed it

Copy link

Choose a reason for hiding this comment

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

Whether lru_cache can help or not really depend on the characteristics of the strings we are comparing. Removing it is optional; it's more important to understand its pros and cons, which you did. Good job!

Copy link

Choose a reason for hiding this comment

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

Very clever approach.

@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 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