Conversation
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
1 similar comment
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Great work on both tasks, Alaa! You've successfully applied precomputing to improve the performance of both functions. Here's a summary of the review:
What you did
You tackled two separate functions and improved both:
-
count_letters: You precomputed two sets (lower_setandupper_set) before doing any lookups. This is a classic precomputing pattern — building a lookup structure once at O(n) cost, then querying it at O(1) per operation. The overall complexity drops from O(n²) to O(n). You also quietly fixed a latent bug: the oldis_upper_casehelper returnedTruefor digits and spaces (since'1'.upper() == '1'), whereaschar.isupper()correctly identifies only genuine uppercase letters. -
common_prefix: You sort the strings first and then only compare adjacent pairs. This works because of a useful property of lexicographic sorting: if two strings share a prefix P, any string that falls between them in sorted order must also start with P — so the pair with the longest common prefix will always be adjacent. This reduces comparisons from O(n²) to O(n log n).
What you did well
- The precomputing pattern in
count_lettersis clean and idiomatic — using set comprehensions is exactly the right tool here. - The sorting insight in
common_prefixis clever and mathematically sound. - Clear, helpful comments explain the reasoning behind both approaches.
- You kept the public function signatures compatible and didn't break any existing tests.
Minor things to consider (not blockers)
- In
count_letters, you make two passes overtext(one for lowercase, one for uppercase). You could do it in one pass by building both sets together. Still O(n), just a small efficiency note. - In
common_prefix, the loopfor index in range(len(sorted_strings) - 1)is correct, but a slightly more Pythonic alternative isfor a, b in zip(sorted_strings, sorted_strings[1:]), which avoids index arithmetic.
These are optional improvements — the code as written is correct and clear.
VERDICT: completed
| """ | ||
| count_letters returns the number of letters which only occur in upper case in the passed string. | ||
| """ | ||
| only_upper = set() |
There was a problem hiding this comment.
This is the key precomputing step. By building lower_set once here (O(n)), you turn every subsequent membership check into O(1) instead of O(n). The original code did letter.lower() not in s where s is a plain string — Python's in operator on strings is a substring search and costs O(n) per call, making the old function O(n²) overall. Your approach is a textbook example of precomputing a lookup structure.
There was a problem hiding this comment.
So, what is a better way to write it?
github-actions
bot
added
completed
1 participant
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.