Manchester | 26-ITP-Jan | Mehroz Munir | Sprint 2 | Coursework#1051
Manchester | 26-ITP-Jan | Mehroz Munir | Sprint 2 | Coursework#1051MehrozMunir wants to merge 5 commits intoCodeYourFuture:mainfrom
Conversation
MehrozMunir
added
📅 Sprint 2
Sprint-2/implement/lookup.test.js
Outdated
| // CASE 2: Should return an empty object if empty array is passed | ||
| test(`should return an empty object if the array is empty`, () => | ||
| expect(createLookup([[]])).toEqual({})); |
There was a problem hiding this comment.
The first argument is not quite an empty array; it is "an array containing an empty array".
For this input, your function actually returns { undefined: undefined } instead of {}, but toEqual() consider them equal because it ignores property named "undefined". A stricter comparison .toStrictEqual() would have flagged them as an error.
There was a problem hiding this comment.
Thanks.
Yes that is absolutely right. Do you think that I should always use toStrictEqual in all the test cases?
There was a problem hiding this comment.
Yes. I think .toStrictEqual() is better when comparing objects.
cjyuan
added
Reviewed
|
@cjyuan |
MehrozMunir
added
the
Needs Review
| function getRecipeIngredients(recipeIngredients) { | ||
| let ingredients = ""; | ||
| for (const element of recipeIngredients) { | ||
| ingredients += element + "\n"; | ||
| } | ||
| return ingredients; | ||
| } |
There was a problem hiding this comment.
Note: This function can be replaced by a single function call to Array.prototype.join().
No change required but that array's method is worth exploring.
| // Then it should return false or throw an error | ||
| [ | ||
| { input: [1], property: "a" }, | ||
| { input: [1], property: "0" }, |
There was a problem hiding this comment.
My concern was just about the test.
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.
So after adding a test such as
{ input: [1], property: "0" },
you can help ensure future modification of the function can still correctly reject array.
| let currentNumberFrequency = frequencyMap.get(number); | ||
| if (currentNumberFrequency !== undefined) | ||
| frequencyMap.set(number, (currentNumberFrequency += 1)); | ||
| else frequencyMap.set(number, 1); |
There was a problem hiding this comment.
-
Could just use
+instead of+=on line 24. -
Why not use the original syntax to implement the code on lines 22-25?
frequencyMap.set(number, (frequencyMap.get(number) || 0) + 1);
| return maxFreq === 0 ? NaN : mode; | ||
| return maxKey; |
There was a problem hiding this comment.
The original code returns NaN instead of null when there is no mode.
| .replace(/\s+/g, " "); | ||
|
|
||
| cleanText.split(" ").forEach((word) => { |
There was a problem hiding this comment.
Note: We can also use regex with .split(), and combine the operation on line 43 and 45 as:
cleanText.split(/\s+/).forEach((word) => {
2 participants
No description provided.