I'm looking for an honest review of what I've got thus far. I'm still trying to get my head wrapped around software development and have picked Java as my starting language. That said, I figured that I would attempt to create a reusable module for which I'd use this as an external library.

I think it would be invaluable to hear from you experts on where I can improve or refactor. The code here is just snippets. Full code on GitHub.

Card.java

package com.frijolie.cards;

public interface Card extends Comparable<Card> 

 // returns the value of the Card. inherently coupled with the Rank
 int getValue();

 // returns the Rank of the Card. Values from Ace,2,3..., Jack,Queen,King
 Rank getRank();

 // returns the Suit of the Card. Values are Diamonds, Hearts, Spades, Clubs
 Suit getSuit();

 // returns the color of the Card. Values are Red or Black
 Color getColor();

 // display the card onscreen
 void displayCard();

 // compares this Card to another, returns if they are equal
 boolean equals(final Object o);

 // Generates a unique int value of a Card
 int hashCode();

 // flips a Card faceUp or faceDown 
 void flip();

 // returns true if this Card is faceUp
 boolean isFaceUp();

Rank.java

package com.frijlie.cards;

public enum Rank 

 ACE("Ace","A",1),
 TWO("Two","2",2),
 THREE("Three","3",3),
 ... // snipped for brevity
 JACK("Jack","J",10),
 QUEEN("Queen","Q",10),
 KING("King","K","10);

 private final String name;
 private final String letter;
 private final int value;

 Rank(final String name, final String letter, final int value) 
 this.name = name;
 this.letter = letter;
 this.value = value;
 

 ... // accessor methods 

Suit.java

package com.frijolie.cards;

public enum Suit 
 CLUBS("Clubs",'u2663', Color.BLACK),
 DIAMONDS("Diamonds",'u2666', Color.RED),
 SPADES("Spades",'u2660', Color.BLACK),
 HEARTS("Hearts",'u2764', Color.RED);

 private final String name;
 private final char symbol;
 private final Color color;

 Suit(final String name, final char symbol, final Color color) 
 this.name = name;
 this.symbol = symbol;
 this.color = color;
 

 ... // accessor methods

Color.java

package com.frijolie.cards;

public enum Color 
 RED("Red"),
 BLACK("Black");

 private final String name;

 Color(final String name) 
 this.name = name;
 

 public final String getName() 
 return name;
 

Deck.java

package com.frijolie;

// snipped imports

public class Deck 

 private boolean isShuffled;
 private Stack<Card> deck;

 // default constructor, will create a single deck of 52 cards
 public Deck() 
 this(1)
 

 // overloaded constructor, will create multiple decks of 52 cards
 public deck(int numberOfDecks) 
 deck = new Stack<>();
 clearDeck();
 for (int i = 0; i < numberOfDecks; i++) 
 populateDeck();
 
 

 // a method to shuffle the collection of cards. default will shuffle 1x
 public void shuffle() 
 shuffle(1);
 

 // overloaded shuffle method. will shuffle the deck a number of times.
 public void shuffle(int numberOfTimes) 
 isShuffled = true;
 for (int i = 0; i < numberOfTimes; i++) 
 Collections.shuffle(deck);
 
 

 // returns true if the deck has been shuffled
 public boolean isShuffled() 
 return isShuffled;
 

 // returns the number of cards remaining in the deck
 public int numberOfCards() 
 return deck.size();
 

 // a method to remove a card from the deck
 public Card removeCard(Card card) 
 if(deck.contains(Objects.requireNonNull(card))) 
 int index = deck.indexOf(card);
 return deck.remove(index);
 else 
 return null;
 
 

 // returns true if the specified card is contained in the deck
 public boolean containsCard(Card card) 
 return deck.contains(Objects.requireNonNull(card));
 

 // populates the deck with all possible cards in the deck. 52 cards
 public void populateDeck() 
 for(Suit suit: Suit.values()) 
 for(Rank rank: Rank.values()) 
 deck.add(new PlayingCard(rank,suit));
 
 
 

 // Overloaded method which allows any number of decks of 52 cards.
 public void populateDeck(int numberOfDecks) 
 for(int i = 0; i < numberOfDecks; i++) 
 populateDeck();
 
 

 // remove all cards from the deck
 public void clearDeck() 
 deck.clear();
 

 // returns an unmodifiable collection, a view, of the deck.
 public Collection<Card> getUnmodifiableCollection() 
 return new ArrayList<>(deck);
 

 // returns the first card in the collection.
 public Card pop() 
 return deck.pop();
 

Hand.java

package com.frijolie.cards;

// snipped imports

public class Hand implements Comparable<Hand> 

 private List<Card> cards;

 // default constructor
 public Hand() 
 cards = new ArrayList<>();
 

 // will add a specific card to the Hand
 public void addCard(Card card) 
 cards.add(Objects.requireNonNull(card));
 

 // will remove a specific card from the Hand if it's contained in the Hand
 public Card removeCard(Card card) 
 if (cards.contains(Objects.requireNonNull(card))) 
 int index = cards.indexOf(card);
 return cards.remove(index);
 else 
 throw new IllegalArgumentException("This card did not exist in the hand. It cannot be removed.");
 
 

 // method to display all the Cards in the Hand
 public void displayHand() 
 for (Card card : cards) 
 if (card.isFaceUp()) 
 card.displayCard();
 else 
 System.out.println("[ Card Back ]");
 
 
 

 // returns the sum of the value of all Cards in the Hand
 public int calculateValue() 
 var handValue = 0;
 for (Card card : cards) 
 handValue += card.getRank().getValue();
 
 return handValue;
 

 // remove all cards in the Hand
 public void clearHand() 
 cards.clear();
 

 // returns a unmodifiable collection, a view, of all cards in the Hand
 public Collection<Card> getUnmodifiableCollection() 
 return Collections.unmodifiableCollection(cards);
 

 // compares one Hand to another
 public int compareTo(final Hand otherHand) 
 return this.calculateValue() - otherHand.calculateValue();
 

 // sorts cards in the Hand by Suit. Doesn't factor Rank within the Suit when sorting
 public void sortyBySuit() 
 cards.sort(Comparator.comparing(Card::getSuit));
 

 // sorts cards in the Hand by Rank. Doesn't factor Suit when sorting.
 public void sortByRank() 
 cards.sort(Comparator.comparing(Card::getRank));
 

 // sorts cards in the Hand by Color. Doesn't factor Suit or Rank when sorting.
 public void sortByColor() 
 cards.sort(Comparator.comparing(Card::getColor));
 

 // sorts cards in the Hand by Suit then Rank in ascending order.
 public void sort() 
 cards.sort((Card first, Card second) -> first.compareTo(second));
 

 // allows a card in the hand to be flipped in the opposite orientation
 public void flipCard(int index) 
 cards.get(index).flip();
 

PlayingCard.java

package com.frijolie.cards;

import java.util.Objects;

public class PlayingCard implements Card 

 private final Rank rank;
 private final Suit suit;
 private boolean isFaceUp;

 // default constructor
 public PlayingCard(final Rank rank, final Suit suit) 
 this.rank = rank;
 this.suit = suit;
 isFaceUp = true;
 

 ... // snipping implemented accessor methods from Card interface

 // returns a string representation of the card
 @Override
 public String toString() 
 return rank.getLetter() + suit.getSymbol();
 

 // compares one PlayingCard to another
 @Override
 public boolean equals(final Object o) 
 Objects.requireNonNull(o);
 boolean isEqual;

 if (!(o instanceof Card)) 
 isEqual = false;
 else if (this == 0) 
 isEqual = true;
 else 
 var card = (Card) o;
 isEqual = (this.getRank().equals(card.getRank()) && this.suit.equals(card.getSuit()));
 
 return isEqual;
 

 // generates unique int value of a card
 @Override
 public int hashCode() 
 int suitMultiplier = suit.ordinal() + 2;
 int rankMultiplier = rank.ordinal() + 3;
 return (suitMultiplier * rankMultiplier) * 31;
 

 // flip the card in the opposite orientation, faceUp/faceDown
 @Override
 public void flip() 
 isFaceUp = !isFaceUp;
 

 // returns true if the card is faceUp
 @Override
 public boolean isFaceUp() 
 return isFaceUp;
 

 // compares one card to another and returns an integer value
 @Override
 public int compareTo(Card o) 
 var card = Objects.requireNonNull(o);
 var suitCompareResult = this.suit.compareTo(card.getSuit());
 if (suitCompareResult != 0) 
 return suitCompareResult;
 else 
 return this.rank.compareTo(card.getRank());
 
 

What could I improve?

I've toyed with the idea to make Hand and/or Deck an interface. However, I opted to go with classes in favor of code reuse and contained behavior. I send unmodifiable collections (Hand, Deck) as I want to control the behavior of the internal collection within their respective classes. You can see what it looks like, but you can't modify the Deck or Hand outside the class. Is this a good defensive strategy?

I'm also not 100% comfortable with the requirement of a Rank and Suit. How about a Magic the Gathering Card? That doesn't have a suit, but you could argue that it has a rank. Is it better to leave the implementation up to the other developer? Same could be said for an Uno card, Color.Red and Color.BLACK aren't the only options there.

I've thought about the Rank enum also. If the game you model has an ACE as a high card (not low as supplied here), how would you fix that in your own implementation? Create a new AceHighRank and declare them in a different order? However, that would break PlayingCard because it's expecting a Rank (not AceHighRank). Right?

I think you are really thinking well about what is going on, what potential up and downsides are. I am not really familiar with card games, but my initial guess would be that it may be a bit long stretched to find a unified interface for all types of card games. In general you have cards, hands and decks, but the similarities can pretty much end there.

I also like that you used good descriptive names, private fields, and final where possible (though you are not entirely consistent with it). There is quite a bit of code, so I'll comment on the major code wise things that spring to mind:

• The method comments should really be changed to true JavaDoc, in order for them to be useful to the users of your library. EDIT: apparently you already did this in your GitHub version.




• I am not sure if you would really want to force implementors to implement equals/hashcode.




• Like you said, the Rank (more specifically, it's value) can differ from game to game. This would make more sense in the Card implementation itself I'd think. Possibly even higher up?




• isShuffled in Deck seems a bit strange to me. Why would you need it? As soon as you shuffle the deck, it will also always be true.




• You should generally not use the Stack class in Java, see Stack




• You do not need to loop yourself in order to fill the deck multiple times, you already have a method for that!




• I would throw an exception when someone passes a card which is not contained in the deck, instead of null.




• Think about how you would support decks for games which would not use the entire range of cards.




• You are returning a Collection, which does not imply order. Your deck is very much ordered, so return a List, or your stack-like class. Also, the method does not tell me anything about what it is giving me.




• I would rename pop to something like drawCard.




• I would not mix user interface code within your domain objects, like e.g. the displayHand method. What if you would want to use it with a realistic 3D user interface?




• The compareTo method in Hand is implemented somewhat risky. Only because you know value is always positive, it always works. Just something to think about.




• Nice use of method references with the Comparator! You could use a static import to write sort(comparing(Card::getSuit)). You can also use this for the basic sort method: cards.sort(Card::compareTo).




• Return early. In the equals method, as soon as you know it is not equal, return false. Don't introduce unnecessary variables, unless they would help with readability.




• Your hashCode implementation is not really good, e.g. the hashcode of suit.ordinal = 1, rank.ordinal = 2 is equal to the one of suit.ordinal = 3, rank.ordinal = 0, and there are more to be found. You can look up how to implement a correct hashcode, or use some auto generation in your IDE to get started. Returning a lot of equal values is not wrong, but can slow down e.g. hashing data structures.

Your question about only changing state through the public interface: definitely yes! It is a really good principle. It makes sure you are in control.

Finally, this is me, but I am not much of a fan of using the get prefix. Apart from implying it always is a simple field acces (which granted, in this case is generally true), for me it feels superfluous. Apparently, it also has a name, namely the Uniform Access Principle. There are some interesting reads about this I think.

Every competent non-junior developer is self-taught, because you don't learn everything you need to know in school.

This is actually a reasonably tricky problem you've picked out, so don't be discouraged by all the feedback. You need to spend more time thinking about the bounds between application-specific code (Poker rules, Uno rules, etc) and the generic library code (How does a Deck work, what does a Card look like). Right now it's all mixed together. That's not surprising - building good APIs is hard.

It also seems like you have a lot of methods that serve no particular purpose because you think they might be handy later. It's really easy to add and really hard to remove, and YAGNI. Wait for a use case before adding things, and make sure you do it right, because if you have to fix it all your clients will break.

Card

• Use javadoc, not comments




• equals and hashCode aren't doing anything. All objects have them, so every Card will always fulfill that part of the contract without doing any extra work.




• Consider having flip() return the new state of the card.




• What is the point of this interface? If you're only building for poker cards, the interface is noise. If you're building for other kinds of cards, you're forcing a ton of false requirements - Uno cards, for instance, can't fulfill this contract. Strongly consider making this a marker interface if you want to support non-poker games.




• The methods are mixed-up. On the one hand, clients can ask your for your internals and you freely share them. On the other hand, the card renders itself with no inputs. Either keep your privates private and use instead methods like hasRank(Rank rank) or get rid of the render method and let the client do it.

Color

• Having a class with the same name as a class in the Java API is a bad idea. You're asking for conflicts. It's not really a Color, it's a CardColor or some such.

Deck

• Deck needs to be final. You can't have users extending Deck with their own functionality (like putting all the good cards in one place when "shuffling")s




• A deck is a well-understood concept. It's nonsensical to create a deck and specify the number of decks it has. You need a separate concept, a Shoe. Shoes can hold multiple decks of poker cards in them.




• You don't need to clear a new Deck. It's already clear.




• This isn't a generic Deck class. It's a poker deck. Do you want something useful for other games? If so, you need to take all the poker functionality into a separate class.




• Shuffling more than once in a row is not sensical in code. The deck will be no more shuffled after the eighth time than the first.




• isShuffled() is not useful. After you call shuffle, the deck is shuffled. Unless you add some way to unshuffle a deck?




• I doubt the utility of removeCard(Card) and hasCard(Card). I guess there might be games where they are helpful, but I can't think of any.




• It's visually ugly to use the return value of Objects.requireNonNull() like that. Declare it as a standalone call.




• populateDeck() again assumes a poker deck. And it's public, which means any yahoo with access to the deck can throw in cards at any time.




• clearDeck() is the same problem. And what does it mean for a deck to be cleared? In real life, you don't ever get rid of all the cards, still have a "deck", and then add cards back in. You might take cards out of the deck or put them back in, but that's different than the abstraction you have.




• What's the use case for giving out a list of the cards? You should return a List, not a Collection, because decks typically have an order.




• A deck is usually not a stack. A Deque might be more appropriate.




• pop is leaking internal details. Nobody says "pop me a card off that deck".

Hand

• Needs to be final for the same reason as Deck.




• Is there a use case for removing a specific card from a Hand?




• Again, keep the display code out of the model. Either pass in something to do the rendering or have the UI call methods on Hand to render.




• calculateValue - is there a use case for this? What game uses as a value the sum of all the cards?




• clearHand - this needs a better name, because that's not what people talk about doing in real life. Also, it should return the cards you just tossed, to facilitate putting them back in the deck. Or maybe pass in a deck and add the cards back to it?




• compareTo - there are lots of different ways to compare the strengths of two hands. You shouldn't be picking one and hard-coding it here.




• Don't have multiple hard-coded sort methods. Have one method that takes a comparator. Otherwise every time any client wants a new sort method, the API needs to change.




• Maybe revealed would be better than flipped? I also think that the revealed state of a card belongs here, not on the Card.

Playing Card

• Does isFaceUp make sense on PlayingCard? I guess it might if your deck can have face-up cards in it. But strongly consider if that should be external state. That sounds like it belongs to the UI layer, not the model.




• Return early from equals(). The reason for a single return was made obsolete years ago.




• Strongly consider whether you want to implement compareTo(). Some games have trump suits which can change the natural ordering of the cards in the deck.

I took a stab at a generic deck class, and came up with this. It should probably have a draw(int cards) and overloaded discard methods also, but I need to pack.

//Note that this class is *NOT* thread safe. It will definitely have problems in a multi-threaded environment.
public final class Deck<T extends Card> 

 private final List<T> deck = new LinkedList<>();
 private final Set<T> outstandingCards = new HashSet<>();
 private final List<T> discardPile = new LinkedList<>();

 public Deck(final Collection<T> cards) 
 super();
 this.deck.addAll(cards);
 

 public void shuffle() 
 this.deck.addAll(this.discardPile);
 this.discardPile.clear();
 Collections.shuffle(this.deck);
 

 public boolean isDeckEmpty() 
 return this.deck.isEmpty();
 

 public int deckSize() 
 return this.deck.size();
 

 public T draw() 
 if (this.isDeckEmpty()) 
 throw new IllegalStateException();
 

 final T card = this.deck.remove(0);
 this.outstandingCards.add(card);
 return card;
 

 public void discard(final T card) 
 if (!this.outstandingCards.contains(card)) 
 // Somebody tried to sneak a new card into the deck!
 throw new IllegalStateException();
 
 this.outstandingCards.remove(card);
 this.discardPile.add(card);
 

In Deck.java you have:

if(deck.contains(Objects.requireNonNull(card))) {

but you have a function containsCard that could shorten it:

if(containsCard(card))