LON | Amirhossein Aminian | Module Stracturing and Testing Data | Sprint 3 #317
Conversation
AmirhosseinAminian
changed the title
AmirhosseinAminian
added
the
Needs Review
cjyuan
added
the
Review in progress
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Some files contain multiple implementations of the same function; you should delete or comment out those that you are not using.
There are some bugs in your code. Hope my comments and suggestions can help you fix them.
There was a problem hiding this comment.
Why are there multiple implementations of isProperFraction()? Which one is the finalised one?
| if (denominator === 0) { | ||
| return "Undefined"; | ||
| } |
There was a problem hiding this comment.
Why return a string when denominator is 0? Can you think of a more appropriate value to return or another approach to report the denominator is 0?
|
|
||
|
|
||
| // Case 3: Improper fraction (absolute numerator >= absolute denominator) | ||
| if (Math.abs(numerator) >= Math.abs(denominator)) { |
There was a problem hiding this comment.
There is room to simplify the code in this function.
This is because when both parameters are numbers, by the time the execution reaches line 15, the condition will always be true.
| } | ||
|
|
||
| // Case 5: Handle Invalid Cards | ||
| return null; // Invalid card returns null |
There was a problem hiding this comment.
It is a better practice to design a function that returns only one type of value.
| function getCardValue(card) { | ||
| const rank = card[0]; | ||
| if (rank === "A") return 11; | ||
|
|
||
| if (parseInt(rank) >= 2 && parseInt(rank) <= 9) { | ||
| return parseInt(rank); | ||
| } | ||
|
|
||
| if (rank === "J" || rank === "Q" || rank === "K") { | ||
| return 10; | ||
| } | ||
|
|
||
| throw new Error("Invalid card rank."); | ||
| } |
There was a problem hiding this comment.
Does your function return the value you expected from each of the following function calls?
getCardValue("AA♠");
getCardValue("10♠");
getCardValue("QQ♠");
getCardValue("2.1♠");
| if (typeof str !== "string" || typeof count !== "number") { | ||
| return "Invalid input"; | ||
| } | ||
| if (count < 0) { | ||
| return "Count cannot be negative"; | ||
| } | ||
| return str.repeat(count); |
There was a problem hiding this comment.
How would the caller distinguish the result of the following two function calls?
repeat("Count cannot be negative", 1)repeat("", -1)
Both function calls return the same value.
Same goes for
repeat("Invalid input", 1)repeat("", "")
Can you think of a better way to indicate error?
| test("valid credit card number", () => { | ||
| const cardNumber = "9999777788880000"; | ||
| const result = isValidCreditCard(cardNumber); | ||
| expect(result).toBe(true); | ||
| }); | ||
|
|
||
| test("credit card with only one digit type", () => { |
There was a problem hiding this comment.
Could include "Expect false when" in the description to make it clearer.
| test("credit card with multiple digit types", () => { | ||
| const cardNumber = "6666666666661666"; | ||
| const result = isValidCreditCard(cardNumber); | ||
| expect(result).toBe(true); | ||
| }); |
There was a problem hiding this comment.
How is this test different from the test specified in lines 3-7?
| @@ -1,6 +1,25 @@ | |||
| const passwords = ["previousPassword1", "anotherOldPassword"]; | |||
There was a problem hiding this comment.
Would be better to specify valid passwords here so that when passwordValidator("previousPassword1") returns false, you can be certain it is not because the password is missing a special symbol.
| test("password contains at least one uppercase letter", () => { | ||
| const password = "123A5"; // Contains uppercase "A" | ||
| const result = passwordValidator(password); | ||
| expect(result).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Is "123A5" a valid password?
Should passwordValidator("123A5") return true?
cjyuan
added
Reviewed
|
Good morning @cjyuan, thank you for your valuable tips and for helping me improve my code! |
AmirhosseinAminian
added
the
Complete
3 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.