West Midlands | Sept-ITP-25 | Mustaf Asani | Sprint 3 | Coursework/sprint 3 practice tdd#813
West Midlands | Sept-ITP-25 | Mustaf Asani | Sprint 3 | Coursework/sprint 3 practice tdd#813asaniDev wants to merge 12 commits intoCodeYourFuture:mainfrom
Conversation
…he function that checks for that
…get the ordinal numbers
…t takes in a string and count to create a new string
Sprint-3/2-practice-tdd/repeat.js
Outdated
| } else { | ||
| return "Error invalid count used, please use integers from 0 upwards."; | ||
| } |
There was a problem hiding this comment.
How would the caller distinguish the result of the following two function calls?
repeat("Error invalid count used, please use integers from 0 upwards.", 1)repeat("", -1)
Both function calls return the same value.
There was a problem hiding this comment.
If the input is invalid (negative integers), how can we tell the user that there's an issue instead of returning a response?
…s to count test file
…sts to check that different numbers work
|
Did you forget to push the changes to GitHub? |
Sprint-3/2-practice-tdd/repeat.js
Outdated
| } else { | ||
| return "Error invalid count used, please use integers from 0 upwards."; | ||
| } |
There was a problem hiding this comment.
If the input is invalid (negative integers), how can we tell the user that there's an issue instead of returning a response?
| return "3rd"; | ||
| } else if (num > 3 || num < 21) { | ||
| return `${num}th`; | ||
| const lastDigit = num.toString()[num.toString().length - 1]; |
There was a problem hiding this comment.
Could consider using the more efficient approach involving the % operator to extract the last digit and the last two digits from a number directly.
There was a problem hiding this comment.
used the % to have less code and less function calls
| 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"); | ||
| }); |
There was a problem hiding this comment.
These tests are quite comprehensive. If you use this test script to test your implementation, you can discover some bug in your code.
There was a problem hiding this comment.
added code to catch negative and non-integer values
Sprint-3/2-practice-tdd/repeat.js
Outdated
| ); | ||
| //return "Error invalid count used, please use integers from 0 upwards."; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
-
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;
There was a problem hiding this comment.
refactored the code to reduce the number of if else statements.
cjyuan
left a comment
There was a problem hiding this comment.
There is a bug in the getOrdinalNumber(). Other changes look good.
| 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"); | ||
| }); |
There was a problem hiding this comment.
If you add enough samples in this categories, you may discover a bug in your implementation.
There was a problem hiding this comment.
i found a bug, in the code for values ending in 11 i had only covered for 11 not other numbers ending in 11.
cjyuan
left a comment
There was a problem hiding this comment.
Good job.
That's why a comprehensive test is important.
Learners, PR Template
Self checklist
Changelist
Created tests to check the expected output of functions before they are created.