London | 26-ITP-May | Rizqah Popoola | Sprint 2 | Data groups - #1323
London | 26-ITP-May | Rizqah Popoola | Sprint 2 | Data groups#1323risikatpopoola wants to merge 6 commits into
Conversation
risikatpopoola
added
🏕 Priority Mandatory
cjyuan
added
Reviewed
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Code is pretty solid.
Note: After our code is working correctly, its a professional practice to change all variable declarations from let to const if they are not going to be reassigned a value.
| // Given invalid parameters like an array | ||
| // When passed to contains | ||
| // Then it should return false or throw an error | ||
| test("returns true if the object contains the property, false otherwise", () => { | ||
| const currentOutput = contains(["a", "b", "c", "e"], "e"); | ||
| const targetOutput = false; | ||
|
|
||
| expect(currentOutput).toEqual(targetOutput); | ||
| }); |
There was a problem hiding this comment.
This test does not quite match the spec given on lines 52-54.
Please note that in JS, an array is a kind of object with its indices serve as its keys.
| const [key, value] = pair.split("="); | ||
| queryParams[key] = value; | ||
| if (pair === "") continue; | ||
| const [key = "", value = ""] = pair.split(/=(.*)/); |
risikatpopoola
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good. Just one more change needed.
| const currentOutput = contains(["a", "b", "c", "e"], "e"); | ||
| const targetOutput = 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. "e" is not a key of the array you are testing, but "1" is.
There was a problem hiding this comment.
That makes sense, i wasn't checking for the key before, i have now fixed this, thank you.
cjyuan
added
Reviewed
risikatpopoola
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good.
In the invalid case, you could also test other types of invalid input.
cjyuan
added
Complete
2 participants
Learners, PR Template
Self checklist