Skip to content

ZA Cape Town | ITP-May-2025 | Dawud Vermeulen | Module-Structuring and Testing Data Sprint-3 #688

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Dave-Vermeulen
Copy link

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Completed all exercises in the following subfolders:

  • implement
  • practice
  • rewrite

Questions

Please review my code 🪻🙏🏽

@Dave-Vermeulen Dave-Vermeulen added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 30, 2025
@illicitonion illicitonion added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 6, 2025
Copy link
Member

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is generally looking pretty good, but please format your code to be easier to read, and make sure you tidy things up (e.g. remove obsolete comments).

}
const rank = card.slice(0, -1);
if (rank === "A") return 11;
else if (rank === "J", "Q", "K") return 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each line of your code here seems to be getting more and more indented, even though there aren't any {s - please can you format your code consistently so it's easier to read.

This applies to all of your code in this PR.

return 11;
const rank = card.slice(0, -1); // works to extract the rank and ignores the suit
if (rank === "A") return 11 //works
else if (["J", "Q", "K", "10"].includes(rank))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your other implementation you included 10 in the 2-9 case, but in this one you're including it in the J/Q/K case - both work, but can you talk through which you think is simpler?

// Case 5: Handle Invalid Cards:
test("should throw an error for invalid card", () => {
expect(() => getCardValue("Z♠")).toThrow("Invalid card rank."); //not sure how to write this test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a comment here saying you're not sure how, but it looks like you have - is it something you want to talk through, or if not, please remove the comment :)

if (num === 1) return "1st";
if (num === 2) return "2nd";
if (num === 3) return "3rd";
return num + "th"; // handles all cases >= 4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work for 21?

@illicitonion illicitonion added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review with trainee action still to take.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ITP-May-2025 | Structuring and testing data | Dawud Vermeulen
2 participants