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.

Sign up for GitHub

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

Jump to bottom

deck: completed function to create and shuffle a UNO card deck. #30

Merged
merged 1 commit into from
Jun 3, 2024
Merged

deck: completed function to create and shuffle a UNO card deck. #30

merged 1 commit into from
Jun 3, 2024

Conversation

sethdivyansh
Copy link
Contributor

@sethdivyansh sethdivyansh commented May 31, 2024 •
edited
Loading

Description

  1. Added black to CardColor in types.d.ts.
  2. Split special and wild cards into specialValues and wildCardValues arrays.
  3. For each color in the colors array:
  • Add two cards for each number (0-9), except one card for '0'.
  • Add two cards for each special value.
  • For 'black', add four cards for each wild card value.

Fixes: #1

Checklist

  • I have tested these changes locally.
  • I have reviewed the code and ensured it follows the project's coding guidelines.
  • I have updated the documentation, if necessary.
  • I have assigned reviewers to this pull request.

@sethdivyansh sethdivyansh force-pushed the fix/issue-#1 branch 4 times, most recently from 25181d0 to 31b341a Compare June 1, 2024 20:22
@kuv2707
Copy link
Collaborator

kuv2707 commented Jun 2, 2024

Can you edit the commit message to occupy less width per line?

@kuv2707 kuv2707 mentioned this pull request Jun 2, 2024
Closed
@sethdivyansh
Copy link
Contributor Author

Can you edit the commit message to occupy less width per line?

I have shortened the commit message.

@kuv2707
Copy link
Collaborator

kuv2707 commented Jun 2, 2024

Great work!
It would be further great if you could add some tests to verify if the array generated has all the cards in the right amount.
Feel free to take the test cases from this PR if you don't feel like writing it yourself (do credit the author for it though).

kuv2707
kuv2707 requested changes Jun 2, 2024
View reviewed changes
Copy link
Collaborator

@kuv2707 kuv2707 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Posted some minor comments.
Overall, this seems promising and can be merged once the comments are addressed and tests are added.

PS. the commit message is perfect. Kudos for that!

backend/uno-game-engine/deck.ts Outdated
const deck = [];
const specialValues: Array<SpecialCardNames> = ['skip', 'reverse', 'draw2'];
const wildCardValue: Array<SpecialCardNames> = ['colchange', 'draw4'];
const deck: Array<UNOCard> = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this variable is useless.
You can go ahead and remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would also make the eslint-backend check pass.

backend/uno-game-engine/deck.ts Outdated
@@ -1,5 +1,5 @@
const colors: Array<CardColor> = ['red', 'yellow', 'green', 'blue'];
const values = [
const numValues: Array<CardNumbers> = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make CardNumbers singular.

backend/uno-game-engine/deck.ts Outdated
];
const specialCards = ['wild', 'draw4'];
const deck = [];
const specialValues: Array<SpecialCardNames> = ['skip', 'reverse', 'draw2'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here too, types should be singular, unless they are an array

backend/uno-game-engine/deck.ts Show resolved Hide resolved
backend/uno-game-engine/deck.ts Outdated
const specialCards = ['wild', 'draw4'];
const deck = [];
const specialValues: Array<SpecialCardNames> = ['skip', 'reverse', 'draw2'];
const wildCardValue: Array<SpecialCardNames> = ['colchange', 'draw4'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

The variable name should be plural.

@kuv2707 kuv2707 added the under review There have been some rounds of review on the code changes in the PR. label Jun 2, 2024
@sethdivyansh
deck: completed function to create and shuffle a UNO card deck.
e5cfedf 
1. Added black to CardColor in types.d.ts.
2. Split special and wild cards into specialValues and wildCardValues arrays.
3. For each color in the colors array:
 - Add two cards for each number (0-9), except one card for '0'.
 - Add two cards for each special value.
 - For 'black', add four cards for each wild card value.
4. Added test,in deck.test.ts, to check the correct functioning of getShuffledCardDeck()
Fixes: shivansh-bhatnagar18#1
@sethdivyansh
Copy link
Contributor Author

@kuv2707 Please check I have made the desired changes and added a test to validate the created function.

kuv2707
kuv2707 approved these changes Jun 3, 2024
View reviewed changes
Copy link
Collaborator

@kuv2707 kuv2707 left a comment

Choose a reason for hiding this comment

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

Looks great!

backend/tests/deck.test.ts
getShuffledCardDeck,
} from '../uno-game-engine/deck';

describe('getShuffledCardDeck', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Appreciate the thorough testing!

@kuv2707 kuv2707 merged commit 2e865b9 into shivansh-bhatnagar18:master Jun 3, 2024
5 of 6 checks passed
@kuv2707
Copy link
Collaborator

kuv2707 commented Jun 3, 2024

Merged, thanks @sethdivyansh !
This is a huge progress and unblocks further work on the engine.
Also, good to see all the backend checks passing.
The prettier test was failing due to a small error from my side.

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

@kuv2707 kuv2707 kuv2707 approved these changes

Assignees
No one assigned
Labels
under review There have been some rounds of review on the code changes in the PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement the Card array generation logic
2 participants
@sethdivyansh @kuv2707