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 2 commits into
Conversation
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.
| for (let index = 0; index < Object.keys(object).length; index++) { | ||
| const element = Object.keys(object)[index]; | ||
| if (element === target) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Code is correct.
Could also consider taking advantage of built-in Array or Object methods to achieve the same behavior with less code.
There was a problem hiding this comment.
All the tests in this file have the same description.
If they belong to the same category, you could group the tests into one cateogry.
Otherwise, it's better to give them distinct test descriptions.
| // 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(/=(.*)/); |
| function tally() {} | ||
|
|
||
| function tally(array) { | ||
| let tallyObject = {}; |
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()
| if (Array.isArray(array)) { | ||
| for (const item of array) { | ||
| if (tallyObject[item] === undefined) { | ||
| tallyObject[item] = 1; | ||
| } else { | ||
| tallyObject[item]++; | ||
| } | ||
| } | ||
| return tallyObject; | ||
| } else { | ||
| throw new Error("Invalid input"); | ||
| } |
There was a problem hiding this comment.
Could consider structuring the code this way:
if (not an array)
throw error;
// code to deal with normal case (no need else)
...
``
Learners, PR Template
Self checklist