WM | May-2025 | Abdullah Saleh | Sprint-3 #627
Conversation
…rs are numeric also create some test cases
There was a problem hiding this comment.
Always validate input types (string/number) when writing functions that expect numbers.
function isProperFraction(numerator, denominator) {
if (denominator === 0) return "Undefined";
else if (numerator < denominator) return true;
else return false;
}
``` check the type of input variable
illicitonion
added
Review in progress
illicitonion
added
Reviewed
| if (cardNumber.length !== 16) { | ||
| return false; | ||
| } | ||
| const numberInArray = cardNumber.toString().split(""); // convert the cardNumber to sting then into an array |
There was a problem hiding this comment.
It's interesting that you check its length and then convert it to a string. What type do you think cardNumber may have which has a .length property, but isn't a string?
There was a problem hiding this comment.
I originally thought this was an edge case to handle, but it turned out not to be a good approach — even my test tricked me by passing.
The reason the test passed is because cardNumber.length was undefined, so the condition undefined !== 16 returned true, leading to a false return value that matched the expected outcome.
I later got advice from AI that JavaScript rounds numbers longer than 15 digits, so it's always better to pass long numbers like card numbers as strings.
Just to confirm — I’m planning to remove the .toString() call from my function, since the cardNumber will always be passed as a string.
…er return values.
| function repeat(str, count) { | ||
| if (count > 0) return `${str.repeat(count)}`; | ||
| else if (count == 0) return ""; | ||
| else if (count < 0) return "negative counts are not valid"; |
There was a problem hiding this comment.
What do you think about returning a string here vs throwing an error? With the fraction with denominator of 0 you decided to throw - is there a reason to prefer one or the other here?
There was a problem hiding this comment.
You're right — there's no strong reason for treating them differently.
It would be more consistent to throw an error for invalid input like a negative count, just as I did with the zero denominator case.
| else if (rank >= 2 && rank <= 9) return Number(rank); | ||
| else if (rank === 10 || rank === "J" || rank === "Q" || rank === "K") return 10; |
There was a problem hiding this comment.
You could have handled 10 in the "If a number" case or the "J/Q/K" case (both of which work). Why did you decide to choose the approach you did? What benefits/drawbacks does each have?
There was a problem hiding this comment.
Actually, I handled it that way because I was following the structure of the test cases. I assumed that 10 could be treated as part of the numeric range and added to the first condition, rather than grouping it with the string values like "J", "Q", or "K".
illicitonion
added
Complete
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.