Skip to content
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

LONDON_JAN25 | GIOSEF FERRARO | STRUCTURING_AND_TESTING_DATA | SPRINT 3 #405

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Giosef92
Copy link

Self checklist

  • [X ] I have committed my files one by one, on purpose, and for a reason
  • [X ] I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • [X ] I have tested my changes
  • [X ] My changes follow the style guide
  • [X ] My changes meet the requirements of this task

@Giosef92 Giosef92 added the 👀 Review Requirements Changes requested to meet requirements label Mar 16, 2025
Copy link

Choose a reason for hiding this comment

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

Code looks fine

Copy link

Choose a reason for hiding this comment

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

Code looks fine

Copy link

Choose a reason for hiding this comment

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

Code is fine

Copy link

Choose a reason for hiding this comment

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

Code is fine

@fearcyf fearcyf self-assigned this Mar 31, 2025
Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

do about three expect statements for each condition to strengthen your tests

Copy link

Choose a reason for hiding this comment

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

Could you explain your reasoning in a bit more detail - in particular your parseInt approach

Copy link

Choose a reason for hiding this comment

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

This relates to line 10 - the if statement How do we ensure that rank returns single digits so that something like 2.2 doesn't slip through. See the link above

Copy link

Choose a reason for hiding this comment

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

Could a for loop be written for tests to return number cards, have a look at this link -> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Loops_and_iteration

Copy link

Choose a reason for hiding this comment

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

how could your Function be improved so that -> 111, 912, 413 returns "111th", "912th", "413th" respectively.

@fearcyf
Copy link

fearcyf commented Apr 1, 2025

Good work overall - areas to be addressed has been commented on.

@fearcyf fearcyf added Reviewed Volunteer to add when completing a review and removed 👀 Review Requirements Changes requested to meet requirements labels Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants