CYF-ITP-South Africa | Natalie Patel | Module-Structuring-and-Testing-Data | Sprint 3 #229
Conversation
| else if (angle > 180 && angle < 360) { | ||
| return "Reflex angle"; | ||
| } | ||
| else if (angle <= 0 && angle >= 360) { |
There was a problem hiding this comment.
If angle is not any of the valid value, then it must be invalid. So an else would be suffice.
Sprint-3/implement/get-card-value.js
Outdated
| } | ||
|
|
||
| //Handling when card inputs are 2-9 | ||
| if (rank >= "2" && rank <= "9") { |
There was a problem hiding this comment.
Todo
Can you also check if getCardValue("23♠") or getCardValue("8ABC♠") are returning the value you expect?
|
|
||
| function isProperFraction (numerator, denominator) { | ||
| //Check if numerator OR denominator is 0, throw an error | ||
| if (numerator === 0 || denominator === 0) { |
There was a problem hiding this comment.
0 can be considered a fraction. So numerator should be allowed to be 0.
| let den = Math.abs(denominator); | ||
|
|
||
| if (num < den) { | ||
| return "True"; |
There was a problem hiding this comment.
Todo
Should return the Boolean value true or false instead of strings "True" or "False".
| if (num < den) { | ||
| return "True"; | ||
| } | ||
| else if (num > den) { |
There was a problem hiding this comment.
Assuming num and den are finite numbers.
If num is not less than den, then it would mean num is either greater than or equal to den. As a result, an else would be suffice.
| //This will isolate the last character and last two characters to handle what to add to the end of the returned string. | ||
| //1 will return 1st, 2 will return 2nd but 11, 12 and 13 with return 11th, 12th and 13th | ||
| const findLastCharacter = number % 10; //The remainder of the division is the last character | ||
| const findLastTwoCharacters = number % 100; //The remainder of the division are the last two characters |
There was a problem hiding this comment.
lastDigit and lastTwoDigits would be shorter.
| expect(getOrdinalNumber("23")).toBe("23rd"); | ||
| }); | ||
|
|
||
| test("should return a string ending in 'th' if any number containing 2 except 12 is input or 11, 12 and 13 are input", () => { |
There was a problem hiding this comment.
A more concise description could be "Expect suffix 'th' if the last two digits of the number are 11, 12, or 13"
| return false; //Numbers divisible by 2 or 3 with no remainder are prime numbers | ||
| } | ||
|
|
||
| return true; //All other instances are prime numbers |
There was a problem hiding this comment.
Todo
5 is a prime numbers.
5 * 5 = 25 is not divisible by 2 or 3, but it is divisible by 5. So 25 is not a prime number.
You need additional code to make this function work for other integers.
| test("should return false if input does not contain a lowercase letter", () => { | ||
| const previousPasswords = ["passWord55@", "Access1!Point", "#MarioBros3"] | ||
| expect(passwordValidator("TELEVISION", previousPasswords)).toBe(false); | ||
| expect(passwordValidator("01010101010", previousPasswords)).toBe(false); |
There was a problem hiding this comment.
The test value "01010101010" is not specific enough. When this test fails, it could mean one or more of the followings:
- The function failed to check if the password contain a non-alphanumeric character
- The function failed to check if the password contain a lowercase letter
- The function failed to check if the password contain an uppercase letter
which is not exactly "... does not contain a lowercase letter".
Ideally, when a test fails, we want to know exactly why the test failed.
Many of the test values (invalid passwords) in this file are not specific enough.
| test('should return "Please enter a positive number" if a negative number, null or undefined is input', () => { | ||
| expect(() =>repeat("-1")).toThrow("Please enter a positive number"); | ||
| expect(() =>repeat(null)).toThrow("Please enter a positive number"); | ||
| expect(() =>repeat(undefined)).toThrow("Please enter a positive number"); |
There was a problem hiding this comment.
Why call repeat() with only one parameter?
cjyuan
added
Reviewed
| //Handling when card inputs are 2-9 | ||
| if (rank >= "2" && rank <= "9") { | ||
| //Handling when card inputs are 2-10 | ||
| if (!isNaN(rank) && rank.length<=2 && Number(rank) >= "2" && Number(rank) <= "10") { |
There was a problem hiding this comment.
Do you want to accept rank such as "07" in your design?
When we treat strings as number, we have to consider a lot of possibilities.
It is easier to
- First, ensure the string matches our expected pattern (e.g., one of 2", "3", ...., "10")
- Next, convert the string to its equivalent numerical value.
Your previous code is actually quite solid.
//Handling when card input is 10
if (rank === "10") {
return 10;
}
Self checklist
In this PR, I completed the exercises outlined in Sprint 3. I created .test files as well as .js files where applicable, ensuring that my code has been written in the right file formats which nearly caught me out.
I had to consult the documentation a lot and iterated through failed tests to reach tests that passed.
It has been interesting to learn how to write functions in different ways, especially as the use cases become larger or more advanced. I am expecting to learn more complex ways as I progress through my development.