CYF London | Fatma Arslantas | Module-Structuring-and-Testing-Data | Week 3#209
CYF London | Fatma Arslantas | Module-Structuring-and-Testing-Data | Week 3#209AFatmaa wants to merge 13 commits intoCodeYourFuture:mainfrom
Conversation
cjyuan
left a comment
There was a problem hiding this comment.
I left my comments and suggestions in the code.
|
|
||
| // Example assertions to test the function | ||
| try { | ||
| console.assert(getCardValue("3♠") === 3, "Test failed for '3♠'"); |
There was a problem hiding this comment.
What do you expect from the following function calls?
Does your function returns the value you expected?
getCardValue("20Q♠");
getCardValue("02♠");
getCardValue("AA♠");
getCardValue("2.1♠")
|
|
||
| console.log(isProperFraction(2, 3)); | ||
| console.log(isProperFraction(5, 2)); | ||
| console.log(isProperFraction(-4, 7)); |
There was a problem hiding this comment.
Suppose the notation |X| denotes the absolute value of X.
If you are unfamiliar with the definition of absolute value, you should look the term up.
To test your function more comprehensively, you should consider testing all combinations of positive and negative parameters. For examples,
- For cases where |numerator| < |denominator|, test
isProperFraction(4, 7),isProperFraction(-4, -7),isProperFraction(-4, 7),isProperFraction(4, -7). - Do the same for cases where |numerator| > |denominator|.
Hint: If you compute the absolute value of both parameters inside the function first, the code can become much simpler.
| return false | ||
| } | ||
| // Check for non-positive side length | ||
| if (a <= 0 || b <= 0 || c <= 0) { |
There was a problem hiding this comment.
Is it necessary to include the if-statement at lines 23-25?
Can you find any values for a, b, and c, such that the function will fail after you removed the if-statement at lines 23-25?
To check if three sides can form a valid triangle, a lot of people added an if-statement in their code to ensure the three sides are positive numbers. But have you ever wondered why they did that? The answer is related to how the value of a, b, and c are represented in a particular programming language, and you may not have the necessary knowledge yet to understand why it is necessary to ensure a, b, and c are positives in some programming languages.
The main point I would like to raise is, you should fully understand what you wrote in your code. An interviewer may ask you questions like what I am asking here, and it would reflect poorly on you if you cannot explain your code.
| expect(getAngleType(200)).toBe("Reflex angle"); | ||
| }); | ||
|
|
||
| test('Get Invalid angle ', () => { |
There was a problem hiding this comment.
Why not test also angles that are < 0 and angles that are >= 360? Especially the boundary cases like 0 and 360.
|
|
||
| // Return the number with the appropriate suffix. | ||
| // Use modulus to determine the correct suffix or fall back to "th". | ||
| return `${num}${suffixes[(value - 20) % 10] || suffixes[value] || suffixes[0]}`; |
There was a problem hiding this comment.
Can you elaborate how this expression yield the suffix value when value is 11?
suffixes[(value - 20) % 10] || suffixes[value] || suffixes[0]
| // Check if the input is less than 2, which is not prime | ||
| if (num < 2) return false; | ||
|
|
||
| // Loop from 2 to the square root of the number |
There was a problem hiding this comment.
You can possibly improve the performance of the code in the following manners:
- Check if num is 2, and check only odd numbers >= 3 in the loop
- Avoid calling
Math.sqrt(num)repeatedly by assigning the value ofMath.sqrt(num)to a variable once, and then refer to the variable in the condition of the loop.- Note: The condition is checked at the start of every iteration.
| }); | ||
|
|
||
| test('should handle larger non-prime numbers', () => { | ||
| expect(isPrime(100)).toBe(false); |
There was a problem hiding this comment.
Could also test some odd composite numbers, and specify the parameter as 101 * 13 (a composite number as a product of two prime numbers).
| }); | ||
|
|
||
| test('should return false for a password that is too short', () => { | ||
| expect(isPasswordValid("sh1*", previousPasswords)).toBe(false); |
There was a problem hiding this comment.
"Sh1*" would be a better test value because when the function return false, it would imply "Sh1*" passes all other checks except the password length check.
|
Hi @cjyuan, Thank you for your review. I'll check and fix them! 🙌😊 |
|
Hi @cjyuan, Thank you for taking the time to review my PR and for your detailed comments and suggestions. I've carefully examined each of them, made the necessary adjustments, and committed the changes. Your feedback was helpful and I truly appreciate your guidance and feedback! Please let me know if there’s anything else that needs improvement. 🙌 |
|
Code is correct. Can you explain how this expression work when |
|
Hi @cjyuan, Can you explain how this expression work when value is 15?
Example: value = 15
The || suffixes[0] part ensures that the default "th" suffix is used if there’s no valid suffix at the index value % 10. For example, when the last digit is 5 or 6, "th" is returned as a fallback. Thank you for taking the time to ask detailed questions and help me learn. I really appreciate it! 😊 |
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.