Skip to content

Comments

Sheffield | 25-SDC-Nov | Sheida Shabankari | Sprint 2 |Improve code with precomputing#113

Open
sheida-shab wants to merge 4 commits intoCodeYourFuture:mainfrom
sheida-shab:Feat-improve-with-precomputing
Open

Sheffield | 25-SDC-Nov | Sheida Shabankari | Sprint 2 |Improve code with precomputing#113
sheida-shab wants to merge 4 commits intoCodeYourFuture:mainfrom
sheida-shab:Feat-improve-with-precomputing

Conversation

@sheida-shab
Copy link

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

PR summary :

  • Improved find_longest_common_prefix using two techniques:
  1. Precomputing prefix hashes for each string to avoid repeated character-by-character comparisons.
  2. Sorting the strings so that only adjacent strings need to be compared, which greatly reduces the number of comparisons for large lists.
    These changes make the function much faster while keeping all existing tests passing, including the large list test.
  • Improved count_letters by precomputing lowercase letters for faster lookup. Only uppercase letters without a lowercase version are counted. All tests, including long strings, pass.

@sheida-shab sheida-shab added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Complexity The name of the module. labels Feb 18, 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 why your implementation is an improvement?

Comment on lines 18 to 19
# Precompute prefix hashes for each string to speed up comparisons
prefix_map={s:[hash(s[:i+1]) for i in range(len(s))] for s in strings}
Copy link

Choose a reason for hiding this comment

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

How do these "prefix hashes" speed up comparison?

Copy link
Author

Choose a reason for hiding this comment

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

At first, I thought the exercise needed me to precompute and keep some data for each string. So I used prefix hashes, thinking this would avoid comparing letters one by one and make the function much faster.
But after testing, I saw that using hashes did not make it much faster. Sorting the strings and comparing only neighbors worked better, was simpler, and gave the speed improvement needed.

Copy link

@cjyuan cjyuan Feb 20, 2026

Choose a reason for hiding this comment

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

Note: With the way you used prefix hashes, the complexity actually became higher (when the length of each string is large).

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense. In my case, prefix hashing introduced additional preprocessing and memory costs without improving the asymptotic complexity. Once I removed it and only compared adjacent strings after sorting, the solution became simpler and faster, especially for longer inputs.

@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 Feb 19, 2026
@sheida-shab
Copy link
Author

Hi CJ,Thanks for your feedback.
I improved count_letters by reducing time complexity from O(n²) to O(n).

Previously, for each character in the string (O(n)), the code performed a membership check on the entire string (O(n)), resulting in O(n²) complexity.

By precomputing a set of all lowercase characters once (O(n)) and using O(1) set lookups inside the loop, the overall complexity is reduced to O(n) .

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

cjyuan commented Feb 20, 2026

Can you also do a complexity analysis for find_longest_common_prefix?

@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 Feb 20, 2026
@sheida-shab
Copy link
Author

The time complexity of find_longest_common_prefix now is O(n log n + n · m), where n is the number of strings and m is the average length of a string.
Sorting the strings takes O(n log n), and then comparing only adjacent strings takes O(n · m) in the worst case.

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

cjyuan commented Feb 20, 2026

How does the program determine the order between two strings? Is the performance affected by string length when we compare two strings? If we take into account m, then the sorting complexity is no longer just O(nlogn).

@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 Feb 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Module-Complexity The name of the module. Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants