generated from CodeYourFuture/Module-Template
-
-
Notifications
You must be signed in to change notification settings - Fork 241
London | 25-ITP-Sep | Adnaan Abo | Sprint 3 | coursework/sprint-3-implement-and-rewrite #713
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
AdnaanA
wants to merge
13
commits into
CodeYourFuture:main
Choose a base branch
from
AdnaanA:coursework/sprint-3-implement-and-rewrite
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
3cbfc96
implemented test cases and test passed
AdnaanA 61f0da7
created test cases for fractions and tested them
AdnaanA f6e6f38
created testcases for get card value and tested with a pass
AdnaanA 6fe6458
created testcases for get angle-type and tested to pass
AdnaanA b07437b
created testcases for proper-fractions, cases tested and passed.
AdnaanA 6152bda
test-cases written and passed for get card value.
AdnaanA d8e8c60
modified: Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-wit…
AdnaanA 48c7af8
modified: Sprint-3/2-practice-tdd/count.test.js
AdnaanA 7f195fc
updated and simplified the isproperfraction function
AdnaanA db40788
changed the custom error class to embed the invalid card rank
AdnaanA 91ced4e
modified case 5 handle invalid cards
AdnaanA 991180d
changed the expected error message to include "1"
AdnaanA d6b9753
test case fixed and test passed
AdnaanA File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could have handled "10" in either the K/Q/J case, or the 2-9 case - which do you prefer? What advantages/disadvantages do you think each has?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello,
Thank you for the feedback, I prefer to handle the "10" in the K/Q/J case. for clarity and explicitness, and to maintain semantical grouping since "10" has the same card value as the face cards. a disadvantage could be mixing string handling logic "K/Q/J" with numeric checking logic "10".
We can also handle "10" in the 2-9 case and the pros for this would be logical consistency the numeric branch handles anything numeric between 2-10. and its easier to extend if we ever want to adjust the ranges to include 1 or 11 or validate different ranges we just change the numeric branch. Disadvantages risk of mis-parsing since !isNaN(rank) is loose we might accidently accept strings like "10abc". Edge-case handling we need to make sure to handle trimming, ect. in case someone passes weird strings like "010" and " 10 ".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks for sharing your thoughts!
Personally I prefer 10 to be included in the number case, because it leads to simpler code. In the numeric case, we already need an upper bound, so the difference between
Number(rank) <= 9andNumber(rank) <= 10is really small, whereas adding a whole extra element to the array we're checking is adding an extra piece:["K", "Q", "J", "10"]vs["K", "Q", "J"].But you're right that there's not one obviously better answer - there are advantages and disadvantages to each :)