Skip to content

Conversation

@saff-coder
Copy link

Learners, PR Template

Self checklist

  • 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

Changelist

I have implemented the required functions, and run the required tests

Questions

No questions, thank you

@saff-coder saff-coder added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 19, 2025
Copy link
Contributor

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

@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 Nov 21, 2025
Now the function can decode a URL-encoded string in JavaScript
Now the function can test if the array is empty
@saff-coder
Copy link
Author

I have updated all the codes

@saff-coder saff-coder 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 Nov 22, 2025
Comment on lines 15 to 18
${recipe.ingredients[0]}
${recipe.ingredients[1]}
${recipe.ingredients[2]}
${recipe.ingredients[3]}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you figure out an approach that could work for any number of ingredients?

Comment on lines 47 to 48
test("contains on invalid parameters returns false", () => {
expect(contains([], "a")).toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Which of the parameters on line 48 is "invalid"?

Copy link
Author

Choose a reason for hiding this comment

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

You’re right — in that test, neither parameter is actually invalid.
[ ] is still an object in JavaScript, and "a" is a valid key, so the test title didn’t match the behaviour.
I’ve now updated the test to use invalid parameters (such as strings, numbers, null, and undefined)

Comment on lines 4 to 6
for (const pair of pairs) {
const country = pair[0];// first element is country code
const currency = pair[1];// second element is currency code
Copy link
Contributor

Choose a reason for hiding this comment

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

Could consider using array destructuring syntax to simplify the code on lines 4-6.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback!
I’ve updated the loop to use array destructuring so the code is cleaner and easier to read.

Comment on lines 1 to 28
function parseQueryString(query) {
const result = {};
if (!query) return result;

const pairs = query.split("&");

for (let pair of pairs) {
const index = pair.indexOf("=");

// Key has no "=" → value is empty string
if (index === -1) {
const key = decodeURIComponent(pair);
result[key] = "";
continue;
}

// Key is before "=", value is EVERYTHING after "="
const rawKey = pair.slice(0, index);
const rawValue = pair.slice(index + 1);

for (const pair of keyValuePairs) {
const [key, value] = pair.split("=");
queryParams[key] = value;
const key = decodeURIComponent(rawKey);
const value = decodeURIComponent(rawValue);

result[key] = value;
}

return queryParams;
return result;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This work well.
Could also consider explore and exploit a built-in function to parse query string.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the suggestion!
I have now explored the built-in URLSearchParams

Comment on lines +35 to +41
test("counts how many times each item appears", () => {
expect(tally(["a", "b", "a", "c", "b", "a"])).toEqual({
a: 3,
b: 2,
c: 1,
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Does tally(["constructor", "constructor"]) return what you expect?

Copy link
Author

Choose a reason for hiding this comment

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

thank you!
tally(["constructor", "constructor"]) didn’t work properly because I was using a normal {} object, so "constructor" clashed with the built-in constructor property.
I used Object.create(null) so "constructor" behaves like normal keys and doesn’t clash with JavaScript’s internal properties.

@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 Nov 22, 2025
@saff-coder saff-coder 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 Nov 23, 2025
Copy link
Contributor

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

Excellent work! You clearly know what the code does.

And thanks for the detailed responses.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 23, 2025
@cjyuan cjyuan added the Complete Volunteer to add when work is complete and all review comments have been addressed. label Nov 23, 2025
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