London | 26-ATP-Jan | Boualem Larbi Djebbour | Sprint 2 | coursework#1188
London | 26-ATP-Jan | Boualem Larbi Djebbour | Sprint 2 | coursework#1188djebsoft wants to merge 13 commits intoCodeYourFuture:mainfrom
Conversation
…perty, and false otherwise
This comment has been minimized.
This comment has been minimized.
djebsoft
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
djebsoft
changed the title
djebsoft
added
the
Needs Review
| // Given invalid parameters like an array | ||
| // When passed to contains | ||
| // Then it should return false or throw an error | ||
| test("given invalid parameters, returns false or throws an error", () => { | ||
| expect(contains(["gitName", "position"], "gitName")).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
This test does not yet confirm that the function correctly returns false when the first argument is an array.
This is because contains(["gitName", "position"], "gitName") could also return false simply because "gitName" 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.
After you fixed this test, make sure you also run the test to check your function.
Note: When testing invalid type of data, undefined and null are usually good candidates to test -- many functions fail because they could not handle undefined or null.
There was a problem hiding this comment.
thank you for the feedback
I just realised that the false returned cases should be the exception, so I changed the if condition to the other way around.
I removed the case of empty object (object length equals to zero) as it does not add any plus to the code.
I also added the case where the input is an array to return false.
and run the test to check the function.
added a test with null input for invalid type of data.
There was a problem hiding this comment.
Why not also update the test (the one that tests array)?
Your function is correct, but 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.
There was a problem hiding this comment.
thanks for the spot
sorry that I got confused about this point
i think the perfect example test is to test tricky arguments where the first argument looks like an object by having two elements and the second argument is string that match one the elements in the first argument
therefore, if the first argument wasn't an array than the test should return true. but because it an array, the function returns false as invalid input.
could you enlighten me about what I'm mistaken here ?
Sprint-2/implement/lookup.js
Outdated
| // the keys of the object should be the first element of each inner array (country code) | ||
| // the values of the object should be the second element of each inner array (currency code) | ||
|
|
||
| function Lookup(coCuPairs) { |
There was a problem hiding this comment.
-
In JS naming convention, both function and variable names usually begin with a lowercase letter (i.e. camelCase). Names starting with an uppercase letter are used for built-in and custom data types (e.g.,
Math). -
countryCurrencyPairsis probably more readable thancoCuPairs.
There was a problem hiding this comment.
thank you for the suggestion
i changed the parameter name to countrycurrencyPairs.
There was a problem hiding this comment.
What about the function name?
cjyuan
added
Reviewed
djebsoft
added
Needs Review
cjyuan
removed
the
Needs Review
djebsoft
added
Needs Review
2 participants
Learners, PR Template
Self checklist
Changelist
completed sprint-2 exercies in data groups module.
Questions