Birmingham | 25-ITP-Sept | Khor Biel | Sprint 2 | module-data-groups/sprint-2 #832
Conversation
…ing) with passing tests
wankoak
added
📅 Sprint 2
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Groups/blob/general-review-feedback/Sprint-2/feedback.md
Doing so can help reviewers speed up the review process. Thanks.
Why not practice committing files one by one? Making small commits has the following advantages:
- Clarity: Each commit tells a clear story (one feature, one fix, one change).
- Debugging: Easy to find and undo the commit that caused a bug.
- Collaboration: Teammates can review and understand changes faster.
- History: Project log becomes a readable timeline, not a messy dump.
- Safety: Progress is saved in safe, logical steps—less risk of losing work.
If you would like to practice committing files one by one in VSCode, you can select which file to stage and then commit only the staged file.
See: this video (at around the 12:50 mark, it shows how to stage a single file).
cjyuan
added
Reviewed
|
@cjyuan thanks a lot, I'll commit 1 by 1 next time. You didn't mention anything like fixing my code or any thing though and you didn't mark my PR complete which I assume there has to be something to fix |
|
Have you checked out the General feedback link I shared in my previous comment? |
|
@cjyuan yes i did and i found more helpful information, thanks |
wankoak
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Code looks great. Changes are optional.
If you would like me to review any new change, just comment or tag me on this PR.
| for (const pair of countryCurrencyPairs) { | ||
| const countryCode = pair[0]; | ||
| const currencyCode = pair[1]; |
There was a problem hiding this comment.
Could also use array destructuring to shorten the code on lines 4-6 (if you have time).
|
|
||
| if (!queryString || queryString.length === 0) { | ||
| return queryParams; | ||
| } | ||
|
|
||
| // Remove leading '?' if present | ||
| if (queryString.startsWith("?")) { | ||
| queryString = queryString.substring(1); | ||
| } | ||
|
|
||
| const keyValuePairs = queryString.split("&"); | ||
|
|
||
| for (const pair of keyValuePairs) { | ||
| const [key, value] = pair.split("="); | ||
| queryParams[key] = value; | ||
| // Only split on the first '=' to handle values containing '=' | ||
| const equalsIndex = pair.indexOf("="); | ||
| if (equalsIndex === -1) { | ||
| // Handle keys without values | ||
| const key = decodeURIComponent(pair); | ||
| queryParams[key] = ""; | ||
| } else { | ||
| const key = decodeURIComponent(pair.substring(0, equalsIndex)); | ||
| const value = decodeURIComponent(pair.substring(equalsIndex + 1)); | ||
| queryParams[key] = value; | ||
| } |
There was a problem hiding this comment.
This work great!
In practice, you can consider using a JS built-in function to parse query string.
| // Use Object.create(null) to avoid inherited properties | ||
| const wordCounts = Object.create(null); | ||
|
|
||
| if (!str || str.trim() === "") { |
There was a problem hiding this comment.
Between (typeof str != "string" || str.trim() === "") and your current condition, which one do you think is better, and why?
cjyuan
added
Complete
| // Split into words and handle punctuation/case | ||
| const words = str.split(/\s+/); | ||
|
|
||
| for (const word of words) { | ||
| // Remove punctuation and convert to lowercase | ||
| const cleanWord = word | ||
| .toLowerCase() | ||
| .replace(/[.,!?]/g, "") | ||
| .trim(); | ||
|
|
||
| // Skip empty strings | ||
| if (cleanWord === "") continue; | ||
|
|
||
| // Count the word | ||
| if (wordCounts[cleanWord]) { | ||
| wordCounts[cleanWord] += 1; | ||
| } else { | ||
| wordCounts[cleanWord] = 1; | ||
| } | ||
| } | ||
|
|
||
| return wordCounts; |
There was a problem hiding this comment.
If you do some pre-processing on str before line 41, you could simplify the code within the loop by about 50% (or more).
3 participants
Self checklist
Changelist
Completed Sprint 2 Implement tasks (contains, lookup, tally, querystring) with passing tests
No question