London|May-2025|KhilolaRustamova|Module-Structuring-&-Testing-Data|sprint 3 #636
Conversation
HilolaRustam
changed the title
HilolaRustam
added
📅 Sprint 3
illicitonion
added
Review in progress
illicitonion
changed the base branch from
to-be-deleted-coursework/sprint-3
to
main
| @@ -8,7 +8,16 @@ | |||
| // write one test at a time, and make it pass, build your solution up methodically | |||
| // just make one change at a time -- don't rush -- programmers are deep and careful thinkers | |||
| function getCardValue(card) { | |||
|
|
|||
| const rank = card.slice(0, -1); // remove the suit emoji | |||
There was a problem hiding this comment.
This comment explains clearly what's going on - nice comment!
|
|
||
| if (num >= 2 && num <= 9) return num; | ||
|
|
||
| throw new Error("Invalid card rank"); |
There was a problem hiding this comment.
If this exception is thrown, it may not be clear to the caller what the invalid rank was - can you think how to make this more clear?
| if (["K", "Q", "J", "10"].includes(rank)) return 10; | ||
|
|
||
| const num = parseInt(rank); |
There was a problem hiding this comment.
This works, but: either of these lines of code could handle the value 10 - why did you decide to handle it in the K/Q/J case rather than the number case?
There was a problem hiding this comment.
By grouping "10" with "K", "Q", and "J", you make the intention clear:
"These are all cards that are worth 10 points, no matter what."
It also avoids need of extra range check (num === 10) in the number parsing block, which could make the logic more complex.
| @@ -22,3 +22,6 @@ test("should count multiple occurrences of a character", () => { | |||
| // 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. | |||
|
|
|||
| console.log(countChar("hello", "l")); // Should print 2 | |||
There was a problem hiding this comment.
These are correct expectations, but they're not tests written in Jest - can you try to write actual tests for these cases?
|
|
||
| }); | ||
| // output | ||
| console.log(getOrdinalNumber(1)); // "1st" |
There was a problem hiding this comment.
Again, these are fine examples, but they're not jest tests - can you turn them into jest tests?
| @@ -13,20 +13,36 @@ test("should repeat the string count times", () => { | |||
| const str = "hello"; | |||
| const count = 3; | |||
| const repeatedStr = repeat(str, count); | |||
| expect(repeatedStr).toEqual("hellohellohello"); | |||
| expect(repeat(str, count)).toEqual("hellohellohello"); | |||
There was a problem hiding this comment.
Why did you change this line? What was broken about what it did before?
illicitonion
added
Reviewed
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.