Card
class
Specify the access modifiers of the class and members properly.
Use proper names for members:
class Card{
Suit s;
Rank r;
for example:
public class Card{
private Suit suit;
private Rank rank;
print()
method
It's better to only create a String
representation of an object and return that. You override toString()
method which will be used implicitly if the object has to be converted to a String
.
It's good practice to override toString()
for all custom classes, because this is where people will look for when searching for a String
representation of the object. You called it print()
, or was it info()
? , who knows? Your future self? Probably not. toString()
is the way to go.
Your current approach appears to be more convenient, but actually causes some trouble like duplicated code:
System.out.print("Rank: "+c.getRank()+" Suit: "+c.getSuit()+"\t");
System.out.println("Card: "+card.getRank()+" Suit: "+card.getSuit());
Card.toString()
could look like this:
@Override
public String toString()
{
return "rank: " + rank + "\t suit: " + suit;
}
CardDeck
is pretty much a collection of a number of cards. CardDeck.toString()
could look something like this:
@Override
public String toString()
{
String result = cards.size() + " cards:" + System.lineSeparator();
for (Card card : cards)
{
result = result.concat(card + System.lineSeparator());
}
return result;
}
Here's what both classes look like so var with a little example program:
Card.java
public class Card
{
private Suit suit;
private Rank rank;
public enum Suit
{
Spade , Heart , Diamond , Clubs
}
public enum Rank
{
ACE(1) , TWO(2), THREE(3), FOUR(4), FIVE(5) , SIX(6), SEVEN(7), EIGHT(8), NINE(9), TEN (10), JACK(11), QUEEN (12), KING(13);
int priority;
Rank(int s)
{
priority = s;
}
public int getPriority()
{
return priority;
}
public Rank getRankByPriority(int p)
{
switch (p)
{
case 1: return Rank.ACE;
case 2: return Rank.TWO;
case 3: return Rank.THREE;
case 4: return Rank.FOUR;
case 5: return Rank.FIVE;
case 6: return Rank.SIX;
case 7: return Rank.SEVEN;
case 8: return Rank.EIGHT;
case 9: return Rank.NINE;
case 10: return Rank.TEN;
case 11: return Rank.JACK;
case 12: return Rank.QUEEN;
case 13: return Rank.KING;
default: return null;
}
}
}
Card(Rank rank, Suit suit)
{
this.rank = rank;
this.suit = suit;
}
Rank getRank()
{
return rank;
}
Suit getSuit()
{
return suit;
}
@Override
public String toString()
{
return "rank: " + rank + "\t suit: " + suit;
}
}
CardDeck.java
import java.util.ArrayList;
public class CardDeck
{
private ArrayList<Card> cards;
CardDeck()
{
cards = new ArrayList<>();
for(Card.Suit s: Card.Suit.values())
{
for(Card.Rank r: Card.Rank.values())
{
cards.add(new Card(r,s));
}
}
}
public static void main(String[] args)
{
CardDeck deck = new CardDeck();
System.out.println(deck);
}
@Override
public String toString()
{
String result = cards.size() + " cards:" + System.lineSeparator();
for (Card card : cards)
{
result = result.concat(card + System.lineSeparator());
}
return result;
}
}
result (excerpt):
$ java CardDeck
52 cards:
rank: ACE suit: Spade
rank: TWO suit: Spade
rank: THREE suit: Spade
rank: FOUR suit: Spade
rank: FIVE suit: Spade
rank: SIX suit: Spade
rank: SEVEN suit: Spade
rank: EIGHT suit: Spade
rank: NINE suit: Spade
rank: TEN suit: Spade
...
shuffle()
method
A method is a block of code that operates on an object. Your shuffle()
method might work on an optional parameter which is entirely independent from the CardDeck
object. The part of the method that operates on an arbitrary ArrayList<Card>
should be static
.
public void shuffle(ArrayList<Card> c){
if(c==null){
Collections.shuffle(cards);
}else{
Collections.shuffle(c);
}
}
Thus splits into:
public static void shuffle(ArrayList<Card> cards)
{
if(cards!=null)
{
Collections.shuffle(cards);
}
}
public void shuffle()
{
Collections.shuffle(cards);
}
The second one makes a lot of sense, but the static version does not. What's the point of calling this method directly when you already have Collections.shuffle()
?
Also, the static version has nothing to do with the CardDeck
class whatsoever. It should not be a member of CardDeck
, but either Card
or better a separate class with utility functionality named CardUtils
for example. But again, there's no real point in having that functionality that does not add anything to Collections.shuffle()
. Delete it.
deal()
method
I have no idea what this is doing by only looking at it. There are so many card games and all have their own ways to deal cards. Does every player get the same number of cards? Or are all cards distributed to a number of players?
Trying your code it looks like the later is true.
The problem with this method is that it does not return anything. It only prints the piles of card out, without any chance to do anything with them. Instead, provide a useful return value you can work with.
One such return value could be CardDeck
. There's a bit of functionality already in there, so why not reuse that? Add a constructor to be able to pass a Collection
of cards to it:
CardDeck(Collection<? extends Card> cards)
{
this.cards = new ArrayList<Card>(cards);
}
When dealing the cards, there's no real point in dealing them one by one. You know how many cards each player receives and should just get that many from the shuffled cards. Think about it like every player takes a certain number of cards from the deck.
public ArrayList<CardDeck> dealAllCards(int numberOfPlayers)
{
ArrayList<CardDeck> playerHands = new ArrayList<>(numberOfPlayers);
ArrayList<Card> shuffledDeck = new ArrayList<>(cards);
int cardsPerPlayer = shuffledDeck.size()/numberOfPlayers; // to be subtracted by 1 after dealing remaining cards
int remainingCards = shuffledDeck.size()%numberOfPlayers;
Collections.shuffle(shuffledDeck);
for (int player = 0; player < numberOfPlayers; ++player)
{
if (player == remainingCards)
{
--cardsPerPlayer; // all remaining cards dealed
}
CardDeck hand = new CardDeck(shuffledDeck.subList(0, cardsPerPlayer + 1));
shuffledDeck.removeAll(hand.cards);
playerHands.add(hand);
}
return playerHands;
}
The full code now looks like this:
Card.java
did not change, see above
CardDeck.java
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
public class CardDeck
{
private ArrayList<Card> cards;
public static void main(String[] args)
{
CardDeck deck = new CardDeck();
ArrayList<CardDeck> playerHands = deck.dealAllCards(5);
for(CardDeck playerHand : playerHands)
{
System.out.println(playerHand);
}
}
CardDeck()
{
cards = new ArrayList<>();
for(Card.Suit s: Card.Suit.values())
{
for(Card.Rank r: Card.Rank.values())
{
cards.add(new Card(r,s));
}
}
}
CardDeck(Collection<? extends Card> cards)
{
this.cards = new ArrayList<Card>(cards);
}
public ArrayList<CardDeck> dealAllCards(int numberOfPlayers)
{
ArrayList<CardDeck> playerHands = new ArrayList<>(numberOfPlayers);
ArrayList<Card> shuffledDeck = new ArrayList<>(cards);
int cardsPerPlayer = shuffledDeck.size()/numberOfPlayers; // to be subtracted by 1 after dealing remaining cards
int remainingCards = shuffledDeck.size()%numberOfPlayers;
Collections.shuffle(shuffledDeck);
for (int player = 0; player < numberOfPlayers; ++player)
{
if (player == remainingCards)
{
--cardsPerPlayer; // all remaining cards dealed
}
CardDeck hand = new CardDeck(shuffledDeck.subList(0, cardsPerPlayer + 1));
shuffledDeck.removeAll(hand.cards);
playerHands.add(hand);
}
return playerHands;
}
public void shuffle()
{
Collections.shuffle(cards);
}
@Override
public String toString()
{
String result = cards.size() + " cards:" + System.lineSeparator();
for (Card card : cards)
{
result = result.concat(card + System.lineSeparator());
}
return result;
}
}
full example output:
$ java CardDeck
11 cards:
rank: FOUR suit: Clubs
rank: FIVE suit: Heart
rank: FOUR suit: Spade
rank: ACE suit: Heart
rank: EIGHT suit: Heart
rank: SIX suit: Heart
rank: JACK suit: Spade
rank: EIGHT suit: Diamond
rank: JACK suit: Clubs
rank: QUEEN suit: Clubs
rank: TWO suit: Spade
11 cards:
rank: SEVEN suit: Heart
rank: NINE suit: Spade
rank: SEVEN suit: Diamond
rank: SEVEN suit: Clubs
rank: SIX suit: Clubs
rank: ACE suit: Clubs
rank: KING suit: Heart
rank: SEVEN suit: Spade
rank: FIVE suit: Clubs
rank: FOUR suit: Diamond
rank: TEN suit: Diamond
10 cards:
rank: KING suit: Clubs
rank: TEN suit: Clubs
rank: ACE suit: Diamond
rank: QUEEN suit: Spade
rank: SIX suit: Diamond
rank: FIVE suit: Spade
rank: EIGHT suit: Spade
rank: QUEEN suit: Heart
rank: FOUR suit: Heart
rank: KING suit: Diamond
10 cards:
rank: THREE suit: Clubs
rank: EIGHT suit: Clubs
rank: THREE suit: Diamond
rank: KING suit: Spade
rank: TWO suit: Clubs
rank: JACK suit: Heart
rank: THREE suit: Spade
rank: THREE suit: Heart
rank: JACK suit: Diamond
rank: TWO suit: Diamond
10 cards:
rank: NINE suit: Heart
rank: NINE suit: Clubs
rank: TEN suit: Heart
rank: ACE suit: Spade
rank: NINE suit: Diamond
rank: TEN suit: Spade
rank: FIVE suit: Diamond
rank: QUEEN suit: Diamond
rank: TWO suit: Heart
rank: SIX suit: Spade
further improvements from the comments
Thanks @Boris!
String.concat
is conventionally written as +; this is never used in a loop - a StringBuilder
(or StringWriter
) is used instead.
That's right. CardDeck.toString()
could look like:
@Override
public String toString()
{
StringBuilder result = new StringBuilder(cards.size() + " cards:" + System.lineSeparator());
for (Card card : cards)
{
result.append(card + System.lineSeparator());
}
return result.toString();
}
This is great, because it also helps readability. It's not just some String
, but a StringBuilder
, which is a bit more self-documenting.
Given that cards
is immutable (no public acces and not private changes), this String
could even be built beforehand and cached in a private member. As I don't think the toString()
method will be called in such quantities that it would make a significant difference, I leave such and further optimisations of the method to the interested reader.
Your deal method is very inefficient; List.removeAll
will call remove
on each of the elements, which is in turn going to loop through the List
to find the element; this is O(n*k)
- very bad. Instead use List.subList.clear()
after you create the CardDeck
for the player.
True, I was more concerned about being close to the real world situation that the code is modelling, that is: tacking a number of cards from a set of cards.
How to solve that problem? Not at all. They say it's clever to solve a problem, but wise to avoid it. Let's simply not remove anything at all.
Instead, move the parameters for subList()
, which essentially has the same effect. The improved version of dealAllCards()
could look like this:
public ArrayList<CardDeck> dealAllCards(int numberOfPlayers)
{
ArrayList<CardDeck> playerHands = new ArrayList<>(numberOfPlayers);
ArrayList<Card> shuffledDeck = new ArrayList<>(cards);
int cardsPerPlayer = shuffledDeck.size()/numberOfPlayers; // to be subtracted by 1 after dealing remaining cards
int remainingCards = shuffledDeck.size()%numberOfPlayers; // #cards is not a multiple of #players
Collections.shuffle(shuffledDeck);
int firstCardIndex = 0;
int lastCardIndex = 0;
for (int player = 0; player < numberOfPlayers; ++player)
{
if (player == remainingCards)
{
--cardsPerPlayer; // all remaining cards dealed
}
lastCardIndex = firstCardIndex + cardsPerPlayer + 1;
playerHands.add(new CardDeck(shuffledDeck.subList(firstCardIndex, lastCardIndex)));
firstCardIndex = lastCardIndex;
}
return playerHands;
}
shuffledDeck
is just a local temporary helper to hold the shuffled cards, after all.