Birmingham | 25-ITP-Sep | Ahmad Ehsas | Sprint 3 | Implement-and-rewrite-tests #735
Conversation
…, and invalid cards
…he goal of this function is to count how many times a specific character appears in a given string
…lues and update the test case.
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
ahmadehsas
changed the title
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
ahmadehsas
changed the title
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
ahmadehsas
added
Needs Review
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
cjyuan
added
Reviewed
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
ahmadehsas
added
Needs Review
|
Good job in keeping the branch updated. Now let's focus on keeping the branch clean. On the upstream branch (CYF's Can you transfer all of your changes to the files in these three subfolders, and then remove all other subfolders in "Sprint-3" folder?
|
cjyuan
added
Reviewed
|
Thank you for your reviewing. I just have a question, should I make another
branch for ‘practice-tdd’ backlog or do that in the same branch?
…On Thu, 30 Oct 2025 at 15:58, CJ Yuan ***@***.***> wrote:
*cjyuan* left a comment
(CodeYourFuture/Module-Structuring-and-Testing-Data#735)
<#735 (comment)>
Good job in keeping the branch updated. Now let's focus on keeping the
branch clean.
On the upstream branch (CYF's main), the subfolders inside "Sprint-3"
folder have been renamed. As a result, the filenames are all "messed up",
and in short, that makes code reviewing difficult.
Can you transfer all of your changes to the files in these three
subfolders, and then remove all other subfolders in "Sprint-3" folder?
- Sprint-3/1-implement-and-rewrite-tests
- Sprint-3/2-practice-tdd
- Sprint-3/3-stretch
—
Reply to this email directly, view it on GitHub
<#735 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BOYJJVD77WICLBREAB6UM2L32IYRHAVCNFSM6AAAAACILF2MFGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTINRYG42DMMJQGI>
.
You are receiving this because you were mentioned.Message ID:
<CodeYourFuture/Module-Structuring-and-Testing-Data/pull/735/c3468746102@
github.com>
|
ahmadehsas
added
Needs Review
Either way works. |
| if (angle === 90) return "Right angle"; | ||
| if (angle < 90) return "Acute angle"; | ||
| if (angle > 90 && angle < 180) return "Obtuse angle"; | ||
| if (angle === 180) return "Straight angle"; | ||
| if (angle > 180 && angle < 360) return "Reflex angle"; | ||
|
|
||
| // read to the end, complete line 36, then pass your test here | ||
|
|
||
| if (angle === 90) { | ||
| return "Right angle"; | ||
| } |
There was a problem hiding this comment.
You have some redundant code here.
| if (Math.abs(numerator) < denominator) return true; // This version of code works correctly for proper and negative fractions. | ||
| if (Math.abs(numerator) >= Math.abs(denominator)) return false; | ||
| if (Math.abs(numerator) === Math.abs(denominator)) return false; | ||
|
|
||
| if (numerator < denominator) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
-
What do you expect from
isProperFraction(-2, -3)? -
Some of the code is redundant.
| if (!isNaN(rank)) { | ||
| return Number(rank); // Number card | ||
| } |
There was a problem hiding this comment.
In JavaScript, strings that represent valid numeric literals in the language can be safely converted to equivalent numbers. For examples, "0x02", "2.1", "9e1" or "0002"
Do you want to recognize these string values as valid ranks?
| test("should return true for negative fractions", () => { | ||
| const negativeFraction = isProperFraction(-4, 7); | ||
| expect(negativeFraction).toEqual(true); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Not all negative fractions are proper fractions. For example, -5/2.
If you allow negative denominator, then you could also test isProperFraction(4, -7), isProperFraction(-4, -7), isProperFraction(-7, 4), isProperFraction(-7, -4), isProperFraction(7, -4).
| test("throws an error or invalid card", () => { | ||
| expect(() => getCardValue("Z♠")).toThrow("Invalid card"); | ||
| }); | ||
|
|
||
| test("should throw error for malformed numeric input", () => { | ||
| expect(() => getCardValue("2.9999♠")).toThrow("Invalid card rank"); | ||
| }); | ||
|
|
||
| test("should throw error for repeated characters in rank", () => { | ||
| expect(() => getCardValue("3AAAA♠")).toThrow("Invalid card rank"); | ||
| }); | ||
|
|
||
| test("should throw error for number beyond valid range", () => { | ||
| expect(() => getCardValue("11♠")).toThrow("Invalid card rank"); | ||
| }); | ||
|
|
||
| test("should throw error for missing suit", () => { | ||
| expect(() => getCardValue("Q")).toThrow("Invalid card rank"); | ||
| }); | ||
|
|
||
| test("should throw error for non-string input", () => { | ||
| expect(() => getCardValue(5)).toThrow("Card must be a string"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
-
Can you current implementation pass all these tests?
-
Why specifically test "repeated characters" in rank? What's so special about this particular string pattern?
-
"2.9999" is not really a malformed numeric value.
-
The function is not expected to check the suit character. Including a test for missing suit might send the wrong message to the person implementing the function.
There was a problem hiding this comment.
The subfolder containing these two files were moved from its original folder.
cjyuan
added
Reviewed
3 participants
Self checklist
Changelist:
I have completed all the mandatory tasks. The changes I have made in the files are as below:
Questions