-
-
Notifications
You must be signed in to change notification settings - Fork 301
ITP-May-25|NW | Rahwa Zeslus| Module-Structuring-and-Testing-Data | Week3 #500
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
ITP-May-25|NW | Rahwa Zeslus| Module-Structuring-and-Testing-Data | Week3 #500
Conversation
…racter occurrences
…ers with test cases that handle the edge cases
|
Note: if you first change to the This makes it easier to review as it's only the relevant changes 🙂 |
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.
This is really good - I've asked some more advanced questions as I can see you're doing really well.
I will mark as Complete because this already meets all the criteria, but feel these questions will help you go further 🙂
| @@ -10,7 +10,16 @@ | |||
| function getAngleType(angle) { | |||
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 you expect to see from this?
getAngleType("45");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 expect an error because "45" is a string not a number.I refactor the code to convert the parameter to number datatype and I use switch(true),just to stretch my self
|
|
||
| if(rank === "A") return 11; | ||
| if (["J","Q","K"].includes(rank)) return 10; | ||
| if(!isNaN(rank) && Number(rank) >= 2 && Number(rank) <= 10 ) return Number(rank); |
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.
Is there a way to simplify this if statement?
e.g. could we extract a common variable, do we need all 3 conditions?
| if(angle > 90 && angle < 180) return "Obtuse angle" | ||
| if(angle === 180) return "Straight angle" | ||
| if(angle > 180 && angle < 360) return "Reflex angle" | ||
| if (typeof angle !== 'number' || angle < 0 || angle >= 360) return "Invalid angle"; |
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.
Could moving this validation if statement to the top help simplify our function?
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.
One thing I like to remember: code is read by the computer, but also by other people.
You're getting on great with these courseworks, so I think you should start considering: can I make this code easier to read for other people?
|
|
||
| // Case 3: Identify Negative Fractions: | ||
| test("should return true for a proper fraction with one of the value negative ", () => { | ||
| expect(isProperFraction(-2, 3)).toEqual(true); |
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.
Jest also has .toBeTruthy(): https://jestjs.io/docs/expect#tobetruthy
Why might that not be a good thing to use here?
|
@CameronDowner Thank you so much for taking the time to review my code. I truly appreciate your thoughtful feedback — it helped me spot things I had overlooked ,your insights are incredibly valuable to my learning, and I’m grateful for your support and encouragement. |
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.
Thank you for your time in reviewing this work