London | 26-ITP-May | Alex Jamshidi | Sprint 2 | Exercises#1226
London | 26-ITP-May | Alex Jamshidi | Sprint 2 | Exercises#1226Alex-Jamshidi wants to merge 16 commits into
Conversation
Alex-Jamshidi
added
📅 Sprint 2
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Some of the code is not consistently formatted or indented.
Have you installed the prettier VSCode extension and enabled "Format on save/paste" on VSCode, as recommended in
https://github.com/CodeYourFuture/Module-Structuring-and-Testing-Data/blob/main/readme.md
?
| it("given invalid parameter (an array) returns false or throws an error", () => { | ||
| const object = []; | ||
| const property = []; | ||
| expect(contains(object, property)).toEqual(false) |
There was a problem hiding this comment.
Your function can correctly return false when the first argument is an array.
However, we write tests not only to verify our current implementation, but also to ensure that future changes do not alter the function's expected behavior.
The current test cannot yet confirm the function can correctly return false when the first argument is an array
because contains([], []) could also return false simply because [] is not a key of the array.
Arrays are objects, with their indices acting as keys. A proper test should use a non-empty array along with a valid key to ensure the function returns false specifically because the input is an array, not because the key is missing.
There was a problem hiding this comment.
Thanks for this, I understand.
I wrote several tests to cover all options for arrays (filled or empty).
cjyuan
added
Reviewed
| expect(contains([], [])).toEqual(false); | ||
| expect(contains(["a", 1], "a")).toEqual(false); | ||
| expect(contains({ a: 1, b: 2 }, ["a"])).toEqual(false); | ||
| expect(contains(["a", 1], ["a"])).toEqual(false); |
There was a problem hiding this comment.
"a" and ["a"] are not keys of ["a", "1"], but "0" and "1" are. You should use either "0" or "1" as key to ensure the function returns false because the first parameter is an array.
There was a problem hiding this comment.
updated and rerun tests to pass
| } | ||
|
|
||
| // Adds percentage-encoded characters | ||
| queryString = decodeURIComponent(queryString); |
There was a problem hiding this comment.
If you decode the characters so early and some the decoded characters are '=', '+', or '&', the query will be incorrectly parsed.
There was a problem hiding this comment.
updated this to resolve decoded chars at the end
refactored code to make more clear
|
Don't forget to add the "Needs Review" label back whenever your PR is ready to be re-reviewed. Your other changes look good and thanks for responding to each comment. |
Alex-Jamshidi
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good. Well done!
cjyuan
removed
the
Needs Review
cjyuan
added
the
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Completed exercises for Sprint 2