London_Jan25| Sabita_Shrestha| Module-Structuring-and-Testing-Data| Sprint 3| Week 6 #337
Conversation
SabitaShrestha325
added
the
Needs Review
suziebrown
added
Review in progress
suziebrown
left a comment
suziebrown
left a comment
There was a problem hiding this comment.
Looking good 👍
I've just left a few small suggestions where things could be improved
| // This should produce the string "CKJ", but you must not write the characters C, K, or J in the code of your solution. | ||
|
|
||
| let initials = ``; | ||
| let initials = firstName.charAt(0) + middleName.charAt(0) + lastName.charAt(0); |
There was a problem hiding this comment.
Great 👍
You can also use e.g. firstName[0] instead of firstName.charAt(0) - either option is fine.
| const twentyFourHourClockTime = "08:53"; | ||
| console.log(twelveHourClockTime); | ||
| console.log(twentyFourHourClockTime); | ||
| //it says syntax error because variable should start with letter or underscore. |
There was a problem hiding this comment.
Well spotted. Can you see any other mistake in the code?
|
|
||
| // a) How many function calls are there in this file? Write down all the lines where a function call is made | ||
| //ans- There are 5 function calls: | ||
| //carPrice = Number(carPrice.replaceAll(",", "")); |
There was a problem hiding this comment.
You're right, but can you make the explanation clearer? Which lines are the function calls on and which are the functions being called?
| // remainingSEcond = movieLength % 60 | ||
|
|
||
| // e) What do you think the variable result represents? Can you think of a better name for this variable? | ||
| // anns: Better name for this variable will be "formattedMovieDuration" because it is more descriptive, clearly indicating that the value is a formatted string representing the movie's duration. |
| const totalHours = (totalMinutes - remainingMinutes) / 60; | ||
| const timeString = `${totalHours}:${remainingMinutes}:${remainingSeconds}`; | ||
| console.log(timeString); | ||
| // It does work with another value as i change the movie length, it does work. |
There was a problem hiding this comment.
Did you try with any other values? Can you think of any edge cases where the formatting might not work correctly?
| // When the function is called with such a card, | ||
| // Then it should throw an error indicating "Invalid card rank." | ||
| const invalidCards =getCardValue("Invalid") | ||
| assertEquals(invalidCards, "Invalid card" ) |
There was a problem hiding this comment.
I think some of the test cases are not passing yet. See if you can change the code to work in all scenarios
There was a problem hiding this comment.
I can see you got all the cases working in the later exercise - well done
| expect(getAngleType(180)).toEqual("Straight angle"); | ||
| }); | ||
|
|
||
| test("should identify reflex angle (90°)", () => { |
There was a problem hiding this comment.
Remember to check that each test name accurately describes the test case. It's easy to make mistakes when writing several similar tests!
|
|
||
| // Case 4: Handle Ace (A): | ||
| test("should return 11 for ace card",()=>{ | ||
| const cardA =getCardValue("A"); |
There was a problem hiding this comment.
Make sure the input used in the test is in the same format as in the instructions
| // And a character char that does not exist within the case-sensitive str, | ||
| // When the function is called with these inputs, | ||
| // Then it should return 0, indicating that no occurrences of the char were found in the case-sensitive str. | ||
| test("should return 0 for the case-sensitive str ",()=>{ |
There was a problem hiding this comment.
Can you improve the name of this test at all? Under what circumstances should 0 be returned?
| test("should return '4th' for 4", () => { | ||
| expect(getOrdinalNumber(4)).toEqual("4th"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Can you think of any other cases that should be tested?
suziebrown
added
Reviewed
Thank you for your review. I really appreciate and I will improved area as you mention me. |
I have made changes as you mentioned. Thank you. |
SabitaShrestha325
added
Complete
3 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.