Skip to content

Conversation

@asaniDev
Copy link

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

Created tests to check the expected output of functions before they are created.

@asaniDev asaniDev added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 27, 2025
Comment on lines 13 to 15
} else {
return "Error invalid count used, please use integers from 0 upwards.";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How would the caller distinguish the result of the following two function calls?

  1. repeat("Error invalid count used, please use integers from 0 upwards.", 1)
  2. repeat("", -1)

Both function calls return the same value.

Choose a reason for hiding this comment

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

If the input is invalid (negative integers), how can we tell the user that there's an issue instead of returning a response?

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 31, 2025
@asaniDev asaniDev added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 21, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Nov 21, 2025

Did you forget to push the changes to GitHub?

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 21, 2025
@asaniDev asaniDev added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 22, 2025
Comment on lines 13 to 15
} else {
return "Error invalid count used, please use integers from 0 upwards.";
}

Choose a reason for hiding this comment

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

If the input is invalid (negative integers), how can we tell the user that there's an issue instead of returning a response?

@wahibkamran wahibkamran removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 22, 2025
return "3rd";
} else if (num > 3 || num < 21) {
return `${num}th`;
const lastDigit = num.toString()[num.toString().length - 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could consider using the more efficient approach involving the % operator to extract the last digit and the last two digits from a number directly.

Copy link
Author

Choose a reason for hiding this comment

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

used the % to have less code and less function calls

Comment on lines 11 to 41
test("should append 'st' to numbers with 1 at the end except for those ending with 11", () => {
expect(getOrdinalNumber(1)).toEqual("1st");
expect(getOrdinalNumber(21)).toEqual("21st");
expect(getOrdinalNumber(101)).toEqual("101st");
expect(getOrdinalNumber(151)).toEqual("151st");
expect(getOrdinalNumber(2061)).toEqual("2061st");
});

test("should return '2nd' for 2", () => {
test("should append 'nd' to numbers with 2 at the end except for those ending with 12", () => {
expect(getOrdinalNumber(2)).toEqual("2nd");
expect(getOrdinalNumber(22)).toEqual("22nd");
expect(getOrdinalNumber(342)).toEqual("342nd");
expect(getOrdinalNumber(592)).toEqual("592nd");
expect(getOrdinalNumber(1972)).toEqual("1972nd");
});

test("should return '3rd' for 3", () => {
test("should append 'rd' to numbers with 3 at the end except for those ending with 13", () => {
expect(getOrdinalNumber(3)).toEqual("3rd");
expect(getOrdinalNumber(33)).toEqual("33rd");
expect(getOrdinalNumber(353)).toEqual("353rd");
expect(getOrdinalNumber(93)).toEqual("93rd");
expect(getOrdinalNumber(783)).toEqual("783rd");
});

test("should return any number between 4 and 20 with a suffix of 'th' at end of number", () => {
test("should append 'th' to all other numbers which do not end in 1,2,3,11,12 or 13", () => {
expect(getOrdinalNumber(10)).toEqual("10th");
expect(getOrdinalNumber(11)).toEqual("11th");
expect(getOrdinalNumber(212)).toEqual("212th");
expect(getOrdinalNumber(113)).toEqual("113th");
expect(getOrdinalNumber(17)).toEqual("17th");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests are quite comprehensive. If you use this test script to test your implementation, you can discover some bug in your code.

Copy link
Author

Choose a reason for hiding this comment

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

added code to catch negative and non-integer values

);
//return "Error invalid count used, please use integers from 0 upwards.";
}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

  • What's the purpose of the statement on line 19? When do you expect it to be executed?

  • You can also consider structuring your code in the following way:

  if (code < 0)
    throw ...

  if (code == 0) return "";

  if (code == 1) return ...;

  let newStr = "";
  ...
  return newStr;

or (if you don't mind trading performance for simpler code)

  if (count < 0)
    throw ...;

  // this code will produce the correct result for any count >= 0 anyway.
  let newStr = "";
  ...
  return newstr;

Copy link
Author

Choose a reason for hiding this comment

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

refactored the code to reduce the number of if else statements.

@asaniDev asaniDev added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 24, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

There is a bug in the getOrdinalNumber(). Other changes look good.

Comment on lines +35 to +41
test("should append 'th' to all other numbers which do not end in 1,2,3,11,12 or 13", () => {
expect(getOrdinalNumber(0)).toEqual("0th");
expect(getOrdinalNumber(11)).toEqual("11th");
expect(getOrdinalNumber(212)).toEqual("212th");
expect(getOrdinalNumber(113)).toEqual("113th");
expect(getOrdinalNumber(17)).toEqual("17th");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add enough samples in this categories, you may discover a bug in your implementation.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 24, 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.

3 participants