Skip to content

London | 26-ITP-Sep| Abdennour Hachemi | Sprint 2 | Implement-and-rewrite-tests - #1761

Open
AbdennourHachemi wants to merge 10 commits into
CodeYourFuture:mainfrom
AbdennourHachemi:coursework/sprint-3-implement-and-rewrite
Open

AbdennourHachemi wants to merge 10 commits into
CodeYourFuture:mainfrom
AbdennourHachemi:coursework/sprint-3-implement-and-rewrite

Conversation

@AbdennourHachemi

Copy link
Copy Markdown

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

CYF-1059

@github-actions

This comment has been minimized.

@AbdennourHachemi AbdennourHachemi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 27, 2026

@iscmiguelsamaniego iscmiguelsamaniego left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work overall! Your implementations and test suites show a solid understanding of logic and edge cases.

Before final approval, please clean up any duplicated test blocks, ensure proper closing brackets, and verify function scopes. Excellent progress!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing your tests! You’ve set up a solid foundation covering the main angle categories.

Are there any boundary values or edge cases (such as 0°, 360°, or negative numbers) that might be worth including to ensure the function handles all possible inputs precisely?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid tests! Have you noticed a couple of minor typos in the descriptions, and might negative numbers be worth covering?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test coverage for valid cards and invalid formats! How might the function handle lowercase inputs or missing values?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid implementation! What happens if a negative number is passed into the function?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid implementation! How does the function handle negative numbers, and should they be considered proper fractions in this context?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean regex logic! Notice where the function's closing brace is placed—is all of that implementation code currently inside the function body?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great coverage of the angle types! Have you checked the syntax of the first test block—is anything missing at the end before the next test starts?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test coverage! Have you noticed the duplicated test blocks throughout the file

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensive and thorough test suite covering numbers, face cards, and multiple error formats! Have you considered whether testing the Ace card is also needed in this block?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants