Skip to content

Emin Akturk| West Midlands| MAY | Module-Structuring-data| Coursework-Sprint3 #692

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

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

eminakturk
Copy link

@eminakturk eminakturk commented Aug 4, 2025

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

@eminakturk eminakturk added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 4, 2025
@jenny-alexander jenny-alexander self-requested a review August 11, 2025 12:14
@jenny-alexander jenny-alexander added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 11, 2025
Copy link

@jenny-alexander jenny-alexander left a comment

Choose a reason for hiding this comment

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

You have done a lot of work in this PR! Good job.

In order for me to set the status to complete, there are a few fixes to make (please see comments left throughout the PR).

In addition to the comments that I left:

  • I noticed that a number of tests are failing. It's really important to run the tests yourself and make sure they pass before you submit your PR for review. When you submit a PR with failing tests, it means that your code implementation is 'broken'.

  • The files within Sprint-1 are set to be deleted. This will delete the files in the main repository (and for everyone 😱). I think you did this because you made changes to the file by mistake, is that correct? Since it was a mistake, can you revert these changes instead of deleting the files?
    One way to do this is to revert each file to the commit SHA before your deletion commit SHA. Let me know if you can figure out how to do this or if you would like some additional explanation.

Choose a reason for hiding this comment

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

I think you may be missing a few edge cases in your getAngleType function.

  • What if we don't pass an angle value? const nothing = getAngleType();
  • What if we pass a value that isn't a valid angle value? const tooBig = getAngleType(361);

Choose a reason for hiding this comment

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

If you run this file to see if your tests pass, what happens? I get an error related to the assertEquals function. Can you find the error and fix it?

fyi: You can test the file by opening your terminal (making sure you are in the Sprint-3/1-key-implement directory) and typing:
node 2-is-proper-fraction.js

Choose a reason for hiding this comment

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

There may be a few edge cases missed here.

  • I expected the "Invalid card rank." assertion to be true for: const oddValue = getCardValue("345");
  • I expected the "Invalid card rank." assertion to be true for: const onlySuit = getCardValue("♠");
  • I expected the "Invalid card rank." assertion to be true for: const oneHundred = getCardValue("100♠");

Choose a reason for hiding this comment

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

I think you may be missing a few edge cases in your getAngleType function.

  • What if we don't pass an angle value? const nothing = getAngleType();
  • What if we pass a value that isn't a valid angle value? const tooBig = getAngleType(361);

Choose a reason for hiding this comment

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

Did you run the tests in this file? I get an error when I do, can you find it and fix it?

// Case 5: Handle Invalid Cards:
test("should throw error for invalid cards", () => {

Choose a reason for hiding this comment

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

Can you review the test at line 25? The assertion fails.

Choose a reason for hiding this comment

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

I suggest adding tests for these edge cases:

  • card value: 345
  • card value: ♠
  • card value: 100♠

Choose a reason for hiding this comment

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

Here are a few ideas to test edge cases.

  • Is your countChar function count characters even if the case (i.e. lowercase or uppercase) is different? Can you add a test to check this?
  • Does your countChar function count spaces as a character? Can you add a test to check this?

Choose a reason for hiding this comment

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

It looks like you didn't finish adding your tests to this file.
["th", "st", "nd", "rd"];

"th" is tested, but what about "st", "nd" and "rd"?

Choose a reason for hiding this comment

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

I only see the CYF provided test assertion in repeat.test.js file. Can you add more assertions for your repeat function?

@jenny-alexander jenny-alexander removed the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Aug 12, 2025
@jenny-alexander jenny-alexander added the Reviewed Volunteer to add when completing a review with trainee action still to take. label Aug 12, 2025
@jenny-alexander
Copy link

@eminakturk - I noticed that your checklist is not quite right.
You have this: [x ]
😃 Instead, do this: [x]
(notice how I don't have a space around the x?)
It will format it to a checked off item.

@eminakturk
Copy link
Author

Hello Jenny, Thank you for your great insights. I have completed fixing all the errors you pointed out. I hope and believe I did an okay job. Please let me know In case if I need to change anything else 😄

@eminakturk eminakturk added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 12, 2025
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. Reviewed Volunteer to add when completing a review with trainee action still to take.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants