Glasgow| 26-ITP-May| Alasdair MacDonald| Sprint 2 | Exercises#1213
Glasgow| 26-ITP-May| Alasdair MacDonald| Sprint 2 | Exercises#1213MacDonald91 wants to merge 3 commits into
Conversation
MacDonald91
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
To provide fuller context for this code review, can you include the link to your previous reviewed PR in the PR description?
cjyuan
added
Reviewed
| // Then it should return false | ||
| test("returns false when passed invalid parameters like an array", () => { | ||
| expect(contains([1, 2, 3], "a")).toBe(false); | ||
| }); No newline at end of file |
There was a problem hiding this comment.
Your function implementation is correct, but this test is not properly prepared and may fail to detect error when the implementation is modified in the future.
This test cannot yet confirm that the function correctly returns false when the first argument is an array.
This is because contains([1, 2, 3], "a") could also return false simply because "a" is not a key of the array.
Arrays are objects, with their indices acting as keys. A proper test should use a valid
key to ensure the function returns false specifically because the input is an array, not because the key is missing.
| const result = {}; | ||
|
|
||
| for (const item of items) { | ||
|
|
||
| if (result[item]) { | ||
| result[item] += 1; | ||
| } else { | ||
| result[item] = 1; | ||
| } | ||
|
|
||
| } |
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()
2 participants
Learners, PR Template
Self checklist
Changelist
resubmission
Questions
n/aq