London | 25-ITP-Sep | Shaghayegh Shirinfar | Sprint 3 | Practice-tdd #818

shaghayeghfar · 2025-10-28T13:45:41Z

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

jennethydyrova

Hi @shaghayeghfar ! Good start on this PR, I left some comments to address

jennethydyrova · 2025-10-28T16:30:12Z

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

 // 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 the character does not exist in the string", () => {


This test suite is very slim, can you think of more test cases? You should try to cover as many scenarios as possible to test whether your function behaves right if something expected or unexpected passed to your function.

jennethydyrova · 2025-10-28T16:31:05Z

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

@@ -1,5 +1,8 @@
-function getOrdinalNumber(num) {
-  return "1st";
+function getOrdinalNumber(number) {


Unfortunately, this function is not passing the acceptance criteria. What if I pass, for example, 23 to it?

jennethydyrova · 2025-10-28T16:33:39Z

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

  expect(getOrdinalNumber(1)).toEqual("1st");
 });
+
+// Case 2: Identify the ordinal number for 2


I think it's a good illustration why you need to write more versatile unit tests. Before fixing your function, add more unit tests (for different numbers) and see if they pass. You will see that they do not and this is a good sign that your function implementation is incorrect.

jennethydyrova · 2025-10-28T16:35:32Z

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

 // 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 the original string when count is 1", () => {


These unit tests are correct but they are a bit limited. What if I pass undefined as str and count 2? What if I pass empty string? You should try to guard your function against these scenarios and unit tests help to think of different scenarios.

jennethydyrova · 2025-10-28T16:36:28Z

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

@@ -1,5 +1,11 @@
 function countChar(stringOfCharacters, findCharacter) {
-  return 5
+  let count = 0;


Nice use of for loop!

shaghayeghfar · 2025-10-29T00:12:55Z

@jennethydyrova

Dear Jennet,

Thank you very much for reviewing my code. I have made the changes you suggested.

I really appreciate your help, time, and effort.

Best regards,

jennethydyrova

Hi @shaghayeghfar! Very nice improvement since last iterations but there are couple of more things you will need to do before we can mark this PR as complete.

jennethydyrova · 2025-10-29T14:29:57Z

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

+  expect(count).toEqual(0);
+});
+
+test("should return 0 when the character does not exist in the string", () => {


This is a duplicate of the test on L47

jennethydyrova · 2025-10-29T14:32:05Z

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

  expect(count).toEqual(5);
 });

