London | 26-ITP-May | Chinwe Chukwuma | Sprint 2 | Module-Data-Groups - #1353
London | 26-ITP-May | Chinwe Chukwuma | Sprint 2 | Module-Data-Groups#1353ChinweP wants to merge 20 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
There is another Sprint-2 folder inside the Sprint-2 folder. I am not sure which set of files are the ones to be reviewed. Could you update this PR branch accordingly? |
|
@cjyuan, apologies for the inconvenience. I've removed the unnecessary Sprint-2. |
cjyuan
left a comment
There was a problem hiding this comment.
You missed updating a few files in the implement sub-folder.
| // Given an object with properties | ||
| // When passed to contains with an existing property name | ||
| // Then it should return true | ||
| test("returns true when object contains the property", () => { | ||
| const obj = { a: 1, b: 2 }; | ||
| expect(contains(obj, "a")).toBe(true); | ||
| }); | ||
|
|
||
| // Given an object with properties | ||
| // When passed to contains with a non-existent property name | ||
| // Then it should return false | ||
| test("returns false when object does not contain the property", () => { | ||
| const obj = { a: 1, b: 2 }; | ||
| expect(contains(obj, "c")).toBe(false); | ||
| }); |
There was a problem hiding this comment.
How are these two tests different from the tests on lines 19-28?
| // When passed to contains | ||
| // Then it should return false or throw an error | ||
| test("returns false for invalid parameters like an array", () => { | ||
| expect(contains([], "a")).toBe(false); |
There was a problem hiding this comment.
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.
I have removed the duplicate tests and changed the array test to use a non-empty array with a valid key.
|
I've now updated the invalid parameter test. Thanks for pointing this out. |
|
Hi, I've now updated the remaining files in the implement folder and all tests pass. |
| } | ||
|
|
||
| if (!pair.includes("=")) { | ||
| queryParams[pair] = ""; |
There was a problem hiding this comment.
pair could also contain percent-encoded characters.
| const counts = {}; | ||
|
|
||
| for (const item of items) { | ||
| if (counts[item] === undefined) { |
There was a problem hiding this comment.
Does the following function call returns the value you expect?
tally(["toString", "toString"]);
Suggestion:
- Look up an approach to create an empty object with no inherited properties, or
- use
Object.hasOwn()
…d test for 'toString' items
|
I've addressed both comments. I updated the querystring parser to decode percent-encoded keys without values and added a test for this case. I also updated tally to use Object.hasOwn() and added a test for "toString" |
|
Changes look good. Well done. |
|
Thank you @cjyuan for your time and for reviewing my work. |

Learners, PR Template
Self checklist
Changelist
Sprint 2 completed