lyudaio / jcards Goto Github PK

Methods that throw exceptions have to have their Exceptions declared in the method header, e.g.

    public Card(Rank rank, Suit suit) throws IllegalStateException {
        if (rank == null) {
            throw new IllegalArgumentException("Rank cannot be null");
        }
        if (suit == null) {
            throw new IllegalArgumentException("Suit cannot be null");
        }
        this.rank = rank;
        this.suit = suit;
    }

This goes through the entire codebase in every single method that throws an exception.

1. I would extract the enums out of the Card class so that they can be easier used for comparison
2. You omitted a state of a card - a card can either be face up or face down - this can become important in games where you have some open cards on the table - like for Texas Hold'em Poker.
3. Your compareTo method in the Deck is fairly meaningless. What game would need to compare sizes of decks?
4. All your "find" methods in the Deck class are unclear. Why would anybody need to search for a certain card in a deck? - Maybe a method to extract a specific card from a deck could be useful for games that need a specific "starter" card
5. Depending on the game, the Ace card can be high or low. As of now, you only have implemented "Ace low". It is even possible that during a game, the Ace can be either and change state.
6. I would not throw an "IllegalStateException" if the deck is empty. An empty deck is a perfectly valid state for most card games and in this case, the return value should simply be 0.
7. The cardsRemaining and getSize methods do essentially the same. One of them is unnecessary.
8. A method to deal several cards from the deck should be added.
9. The Unicode constants could and should be directly included in the respective Rank and Suit enums. Separate enums don't make much sense.

Different issue because the following are feature suggestions.

1. Maybe construct a "Table" class (for lack of a better name that I can't come up with right now) - it would be similar to the Hand, but cards in their current state would be visible to all players. Should have option for a maximum of cards. - Again for games like "Texas Hold'em" or even for games like "Canasta", "Bridge" where players openly lay card series out
2. Maybe construct a "Discard Pile" class - as of now, this could be emulated with a "Deck", but since it serves a different purpose, it could be in its own class. In a discard pile the top card (or even n top cards) could be visible to all players and could be taken by a player. Certain cards could also "lock" the discard pile so that taking from it is not possible.

Please validate the suggestion against your view on the project. Chances are you keep it simple and narrow-scoped for a reason.
But I noticed, that it would be not so complicated to change classes like Hand, Deck etc to support generic types, and change Card type to be an interface. Typical playing cards (hearths, spades etc) could still be provided as an implementation provided by a library.
It would allow users to supply their own implementations opening up the project for other types of card games (like TCG games like Magic the Gathering).

Hello

It looks like the minimum Java version for this project is 19.
I suggest setting the minimum required version to 17, as it is the latest LTS version.

More about versions here https://en.wikipedia.org/wiki/Java_version_history

• testShuffle could theoretically fail if both decks were randomly shuffled into the same order

• testFindCard should probably assert that the correct card was in fact found, rather than just that the returned index is between 0 and 51

Hey,

not all card games use a standard deck of cards. Some might use different cards that can have different ranks: https://en.wikipedia.org/wiki/Tapp_Tarock
We could generify enums Rank and Suit so that we would convert them to interfaces that would only contain getters and other methods that Rank/Suit needs and then we would "implement" these interfaces for different types of ranks and suites. The current enum Rank would then be named like standard French rank etc.
I can help with implementation if you find this useful.