+test("should count multiple occurrences of a character", () => {


Okay, this is a good improvement on the previous iteration. Can you think of corner cases? What if length of stringOfCharacters is shorter than length of findCharacter? What either of them is an empty string or some other data type?

jennethydyrova · 2025-10-29T14:34:18Z

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

+
+// Case 2: Identify the ordinal number for 2
+// When the number is 2,
+// Then the function should return "2nd"


Same as the comment for countChar function's tests. If I pass undefined, I think your function will output undefinedth, is this a behaviour you expect? Can you think of other corner cases and how you can guard your function against them?

jennethydyrova · 2025-10-29T14:36:36Z

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

+  if (str === undefined) {
+    throw new Error("String must be defined");
+  }
+  if (str === null || typeof str !== "string") {
+    throw new Error("String must be a valid string");
+  }
+
+  if (count === undefined || typeof count !== "number") {
+    throw new Error("Count must be a number");
+  }
+
+  if (count < 0 || !Number.isInteger(count)) {
+    throw new Error("Count must be a non-negative integer");
+  }
+
+  if (str === "" || count === 0) {
+    return "";
+  }


This is okay but it could be simplified by combining some of these if conditions and/or writing more general if condition that would catch most of the cases. You don't need to do anything, it's just something to think about and perhaps to implement on your spare time.

jennethydyrova · 2025-10-29T14:37:03Z

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

+    return "";
+  }
+
+  // 5. Repeat the string count times


Comment seems a remnant of the sequence of comments. Can you either add all or remove this?

shaghayeghfar · 2025-10-30T12:53:54Z

Dear Jennet,

Thank you so much for your feedback. I have made the changes you suggested and truly appreciate all your support and guidance .I am very grateful for it.

jennethydyrova

Hi @shaghayeghfar! Very good improvement but there are couple of things I would like you fix before we complete this PR. Also, can you please remove any unrelated files from this PR?

jennethydyrova · 2025-10-30T18:19:57Z

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

 function countChar(stringOfCharacters, findCharacter) {
-  return 5
+  if (
+    typeof stringOfCharacters !== "string" ||


If either or both arguments are not strings, does it really make sense to return 0? Think of it this way, you call the countChar(2, 3), what would you expect your function to return? I would suggest you to check this link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error

jennethydyrova · 2025-10-30T18:23:43Z

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

+  ) {
+    return 0;
+  }
+  if (findCharacter.length !== 1) {


This is a good check! As an extension, what if stringOfCharacters.length is 0 and findCharacter.length is 1? I see you now return just 0 but would it perhaps make more sense to throw an error because there is nothing to search character in?

jennethydyrova · 2025-10-30T18:30:37Z

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

+// When the function is called,
+// Then it should return 0 because a multi-character string cannot match a single character position.
+
+test("should return 0 when findCharacter is longer than the input string", () => {


I think it's a bit incorrect. This is related to my earlier comment for the countChar function. If str length is shorter than char length, the function should return an error. Returning 0 should be reserved only for the case when character is not found. Otherwise, you function logic becomes convoluted, you wouldn't know if it returned 0 because character is not found or because arguments were invalid. Do you see the problem with it?

jennethydyrova · 2025-10-30T18:32:02Z

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

+function getOrdinalNumber(number) {
+  // Handle invalid inputs
+  if (typeof number !== "number" || !Number.isFinite(number)) {
+    return "Invalid input";


This is okay but it would be cleaner to mention what you expect as a valid input.

jennethydyrova · 2025-10-30T18:32:31Z

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

+// Case NaN input :
+// When the nymber is NaN  ,
+// Then the function should return "Invalid input"
+


Nice test cases, very comprehensive!

shaghayeghfar · 2025-11-02T14:58:01Z

Dear Jennet,

Thank you for your feedback! I’ve made all the changes you requested. However, I’m not entirely sure what you meant regarding removing the unrelated files. Could you please provide a bit more guidance on that?

Thanks so much for your help!

jennethydyrova · 2025-11-03T13:52:06Z

Hi @shaghayeghfar! You have some files that are not related to this pull request, such as package.json and package.lock. You shouldn't commit those files if they are not relevant.

Also, you have some conflicts in your PR. Could you please double check?

jennethydyrova · 2025-11-03T14:18:03Z

Ah, maybe merge conflicts are due to the changes to the main branch. You can ignore it

shaghayeghfar · 2025-11-04T13:45:59Z

ear Jennet,

Thank you so much for your help and advice. I have deleted the files as you suggested.
I really appreciate your effort and support.

jennethydyrova

Hi @shaghayeghfar ! I am marking this PR as complete but consider fixing typos

jennethydyrova · 2025-11-04T16:35:15Z

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

+  //stringOfCharacters shoud not be empty
+  if (stringOfCharacters.length === 0) {
+    return "stringOfCharacters can not be empty";
+  }
+
+  //findCharacter should not be empty
+  if (findCharacter.length === 0) {
+    return "findCharacter can not be empty";
+  }


These could be combined.

jennethydyrova · 2025-11-04T16:35:59Z

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

+    stringOfCharacters.length !== 0 &&
+    findCharacter.length > stringOfCharacters.length
+  ) {
+    return "the stringOfCharacters MUST be lonegr than findingcharacter";


typo in longer and findingcharacter

task 2 of spring 3 clean

3289b60

shaghayeghfar added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 28, 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 28, 2025

jennethydyrova reviewed Oct 28, 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 28, 2025

shaghayeghfar added 4 commits October 28, 2025 22:16

add nore test case to count.test.js

47c3136

get-ordinal-number is done

87552c8

more test is done

4386c93

repeat task is done

a8355cf

jennethydyrova suggested changes Oct 29, 2025

View reviewed changes

chages are done for undified and test cases are updated

ce61aaf

shaghayeghfar mentioned this pull request Oct 30, 2025

LONDON-SEP-25 | Structuring and testing data | Shaghayegh Shirinfar shaghayeghfar/My-Coursework-Planner#37

Open

jennethydyrova suggested changes Oct 30, 2025

View reviewed changes

all changes are done

7795cdb

shaghayeghfar added 4 commits November 4, 2025 13:18

Remove package-lock.json from version control

3fe83f1

Revert package.json to match main branch

4b078fa

Ignore package-lock.json in future commits

9861412

Remove package.json from branch

29a4b36

shaghayeghfar force-pushed the coursework/sprint-3-practice-tdd1 branch from 2f117e1 to 29a4b36 Compare November 4, 2025 13:33

Delete Sprint-3/2-practice-tdd/package.json

1907463

shaghayeghfar 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 4, 2025

jennethydyrova approved these changes Nov 4, 2025

View reviewed changes

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

Uh oh!

London | 25-ITP-Sep | Shaghayegh Shirinfar | Sprint 3 | Practice-tdd #818

Are you sure you want to change the base?

London | 25-ITP-Sep | Shaghayegh Shirinfar | Sprint 3 | Practice-tdd #818

Uh oh!

Conversation

shaghayeghfar commented Oct 28, 2025

Learners, PR Template

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!

shaghayeghfar 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!

shaghayeghfar commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jennethydyrova Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

shaghayeghfar commented Nov 2, 2025

Uh oh!

jennethydyrova commented Nov 3, 2025

Uh oh!

jennethydyrova commented Nov 3, 2025

Uh oh!

shaghayeghfar commented Nov 4, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shaghayeghfar commented Oct 30, 2025 •

edited

Loading

jennethydyrova Oct 30, 2025 •

edited

Loading