Skip to content

Glasgow | 25-ITP-Sep | Fares Bakhet | Sprint 3 | coursework/sprint-3-practice-tdd #757

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Glasgow | 25-ITP-Sep | Fares Bakhet | Sprint 3 | coursework/sprint-3-practice-tdd #757

wants to merge 12 commits into from
+108 −12

Conversation

@Fares-Bakhet
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

  • Build up functions, add tests.
  • Run the suitable tests.

Questions

No questions.

@Fares-Bakhet Fares-Bakhet added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Structuring-And-Testing-Data The name of the module. labels Oct 12, 2025
@cjyuan cjyuan 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 Oct 25, 2025
cjyuan
cjyuan reviewed Oct 25, 2025
View reviewed changes
Sprint-3/2-practice-tdd/get-ordinal-number.js
Comment on lines 2 to 19
num = num.toString();
if (
num.slice(-2) === "11" ||
num.slice(-2) === "12" ||
num.slice(-2) === "13"
) {
return num + "th";
} else if (num.slice(-1) === "1") {
return num + "st";
return num + "st";
} else if (num.slice(-1) === "2") {
return num + "nd";
} else if (num.slice(-1) === "3") {
return num + "rd";
} else {
return num + "th";
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • There is an unreachable statement (dead code).

  • Note: because of the return statements, using else is optional.

Copy link
Author

Choose a reason for hiding this comment

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

@cjyuan, Found it 😅😅😅

Sprint-3/2-practice-tdd/get-ordinal-number.test.js
Copy link
Contributor

Choose a reason for hiding this comment

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

To ensure thorough testing, we need broad scenario coverage. Listing individual values, however, can quickly lead to an unmanageable number of test cases.
Instead of writing tests for individual numbers, consider grouping all possible input values into meaningful categories. Then, select representative samples from each category to test. This approach improves coverage and makes our tests easier to maintain.

For example, we can prepare a test for numbers 2, 22, 132, etc. as

test("append 'nd' to numbers ending in 2, except those ending in 12", () => {
    expect( getOrdinalNumber(2) ).toEqual("2nd");
    expect( getOrdinalNumber(22) ).toEqual("22nd");
    expect( getOrdinalNumber(132) ).toEqual("132nd");
});

Copy link
Author

Choose a reason for hiding this comment

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

@cjyuan, Done 🫡

Sprint-3/2-practice-tdd/repeat.js Outdated
Comment on lines 2 to 3
const str = arguments[0];
const count = arguments[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just declare them as parameters?

Copy link
Author

Choose a reason for hiding this comment

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

@cjyuan , Okay 👌

@cjyuan cjyuan 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 Oct 25, 2025
@Fares-Bakhet Fares-Bakhet added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 25, 2025
cjyuan
cjyuan reviewed Oct 25, 2025
View reviewed changes
Sprint-3/2-practice-tdd/get-ordinal-number.test.js Outdated

test("should return '101st' for 101", () => {
expect(getOrdinalNumber(101)).toEqual("101st");
test("should return 'th' for the rest numbers", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

"the rest [of the] numbers" is not very informative by itself; the developer would need to read all the test descriptions to find out what "the rest ..." mean. Can you make this test description more informative?

Sprint-3/2-practice-tdd/repeat.js Outdated
Comment on lines 2 to 3
//const str = arguments[0];
//const count = arguments[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why keep the unused code?

Sprint-3/2-practice-tdd/get-ordinal-number.js Outdated
Comment on lines 15 to 16
}
return num + "th";
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you have decided to remove the optional else, why only remove the last else?

Copy link
Author

Choose a reason for hiding this comment

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

@cjyuan, Okay

@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 25, 2025
@Fares-Bakhet Fares-Bakhet added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 25, 2025
cjyuan
cjyuan reviewed Oct 25, 2025
View reviewed changes
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.

Changes look good! Well done.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjyuan cjyuan cjyuan left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. Module-Structuring-And-Testing-Data The name of the module. 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Fares-Bakhet @cjyuan