Repository navigation
London | 26-ITP-Sep | Maharit Belai | Sprint 2| Practice TDD #1729
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
base: main
Are you sure you want to change the base?
Changes from all commits
cff3419
e3a0787
6cd3097
5418d11
86ffa32
a23a6a5
4fffb00
0b8b953
623a0a6
cbd3ff3
e0136e6
4a71d58
68e09e3
9b5152e
dd1a452
650bd92
a7c42ab
7f7deb4
5b59e17
494887f
4453a23
91c2378
87eb71e
55e6ea0
2a2811a
119aabb
e8ae9bb
c89337c
a87428c
ad818d2
0f414c7
2e069e9
0cbd674
b6e57ae
6fd92ce
79f8acd
20e8438
217ef19
9551f77
b1e9a88
63986b5
cd87177
19060bc
5fab553
b87e5f4
2aed003
942dc2b
52a1d81
3463aee
08dccc1
9f7796a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,12 @@ | ||
| export function countChar(stringOfCharacters, findCharacter) { | ||
| return 5 | ||
| let count = 0; | ||
| if (findCharacter.length != 1) { | ||
| throw new Error(`Invalid Input,only character is allowed`); | ||
| } | ||
| for (let i = 0; i < stringOfCharacters.length; i++) { | ||
| if (stringOfCharacters[i] == findCharacter[0]) { | ||
| count = count + 1; | ||
| } | ||
| } | ||
| return count; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,74 @@ | ||
| // implement a function countChar that counts the number of times a character occurs in a string | ||
| import { countChar } from "./count.js"; | ||
| // Given a string `str` and a single character `char` to search for, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Given/When/Then comments you removed describe each scenario in plain language, so consider restoring them and writing your own for the new scenarios you’ve added. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the updates. The Given/When/Then comments still appear to have been removed. Restoring them would help document each scenario, so it might be worth adding them back before this is merged. |
||
| // When the countChar function is called with these inputs, | ||
| // Then it should: | ||
|
|
||
| // Scenario: Multiple Occurrences | ||
| // Given the input string `str`, | ||
| // And a character `char` that occurs one or more times in `str` (e.g., 'a' in 'aaaaa'), | ||
| // When the function is called with these inputs, | ||
| // Then it should correctly count occurrences of `char`. | ||
|
|
||
| test("should count multiple occurrences of a character", () => { | ||
| test("should count multiple occurrences of a character 'a' ", () => { | ||
| const str = "aaaaa"; | ||
| const char = "a"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(5); | ||
| }); | ||
|
|
||
| // Scenario: No Occurrences | ||
| // Given the input string `str`, | ||
| // And a character `char` that does not exist within `str`. | ||
| // When the function is called with these inputs, | ||
| // Then it should return 0, indicating that no occurrences of `char` were found. | ||
|
|
||
| test(" Should count 0", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. “Should count 0” is clearer than before. It might help to describe the scenario, for example, that the character doesn’t appear in the string. There’s also still a space at the start of the name. |
||
| const str = "aaaaa"; | ||
| const char = "c"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(0); | ||
| }); | ||
|
|
||
| test("should count a character with strings of different characters", () => { | ||
| const str = "acc"; | ||
| const char = "a"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(1); | ||
| }); | ||
| test("should count a character with strings of different characters that includes uppercase", () => { | ||
| const str = "AAAa"; | ||
| const char = "a"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great idea to include this test. It shows that only the lowercase “a” is counted and the uppercase “A”s are ignored, so your function is case-sensitive. It might be helpful to rename the test to reflect that, perhaps by mentioning uppercase and lowercase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for renaming this; it’s much clearer. You could make it even more precise by saying that uppercase letters are not counted, so a reader knows the function is case-sensitive. |
||
| const count = countChar(str, char); | ||
| expect(count).toEqual(1); | ||
| }); | ||
| test("should count multiple occurences of character when given with numbers", () => { | ||
| const str = "11aa"; | ||
| const char = "1"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(2); | ||
| }); | ||
| test("should count multiple occurences of character with signs", () => { | ||
| const str = "£100"; | ||
| const char = "£"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(1); | ||
| }); | ||
| test("should count multiple occurences of character empty space", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. “Empty space” could be confused with an empty string. Would “space character” describe this test more clearly? |
||
| const str = "let's play tag"; | ||
| const char = " "; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(2); | ||
| }); | ||
| test("should count multiple occurences of character in a sentence", () => { | ||
| const str = "let's play tag"; | ||
| const char = "a"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(2); | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can consider adding tests for an empty string and for a target longer than one character (like "an"), then decide whether the function should handle that or if it’s outside what the function promises. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for adding both tests; this addresses the comment nicely. |
||
| test("should count multiple occurences of character in an empty string", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice addition. Since this test expects 0, it might be worth removing “multiple occurrences” from the name. |
||
| const str = ""; | ||
| const char = "a"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(0); | ||
| }); | ||
| test("should count multiple occurences of character in a sentence", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test seems to share its name with the one on line 52, so it might be worth giving it a unique name that describes its scenario. |
||
| const str = " "; | ||
| const char = " "; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(1); | ||
| }); | ||
| test("should throw an error when findCharacter receives more than one character", () => { | ||
| expect(() => { | ||
| countChar("aaaaaaanaaa", "an"); | ||
| }).toThrow(`Invalid Input,only character is allowed`); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,19 @@ | ||
| export function getOrdinalNumber(num) { | ||
| return "1st"; | ||
| let numStr = num.toString(); | ||
| if (num <= 0 || num == " " || numStr.includes(".")) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good thinking to add validation for 0, negatives, and decimals. Why check for a space " " specifically? It might be worth trying inputs like "abc" or "5" and seeing what the function returns, then deciding which types of input should be accepted. |
||
| throw new Error("Invalid Input"); | ||
| } | ||
|
|
||
| if (numStr.slice(-1) == "1" && numStr.slice(-2) != 11) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. numStr.slice(-2) gives you a string, but it’s compared with the number 11. This currently works because != converts types before comparing. What would happen if you switched to !==, as suggested earlier? It might be worth comparing like with like so the code doesn’t depend on type conversion. Applicable to lines 11 and 14. |
||
| return `${num}st`; | ||
| } | ||
|
|
||
| if (numStr.slice(-1) == "2" && numStr.slice(-2) != 12) { | ||
| return `${num}nd`; | ||
| } | ||
| if (numStr.slice(-1) == "3" && numStr.slice(-2) != 13) { | ||
| return `${num}rd`; | ||
| } | ||
|
|
||
| return `${num}th`; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,8 @@ | ||
| import { getOrdinalNumber } from "./get-ordinal-number.js"; | ||
| // In this week's prep, we started implementing getOrdinalNumber. | ||
|
|
||
| // In this week's prep, we started implementing getOrdinalNumber. | ||
| // Continue testing and implementing getOrdinalNumber for additional cases. | ||
| // Write your tests using Jest — remember to run your tests often for continual feedback. | ||
|
|
||
| // To ensure thorough testing, we need broad scenarios that cover all possible cases. | ||
| // 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 | ||
|
|
@@ -13,8 +12,59 @@ import { getOrdinalNumber } from "./get-ordinal-number.js"; | |
| // Case 1: Numbers ending with 1 (but not 11) | ||
| // When the number ends with 1, except those ending with 11, | ||
| // Then the function should return a string by appending "st" to the number. | ||
|
|
||
| // Case 1: Numbers ending with 1 (but not 11). | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Case 1 comment appears twice (lines 12 and 16), so it might be worth removing one. |
||
| test("should append 'st' for numbers ending with 1, except those ending with 11", () => { | ||
| expect(getOrdinalNumber(1)).toEqual("1st"); | ||
| expect(getOrdinalNumber(21)).toEqual("21st"); | ||
| expect(getOrdinalNumber(131)).toEqual("131st"); | ||
| }); | ||
|
|
||
| // Case 2: Numbers ending with 2(but not 12). | ||
| test("should append 'nd' for numbers ending with 2, except those ending with 12", () => { | ||
| expect(getOrdinalNumber(2)).toEqual("2nd"); | ||
| expect(getOrdinalNumber(22)).toEqual("22nd"); | ||
| expect(getOrdinalNumber(132)).toEqual("132nd"); | ||
| }); | ||
| //Case 3: Number ending with 3(but not 13) | ||
| test("should append 'rd' for numbers ending with 3, except those ending with 13", () => { | ||
| expect(getOrdinalNumber(3)).toEqual("3rd"); | ||
| expect(getOrdinalNumber(23)).toEqual("23rd"); | ||
| expect(getOrdinalNumber(103)).toEqual("103rd"); | ||
| expect(getOrdinalNumber(133)).toEqual("133rd"); | ||
| }); | ||
| //case 4 : Number ending 4,5,6,7,8,9 and 0. | ||
| test("should append 'th' for numbers ending with (4,5,6,7,8,9,0)", () => { | ||
| expect(getOrdinalNumber(4)).toEqual("4th"); | ||
| expect(getOrdinalNumber(10)).toEqual("10th"); | ||
| expect(getOrdinalNumber(15)).toEqual("15th"); | ||
| expect(getOrdinalNumber(58)).toEqual("58th"); | ||
| expect(getOrdinalNumber(100)).toEqual("100th"); | ||
| }); | ||
| //Case 5: the exception numbers left out in all the cases(11,12,13) | ||
| test("should append 'th' for numbers 11, 12, 13", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Try adding a few to this test and see what your function returns. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for adding 111, 112, and 113. This covers the case nicely. |
||
| expect(getOrdinalNumber(11)).toEqual("11th"); | ||
| expect(getOrdinalNumber(12)).toEqual("12th"); | ||
| expect(getOrdinalNumber(13)).toEqual("13th"); | ||
| expect(getOrdinalNumber(111)).toEqual("111th"); | ||
| expect(getOrdinalNumber(112)).toEqual("112th"); | ||
| expect(getOrdinalNumber(113)).toEqual("113th"); | ||
| }); | ||
|
|
||
| //case 6 now we have come to the last one 0 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This says “the last one”, but there are more cases after it, so it might be worth updating the comment. |
||
| test(` should throw an error when num is 0`, () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice work covering the main cases. One more you might like to explore is what happens when str is an empty string. Adding a test for it could be a helpful addition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding the extra validation tests. The tests you’ve added for invalid input are a great addition. |
||
| expect(() => getOrdinalNumber(0)).toThrow(`Invalid Input`); | ||
| }); | ||
|
|
||
| //case 7 how about when empty string is the argument | ||
| test(`Should throw an error when num is empty string`, () => { | ||
| expect(() => getOrdinalNumber(" ")).toThrow(`Invalid Input`); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test name says “empty string”, but the input is a single space " ". It might be worth checking which case you meant to test or adding tests for both. |
||
| }); | ||
| // Case 8 how about when num is -ve number | ||
| test(`should throw an Error when num is -ve number`, () => { | ||
| expect(() => getOrdinalNumber(-5).toThrow(`Invalid Input`)); | ||
| }); | ||
| //case 9 when num is decimal number | ||
| test(`Should throw an Error when num is decimal number`, () => { | ||
| expect(() => getOrdinalNumber(1.5).toThrow(`Invalid Input`)); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,23 @@ | ||
| export function repeatStr() { | ||
| // Your implementation of this function must *not* call String.prototype.repeat (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/repeat). | ||
| export function repeatStr(str, count) { | ||
| // Your implementation of this function must *not* call String.prototype.repeat | ||
| // (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/repeat). | ||
| // The goal is to re-implement that function, not to use it. | ||
| return "hellohellohello"; | ||
|
|
||
| let repeatedStr = str; | ||
| count = Math.floor(count); | ||
|
|
||
| if (str === " ") { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This checks whether str is a single space " ", but the error message says the string should not be empty. A space isn’t an empty string, so it might be worth checking which case you meant. It’s also worth thinking about whether repeating a space, or an empty string, should actually be an error. What does the built-in repeat return for those? |
||
| throw new Error("String shoild not be empty"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small typo: “shoild” should be “should”. |
||
| } | ||
| if (count < 0) { | ||
| throw new Error("Negative counts are not valid"); | ||
|
Comment on lines
+12
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice work handling the negative count with a clear error message. What happens if count is a decimal, like 2.5? Decide what the function should do and add a test for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for handling decimals with Math.floor, that’s a sensible choice, and it matches how the built-in repeat behaves. |
||
| } | ||
| if (count == 0) { | ||
| return ""; | ||
| } | ||
|
|
||
| for (let i = 1; i < count; i++) { | ||
| repeatedStr += str; | ||
| } | ||
| return repeatedStr; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,24 +9,52 @@ import { repeatStr } from "./repeat-str.js"; | |
| // When the repeatStr function is called with these inputs, | ||
| // Then it should return a string that contains the original `str` repeated `count` times. | ||
|
|
||
| test("should repeat the string count times", () => { | ||
| // Case: handle count of 1, 2, 3, 4: | ||
| test("should repeat the string 1 times", () => { | ||
| const str = "hello"; | ||
| const count = 1; | ||
| const repeatedStr = repeatStr(str, count); | ||
| expect(repeatedStr).toEqual("hello"); | ||
| }); | ||
| test("should repeat the string 2 times", () => { | ||
| const str = "hello"; | ||
| const count = 2; | ||
| const repeatedStr = repeatStr(str, count); | ||
| expect(repeatedStr).toEqual("hellohello"); | ||
| }); | ||
| test("should repeat the string 3 times", () => { | ||
| const str = "hello"; | ||
| const count = 3; | ||
| const repeatedStr = repeatStr(str, count); | ||
| expect(repeatedStr).toEqual("hellohellohello"); | ||
| }); | ||
|
|
||
| // Case: handle count of 1: | ||
| // Given a target string `str` and a `count` equal to 1, | ||
| // When the repeatStr function is called with these inputs, | ||
| // Then it should return the original `str` without repetition. | ||
|
|
||
|
Comment on lines
-18
to
-23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Given/When/Then comments for the count of 1 and count of 0 cases are still removed. It might be worth restoring them, as you’ve done for the other cases. |
||
| test("should repeat the string 4 times", () => { | ||
| const str = "hello"; | ||
| const count = 4; | ||
| const repeatedStr = repeatStr(str, count); | ||
| expect(repeatedStr).toEqual("hellohellohellohello"); | ||
| }); | ||
| // Case: Handle count of 0: | ||
| // Given a target string `str` and a `count` equal to 0, | ||
| // When the repeatStr function is called with these inputs, | ||
| // Then it should return an empty string. | ||
|
|
||
| test("should repeat the string 0 times", () => { | ||
| const str = "hello"; | ||
| const count = 0; | ||
| const repeatedStr = repeatStr(str, count); | ||
| expect(repeatedStr).toEqual(""); | ||
| }); | ||
| //case: Handle decimal count: | ||
| test("should repeat the stringthe whole number in the count times", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There’s a missing space in “stringthe”. It might also help to make the name clearer, for example, by saying that a decimal count is rounded down. |
||
| const str = "hello"; | ||
| const count = 2.5; | ||
| const repeatedStr = repeatStr(str, count); | ||
| expect(repeatedStr).toEqual("hellohello"); | ||
| }); | ||
| // Case: Handle negative count: | ||
| // Given a target string `str` and a negative integer `count`, | ||
| // When the repeatStr function is called with these inputs, | ||
| // Then it should throw an error, as negative counts are not valid. | ||
| // Then it should throw an error, as negative counts are not valid | ||
| test("should throw an error when -1 count is given", () => { | ||
| expect(() => repeatStr("hello", -1)).toThrow("Negative counts are not valid"); | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be worth thinking about what should happen when str is an empty string. You might consider adding a test for that case, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for adding this test. One thing to check: the input here is a single space " " rather than an empty string "". Please see my comment on line 9 of repeat-str.js, as the two are linked. |
||
| test(`should throw an error when string is empty`, () => { | ||
| expect(() => repeatStr(" ", 1)).toThrow("String shoild not be empty"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test name says “empty string”, but the input is a space. Once you’ve decided whether an empty string or a space should be an error, it might be worth updating this test to match. Also, if you fix the “shoild” typo in the error message, remember to update it here too. |
||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing each character to findCharacter[0] works well when the target is a single character, as the exercise describes. It might be interesting to try passing something like "an" and see what happens. You could then decide whether the function should handle that case or whether it’s outside what the function promises, and add a test that documents your decision. A test for an empty string could also be a helpful way to confirm the behaviour you expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing this. Adding the length check makes the function’s contract clear, and throwing an error for invalid input is a good decision.