London | 25-ITP-SEP | Imran Mohamed| Sprint 3 | Coursework/sprint 3 practice tdd #794

i786m · 2025-10-23T00:15:34Z

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

I have implemented tests, and used tdd to implement the functions set out in the coursework.

jennethydyrova

Hi @i786m! Kudos on writing clean and readable functions! You did a good job with the unit tests as well. However, unit tests are designed to catch bugs from unexpected usage, which is why it's important to pass different types of parameters and test various scenarios. Please make your test cases more thorough to ensure your functions handle edge cases properly.

jennethydyrova · 2025-10-26T19:29:16Z

Sprint-3/2-practice-tdd/count.test.js

 // 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 if no occurrences of a character", () => {


These unit tests are good but they are a bit slim. Can you think of corner and edge cases that your function might encounter? What if, for example, other data types or not all parameters are passed? Thinking about these scenarios will make your function more robust and your tests more thorough.

💯 @jennethydyrova

jennethydyrova · 2025-10-26T19:35:20Z

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

  expect(getOrdinalNumber(1)).toEqual("1st");
 });
+
+test("should return '2nd' for 2", () => {


Same as my previous comment. What if someone passes "3", undefined, or another unexpected value to your function? I understand your assumptions, but users of your functions (both internal within your team and external if you build some sort of API) can pass values you didn't safeguard against, which can break your function.

jennethydyrova · 2025-10-26T19:42:24Z

Sprint-3/2-practice-tdd/repeat-str.test.js

@@ -21,12 +21,30 @@ test("should repeat the string count times", () => {
 // When the repeat function is called with these inputs,
 // Then it should return the original str without repetition, ensuring that a count of 1 results in no repetition.

+test("should return original string if count is 1", () => {


Same here, the test cases are quite limited. If I call your function with something like repeat("hello", "3") or repeat("hello", null), it will work because of javascript's type coercion, but is that the behavior you want? Writing more test cases with different input types will help you catch any behavior that you didn't intentionally design into your function.

…d invalid inputs

…flect this

…r get ordinal number

i786m · 2025-10-29T09:46:56Z

I have amended the code & tests based on the comments made , pleases let me know if this is sufficient.

jennethydyrova

Hi @i786m! Very good job. There are two things I would like you to do before completing this PR.

Remove console logs.
Decide between throwing error and returning error string in repeat fn.

Rest of my comments are optional, just food for thought 🙂

jennethydyrova · 2025-10-29T13:35:06Z

Sprint-3/2-practice-tdd/count.js

+  if (stringOfCharacters.length === 0) {
+    return 0;
+  }
+  console.log(Array.from(stringOfCharacters));


Can you please remove this console log? This helps to keep PR clean.

I have removed console log, will ensure future PRs are free from debugging logs

jennethydyrova · 2025-10-29T13:49:02Z

Sprint-3/2-practice-tdd/get-ordinal-number.js

+  if (typeof num !== "number" || isNaN(num)) {
+    throw new Error("Input must be a number");
+  }
+  if (!Number.isFinite(num)){
+    throw new Error("Input must be a finite number");
+  }
+  if (!Number.isInteger(num) || num < 0) {
+    throw new Error("Input must be a non-negative integer");
+  }


This is a good validation but I think this could be simplified because at the end your input should be a Non-negative integer. For example, Number.isFinite() and isNan() have some overlap, can you check in the MDN docs?

I am leaving this as an optional comment, no action needs to be taken to complete this PR but I would like you think if this could be written simpler. You do not need to throw different error for each small case, you can have more general error for anything that is negative non-integer.

changed code taking overlap in consideration

jennethydyrova · 2025-10-29T14:07:52Z

Sprint-3/2-practice-tdd/repeat-str.js

+  if ( arguments.length !== 2) {
+    return "Function requires exactly 2 arguments";
+  }
+  if (typeof str !== "string") {
+    return "First argument must be a string";
+  }
+  if (!Number.isInteger(count) || count < 0) {
+    return "Second argument must be a non-negative integer";
+  }


I see you extensively use throw Error in other functions, is there a reason you want to use just return here?

Small point, no action required to complete this PR but you could improve your error messages to be something like throw Error("Second argument must be a non-negative integer, received: ${count} (${typeof count})") (same for str). It would be a bit easier to debug.

changed returns to throw errors to match the rest in this sprint

wheresdiasd

It looks good overall, @jennethydyrova comments must be addressed IMO.

My comments are only suggestions to improve the code synthax.

wheresdiasd · 2025-11-01T14:03:40Z

Sprint-3/2-practice-tdd/count.js

@@ -1,5 +1,22 @@
 function countChar(stringOfCharacters, findCharacter) {
-  return 5
+  if (arguments.length !== 2) {


That looks good, but can we be more specific about which arguments we are expecting? Using " arguments" works, but in my experience is not very common.

wheresdiasd · 2025-11-01T14:06:55Z

Sprint-3/2-practice-tdd/count.js

+  if (findCharacter.length !== 1) {
+    throw new Error("Character to find must be a single character.");
+  } 
+  if (stringOfCharacters.length === 0) {


Would that be the same as
(!stringOfCharacters.length) ?

wheresdiasd · 2025-11-01T14:09:45Z

Sprint-3/2-practice-tdd/count.js

+  if (typeof stringOfCharacters !== 'string'){
+    throw new Error("First argument must be a string.");
+  }
+  if (typeof findCharacter !== "string") {


Can we wrap up this condition and the condition above into a single condition ?

wheresdiasd · 2025-11-01T14:15:58Z

Sprint-3/2-practice-tdd/count.js

+  if (stringOfCharacters.length === 0) {
+    return 0;
+  }  
+  return Array.from(stringOfCharacters).filter(char => char === findCharacter).length;


Could we use spread operator here ? [...stringOfCharacters] ?

wheresdiasd · 2025-11-01T14:16:59Z

Sprint-3/2-practice-tdd/count.test.js

 // 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 if no occurrences of a character", () => {


💯 @jennethydyrova

wheresdiasd · 2025-11-01T14:21:15Z

Sprint-3/2-practice-tdd/get-ordinal-number.js

@@ -1,5 +1,39 @@
 function getOrdinalNumber(num) {
-  return "1st";
+  if (arguments.length !== 1) {


Again, can we be more specific about which arguments we are referring to?

wheresdiasd · 2025-11-01T14:22:07Z

Sprint-3/2-practice-tdd/get-ordinal-number.js

+  if (!isFinite(num)) {
+    throw new Error("Input must be a finite number");
+  }
+  if (!Number.isInteger(num) || num < 0) {


Can we wrap all these conditions in one statement ?

wheresdiasd · 2025-11-01T14:32:32Z

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

  expect(getOrdinalNumber(1)).toEqual("1st");
 });
+
+test("should return '2nd' for 2", () => {


wheresdiasd · 2025-11-01T14:33:54Z

Sprint-3/2-practice-tdd/repeat-str.js

-function repeat() {
-  return "hellohellohello";
+function repeat(str, count) {
+  if ( arguments.length !== 2) {


Same about the args here, could we be more specific ?

wheresdiasd

The PR LGTM as you've addressed @jennethydyrova changes, however, I left suggestions to improve code standards, please let me know if you want to discuss any item, we can book a call/pair programming to do it so.

Thanks.

…r error handling

wheresdiasd · 2025-11-18T10:47:31Z

Could you please solve conflicts on your branch before I mark it as completed ?

…n and error handling

…n and error handling in newly files to reflect upstream changes

wheresdiasd · 2025-11-18T17:20:29Z

Sprint-3/2-practice-tdd/repeat-str.test.js

+
+test('should have the correct amount of arguments', () => { 
+  expect(() => repeatStr('hello')).toThrow(new Error("Function requires exactly two arguments: a string and a count. Received 1 arguments"));
+  expect(() => repeatStr("hello", 3, 3)).toThrow(new Error("Function requires exactly two arguments: a string and a count. Received 3 arguments"));


We don't need to check if the function receives more than 2 arguments.

We need to check whether the arguments are expected or not.

In this case, you have 2 arguments, and your logic works fine. But that wouldn't be accepted in real use cases.

Ideally, you would first check if the function receives both str and count before proceeding to any other steps, and that would be enough.

In the real world, functions can indeed receive more parameters than they expect, but they don't do anything with these parameters.

wheresdiasd

The logic works, but adding a check to see if the function receives more arguments is unrealistic.

i786m · 2025-11-18T19:11:43Z

Removed the test for more than 2 arguments as you've pointed out in your previous comment and I've taken your comments on board

wheresdiasd

The PR looks good, although I would still avoid the checking arguments.length whenever possible.

i786m added 3 commits October 22, 2025 23:19

implement countChar function and associated tests

2cd8d18

implemented getOrdinalNumber function and associated tests

abcb286

implemented repeat function and associated tests

8a60a16

i786m added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 23, 2025

jennethydyrova 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 26, 2025

jennethydyrova suggested changes Oct 26, 2025

View reviewed changes

jennethydyrova 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 26, 2025

i786m added 4 commits October 28, 2025 23:40

updated count char function and tests to include edge/corner cases an…

2958939

…d invalid inputs

added test for correct amount of arguments and amended function to re…

b290716

…flect this

added edge/corner case and invalid input tests and associated code fo…

5e7feb2

…r get ordinal number

Added edge case and invalid input tests and updated function to pass

2beffba

i786m 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 29, 2025

i786m requested a review from jennethydyrova October 29, 2025 09:44

jennethydyrova reviewed Oct 29, 2025

View reviewed changes

jennethydyrova 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 29, 2025

i786m added 2 commits November 1, 2025 11:16

removed console logs from count.js

d933572

applied changes to ordinal and repeat functions and tests

7fabb60

i786m 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 Nov 1, 2025

i786m requested a review from jennethydyrova November 1, 2025 12:53

wheresdiasd reviewed Nov 1, 2025

View reviewed changes

wheresdiasd suggested changes Nov 1, 2025

View reviewed changes

refactor countChar function to validate arguments and update tests fo…

dad2134

…r error handling

wheresdiasd 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 Nov 18, 2025

wheresdiasd added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 18, 2025

wheresdiasd added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Complete Volunteer to add when work is complete and all review comments have been addressed. labels Nov 18, 2025

i786m added 3 commits November 18, 2025 15:38

refactor countChar and repeat functions to improve argument validatio…

5233e20

…n and error handling

add repeatStr function and comprehensive tests for argument validatio…

86d69fb

…n and error handling in newly files to reflect upstream changes

Merge branch 'main' into coursework/sprint-3-practice-tdd

b8bc634

i786m 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 Nov 18, 2025

i786m requested a review from wheresdiasd November 18, 2025 16:38

wheresdiasd reviewed Nov 18, 2025

View reviewed changes

wheresdiasd suggested changes Nov 18, 2025

View reviewed changes

wheresdiasd 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 Nov 18, 2025

removed test for more than 2 arguments

245f285

i786m 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 Nov 18, 2025

i786m requested a review from wheresdiasd November 18, 2025 17:55

wheresdiasd 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 Nov 18, 2025

wheresdiasd approved these changes Nov 18, 2025

View reviewed changes

i786m mentioned this pull request Nov 24, 2025

ITP-SEP-25 | Structuring and testing data | Imran Mohamed i786m/My-Coursework-Planner#89

Open

Uh oh!

London | 25-ITP-SEP | Imran Mohamed| Sprint 3 | Coursework/sprint 3 practice tdd #794

Are you sure you want to change the base?

London | 25-ITP-SEP | Imran Mohamed| Sprint 3 | Coursework/sprint 3 practice tdd #794

Conversation

i786m commented Oct 23, 2025

Changelist

Uh oh!

jennethydyrova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i786m commented Oct 29, 2025

Uh oh!

jennethydyrova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wheresdiasd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wheresdiasd left a comment

Choose a reason for hiding this comment

Uh oh!

wheresdiasd commented Nov 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wheresdiasd left a comment

Choose a reason for hiding this comment

Uh oh!

i786m commented Nov 18, 2025

Uh oh!

wheresdiasd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development