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

West Midlands | Alireza Seifi Shalamzari | Module-Structuring-and-Testing-Data | Week 3 #231

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

Conversation

Alireza1620
Copy link

@Alireza1620 Alireza1620 commented Dec 16, 2024

Self checklist

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

Changelist

This pull request is created to show the completion of Sprint 3

Questions

Notes

For the file Rotate Char, it was too hard to do!

Copy link

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

The code in the 4 .js files you implemented are quite solid.

You also prepared many good test cases but you were not using console.assert() or Jest to specify the tests.

There are a few more exercises in folder revised not yet included in this branch.

Do you need help setting up Jest or using it to specify unit tests? You can seek help from a volunteer in a workshop on Saturday or DM me on Slack to make an appointment. You can also do the same if you need some guidance in these exercises.

Comment on lines 61 to 62
console.log(getAngleType(0)); // Should print "Invalid angle"
console.log(getAngleType(-45)); // Should print "Invalid angle"
Copy link

Choose a reason for hiding this comment

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

Does these two show "Invalid angle"?

It is not easy to spot the error from just console.log() output. That's one reason why a test framework like Jest can help us identify cases when a return value does not match an expected value.

Copy link
Author

Choose a reason for hiding this comment

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

I changed the testing method to Jest

const rank = card.slice(0, -1);

// Handle Number Cards (2-9)
if (rank >= "2" && rank <= "9") {
Copy link

Choose a reason for hiding this comment

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

Todo

This check is not solid enough.
You can test your function by calling getCardValue("23♠") or getCardValue("899♠") to see what they return.

Copy link
Author

Choose a reason for hiding this comment

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

I did work on this and I found a solution!
I added a set of valid ranks so only the valid ones can return an actual value

//Testing the function
console.log(isProperFraction(2, 3)); //should return True
console.log(isProperFraction(5, 2)); //should return False
console.log(isProperFraction(-4, 7)); //should return True
Copy link

Choose a reason for hiding this comment

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

In mathematics, -4/7 == 4/-7, and -4/-7 == 4/7.
So, ideally isProperFraction() should recognise all of them as proper fractions.

Hint: If you compute the absolute value of both parameters inside the function first, the code can become much simpler.

Copy link
Author

Choose a reason for hiding this comment

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

I added a part which first make absolute numbers then check them

@cjyuan cjyuan added 👀 Review Requirements Changes requested to meet requirements Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 Review Requirements Changes requested to meet requirements Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants