Skip to content

London | ITP-May-25 | Anuar Duisenbek | Module-Structuring-and-Testing-Data | Sprint-3 #648

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 7 commits into
base: main
Choose a base branch
from

Conversation

AnuarBey
Copy link

@AnuarBey AnuarBey commented Jul 9, 2025

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_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

This PR includes my responses and code to questions in the: Key Implement, Mandatory Rewrite, Mandatory Practice and the Stretch Investigate files.

Questions

Ask any questions you have for your reviewer.

@AnuarBey AnuarBey added the Needs Review Participant to add when requesting review label Jul 9, 2025
Copy link

@Amundeep-Dhaliwal Amundeep-Dhaliwal left a comment

Choose a reason for hiding this comment

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

Hello Anuar, hope you are well.

Apologies for the delay in marking, this PR looks good to me.

Please see the comments.

@@ -8,8 +8,11 @@
// Then, write the next test! :) Go through this process until all the cases are implemented

function getAngleType(angle) {
if (angle === 90) return "Right angle";
// read to the end, complete line 36, then pass your test here
if (angle === 90) return "Right angle";

Choose a reason for hiding this comment

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

This function look good.

@@ -8,7 +8,7 @@
// write one test at a time, and make it pass, build your solution up methodically

function isProperFraction(numerator, denominator) {
if (numerator < denominator) return true;
return Math.abs(numerator) < Math.abs(denominator);

Choose a reason for hiding this comment

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

Glad that you used a concise statement for this function.

How would you handle situations where the denominator is 0?

// ====> complete with your assertion

// Stretch:
// What other scenarios could you test for?

const zeroNumerator = isProperFraction(0, 5);

Choose a reason for hiding this comment

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

According to a web search, a proper fraction is one where the absolute value of the numerator is smaller than the absolute value of the denominator. Like you implemented.

The numerator also has to be non-zero.

let rank = card.slice(0, -1);
if (rank === "A") return 11;
if (rank === "K" || rank === "Q" || rank === "J" || rank === "10") return 10;
if (Number(rank) >= 2 && Number(rank) <= 9 && rank.length === 1)

Choose a reason for hiding this comment

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

This looks good.

console.log("Test failed: should have thrown an error");
} catch (e) {
assertEquals(e.message, "Invalid card rank");
}

Choose a reason for hiding this comment

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

Appreciate the innovation on this one.

expect(getCardValue("Q♦")).toEqual(10);
});
test("should return 10 for King of Hearts", () => {
expect(getCardValue("K♥")).toEqual(10);

Choose a reason for hiding this comment

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

These tests look good, there is good coverage.

@@ -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.
test("should return 0 when there are no occurrences", () => {

Choose a reason for hiding this comment

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

We could add more test cases, for example:

Counting the number of a in aaa
Counting the number of e in excellent

if (lastDigit === 1) return `${num}st`;
if (lastDigit === 2) return `${num}nd`;
if (lastDigit === 3) return `${num}rd`;
return `${num}th`;

Choose a reason for hiding this comment

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

This implementation looks nice.


test("should return '4th' for 4", () => {
expect(getOrdinalNumber(4)).toEqual("4th");
});

Choose a reason for hiding this comment

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

Testing on 11, 12, 13 would also be good for coverage.


// case: Negative Count:
// Given a target string str and a negative integer count,
// When the repeat function is called with these inputs,
// Then it should throw an error or return an appropriate error message, as negative counts are not valid.
test("should throw error for negative count", () => {

Choose a reason for hiding this comment

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

These tests look good.

@Amundeep-Dhaliwal Amundeep-Dhaliwal added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jul 11, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants