-
-
Notifications
You must be signed in to change notification settings - Fork 240
NW | 25-ITP-Sep | TzeMing Ho | Sprint 3 | coursework/sprint-3-implement-and-rewrite #709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
NW | 25-ITP-Sep | TzeMing Ho | Sprint 3 | coursework/sprint-3-implement-and-rewrite #709
Conversation
…ixed the conflict with a new variable name
…wer case to upper case and replace all space with underline
…toPounds with input condition checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job covering most test cases @TzeMingHo - I left a few comments for you to review.
| // complete the rest of the tests and cases | ||
| // write one test at a time, and make it pass, build your solution up methodically | ||
|
|
||
| function isProperFraction(numerator, denominator) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your isProperFraction works well. However, if you wanted to simplify it even more, you could try:
// this will return either true or false
return (Math.abs(numerator) < Math.abs(denominator));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if I run the test with:
- angle: 'ten'
- angle: 1000
- angle: -22
- angle: null
| // Input: numerator = "a", denominator = 2 | ||
| // target output: false | ||
| // Explanation: The function should handle non-numeric inputs gracefully. In this case, we choose to return false. | ||
| const invalidInputs = isProperFraction("a", 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 Good job making sure that 'unhappy path' is tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run your tests, I get this result:
Assertion failed: Expected 1♠ to equal Error: Invalid card rank
Assertion failed: Expected Z♠ to equal Error: Invalid card rank
Assertion failed: Expected to equal Error: Invalid card rank
Can you take a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a few more 'unhappy path' tests:
- card value of true
- card value of 22
- card value of null
I was expecting ('Error: Invalid card rank').
package-lock.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you included this file by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're almost there! Most everything is passing now except /implement/3-get-card-value.js.
| // Then, write the next test! :) Go through this process until all the cases are implemented | ||
|
|
||
| function getAngleType(angle) { | ||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where you are going with this. :)
It can be simplified to this:
if (angle === null || isNaN(Number(angle))) {
return "Input should be a number or a number in string";
}
⏫ You don't have to modify your code - I am simply pointing out a small improvement for 'next time' you do something similar to this.
| // just make one change at a time -- don't rush -- programmers are deep and careful thinkers | ||
| function getCardValue(card) { | ||
| const errorMessage = | ||
| "Error: Invalid card rank. Input should be a string in the format 'R♠', where R is 2-10, J, Q, K, or A, followed by either ♠, ♥, ♦, or ♣."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest keeping the error message simple here:
'Error: Invalid card rank'
Right now, your tests are failing because the message you are expecting isn't the same as the one you are receiving. If you change the error message to 'Error: Invalid rank', it should pass correctly.
💡 Make sure that you run your tests before you submit your PR.
| }); | ||
| // Case 5: Handle Invalid Cards: | ||
| const errorMessage = | ||
| "Error: Invalid card rank. Input should be a string in the format 'R♠', where R is 2-10, J, Q, K, or A, followed by either ♠, ♥, ♦, or ♣."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest changing the error message back to something more simple and readable: Error: Invalid card rank.
|
Your PR looks good now. Good job! |
Learners, PR Template
Self checklist
Changelist
For the 3 exercises, I have created assert conditions and tests for each case, as well as some stretch cases.