Cfrtjryk

The program interacts between cards and four players among whom cards are to be distributed.

The Program do the following function

• Creates a deck of cards.


• Shuffle the deck.


• Shows the deck.


• Deal cards equally among four players.


• Show the cards of each Player.

Please suggest some better ways of doing this program.

Also suggest new functions and modifications in this program.

package cardgame;



public class Card {



    String suit;

    String rank;



    public Card(String cardSuit, String cardRank){

        this.suit = cardSuit;

        this.rank = cardRank;

    }

}

package cardgame;



import java.util.*;



public class DeckOfCards {



    final int size = 52;

    Card deckOfCards = new Card[size];





    public DeckOfCards(){



        int count=0;



        String suits = {"Diamonds","Clubs","Hearts","Spades"};

        String ranks ={"King","Queen","Jack","Ten","Nine","Eight","Seven","Six","Five","Four","Three","Deuce","Ace",};



        for (String s:suits){

            for (String r:ranks){



                Card card = new Card(s, r);

                this.deckOfCards[count] = card;

                count++;

            }

        }



    }

    public void ShuffleCards(){

        Random rand = new Random();

        int j;

        for(int i=0; i<size; i++){

            j = rand.nextInt(52);

            Card temp = deckOfCards[i];

            deckOfCards[i]=deckOfCards[j];

            deckOfCards[j]= temp;

        }

    }

    public void showCards(){

        System.out.println("---------------------------------------------");

        int count =0;

        for (Card card : deckOfCards){

            System.out.print(card.rank + " of " + card.suit + "     ");

            count++;

            if(count%4==0)

                System.out.println("");

        }

        System.out.println("---------------------------------------------");

    }

    public void dealCards(Players player1,Players player2,Players player3,Players player4){



        int count = 0;



        for (Card card : deckOfCards){



            if (count>38){

                player1.playCards[count%13] = card;

                //System.out.println(player1.playCards[count/12].rank+"   "+player1.playCards[count/12].suit);

            }

            else if (count>25){

                player2.playCards[count%13] = card;

            }

            else if (count>12){

                player3.playCards[count%13] = card;

            }

            else{

                player4.playCards[count%13] = card;

            } 

            count++;

        }

    }





}

package cardgame;



public class Players {

    String name;

    Card playCards = new Card[13];





    public Players(String name){

        this.name = name;

    }

    public void ShowPlayerCards(){

        System.out.println("---------------------------------------------");

        for (Card card : playCards){

            if(card!=null)

                System.out.println(card.rank + "  of  " + card.suit);

        }

        System.out.println("---------------------------------------------");

    }

    public String getName(){

        return name;

    }



}

package cardgame;



import java.util.*;





public class CardGame {



    public static void main(String args) {

        DeckOfCards deck = new DeckOfCards();

        System.out.println("UnShuffeled Cards.");

        deck.showCards();

        deck.ShuffleCards();

        System.out.println("Shuffeled Cards.");

        deck.showCards();



        Scanner input = new Scanner(System.in);

        System.out.println("Player One...nEnter Name:");

        Players player1 = new Players(input.nextLine());

        System.out.println("Player Two...nEnter Name:");

        Players player2 = new Players(input.nextLine());

        System.out.println("Player Three...nEnter Name:");

        Players player3 = new Players(input.nextLine());

        System.out.println("Player Four...nEnter Name:");

        Players player4 = new Players(input.nextLine());



        deck.dealCards(player1, player2, player3, player4);

        System.out.println("---------------------------------------------");

        System.out.println(player1.getName());

        player1.ShowPlayerCards();

        System.out.println(player2.getName());

        player2.ShowPlayerCards();

        System.out.println(player3.getName());

        player3.ShowPlayerCards();

        System.out.println(player4.getName());

        player4.ShowPlayerCards();

    }



}

Ok... I am not sure how to show you all of the refactorings I did in a way that will make sense, so I'm just going to post the refactored classes and go from there.

Main:

public static void main(String args) {



    Scanner input = new Scanner(System.in);

    Players players = new Players[4];

    Card deck = Dealer.getDeckOfCards();



    System.out.println("Un-shuffled Cards.");

    Dealer.showCards(deck);

    Card shuffledCards = Dealer.shuffleCards(deck);

    System.out.println("Shuffled Cards.");

    Dealer.showCards(shuffledCards);



    for(int i = 0; i < players.length; i++) {

        System.out.println("Enter Player Name: ");

        players[i] = new Players(input.nextLine());

    }



    Players playersWithCards = Dealer.dealCards(players, shuffledCards);



    System.out.println("---------------------------------------------");



    for(Players player : playersWithCards) {

        System.out.println(player.getName());

        player.showPlayerCards();

    }



}

Players:

class Players {



    private String name;

    private Card cards = new Card[13];



    Players(String name){

        this.name = name;

    }

    void showPlayerCards(){

        System.out.println("---------------------------------------------");

        for (Card card : cards){

            //you had been checking here if this was null, but there was no need for that check

            System.out.printf("%s  of %sn", card.rank, card.suit);

        }

        System.out.println("---------------------------------------------");

    }

    void receiveCard(Card card, int position){

        cards[position] = card;

    }

    String getName(){

        return name;

    }



}

Dealer (formerly DeckOfCards)

class Dealer {

    private static final int SIZE = 52;

    private static Card deckOfCards = new Card[SIZE];



    static Card getDeckOfCards() {



        int count = 0;



        String suits = {"Diamonds", "Clubs", "Hearts", "Spades"};

        String ranks = {"King", "Queen", "Jack", "Ten", "Nine", "Eight", "Seven", "Six", "Five", "Four", "Three", "Deuce", "Ace"};



        for (String s : suits) {

            for (String r : ranks) {



                Card card = new Card(s, r);

                deckOfCards[count] = card;

                count++;

            }

        }



        return deckOfCards;



    }



    static Card shuffleCards(Card deckOfCards) {

        Random rand = new Random();

        int j;

        for (int i = 0; i < SIZE; i++) {

            j = rand.nextInt(SIZE);

            Card temp = deckOfCards[i];

            deckOfCards[i] = deckOfCards[j];

            deckOfCards[j] = temp;

        }

        return deckOfCards;

    }



    static void showCards(Card deckOfCards) {

        System.out.println("---------------------------------------------");

        int count = 0;

        for (Card card : deckOfCards) {

            System.out.printf("%s of %st", card.rank, card.suit); //use print f with t (tab character)

            count++;

            if (count % 4 == 0)

                System.out.println();

        }

        System.out.println("---------------------------------------------");

    }



    static Players dealCards(Players players, Card deck) {

        int numOfCardsPerPlayer = deck.length / players.length;

        for (int i = 0; i < deck.length; i++) {

            int positionInHand = i % numOfCardsPerPlayer;

            players[i % players.length].receiveCard(deck[i], positionInHand);

        }



        return players;

    }

}

and Card:

class Card {

    String suit;

    String rank;



    Card(String cardSuit, String cardRank){

        this.suit = cardSuit;

        this.rank = cardRank;

    }

}

1. The first thing I did after refactoring your Main to use loops whenever possible was to ensure that you weren't unnecessarily making code public. All of your classes are in the same package, so you can make them package-private by removing the public modifiers. This is just generally considered good practice so that when you start working on projects with many classes, (some of which may have the same name) you are limiting conflicts.




2. Probably the single biggest difference between your code and the way I refactored it was that I changed DeckOfCards to a Dealer, and made it static. In programming, an abstraction of a DeckOfCards is really just an array of cards, like Card deck = Dealer.getDeckOfCards();. It seemed to me that most of the tasks you were calling from DeckOfCards were really the job of a Dealer, so I changed the code to reflect that, passing in the values created in the driver class as the program progresses. (For example in the line Card shuffledCards = Dealer.shuffleCards(deck);) If you look at this class, you'll see that all of its methods are static, which is really just a preference thing. If you wanted to make a constructor like Dealer dealer = new Dealer(); for a dealer and view it more as an entity than a doer, you could.

I'm sure I probably missed some stuff so if you have any questions let me know. All in all I think you did a really good job for a new developer.

I have the following suggestions:

1. Make Suit and Rank enums since they are fixed and are not going to be altered.


2. It is usually a good practice to make all instance variables private and have getter and setter methods to access them.


3. Make size final static since it is a constant value and is not going to be changed.

Here's the complete code:

Suit enum

package cardGame;



enum Suit {

    DIAMONDS,

    CLUBS,

    SPADES,

    HEARTS;

}

Rank enum

package cardGame;



enum Rank {

    ACE,

    DEUCE,

    THREE,

    FOUR,

    FIVE,

    SIX,

    SEVEN,

    EIGHT,

    NINE,

    TEN,

    JACK,

    QUEEN,

    KING;

}

Card class

package cardGame;



class Card {

    private final Suit suit;

    private final Rank rank;



    Card(Suit suit, Rank rank) {

        this.suit = suit;

        this.rank = rank;

    }



    Rank getRank() {

        return rank;

    }



    Suit getSuit() {

        return suit;

    }



    @Override

    public String toString() {

        return rank + " of " + suit;

    }

}

DeckOfCards class

package cardGame;



import java.util.Random;



class DeckOfCards {

    public static final int SIZE = 52;

    private final Card cards = new Card[SIZE];



    DeckOfCards() {

        int currentCardIndex = 0;



        for (Suit suit : Suit.values()) {

            for (Rank rank : Rank.values()) {

                cards[currentCardIndex++] = new Card(suit, rank);

            }

        }

    }



    Card getCards() {

        return cards;

    }



    Card getCard(int index) {

        return cards[index];

    }



    void shuffleDeck() {

        Random rand = new Random();



        for (int i = 0; i < SIZE; i++) {

            int j = rand.nextInt(SIZE);

            swapCards(i, j);

        }

    }



    void swapCards(int i, int j) {

        Card temp = cards[i];

        cards[i] = cards[j];

        cards[j] = temp;

    }



    @Override

    public String toString() {

        StringBuilder stringBuilder = new StringBuilder();



        stringBuilder.append("Current Deck:n");



        for (int i = 0; i < DeckOfCards.SIZE; i++) {

            stringBuilder.append("Card #" + (i + 1) + ": " + getCard(i) + "n");

        }



        return stringBuilder.toString();

    }

}

Player class

package cardGame;



import java.util.ArrayList;

import java.util.List;



class Player {

    private String name;

    private List<Card> cards = new ArrayList<>();



    Player(String name) {

        this.name = name;

    }



    void giveCard(Card card) {

        cards.add(card);

    }



    List<Card> getCards() {

        return cards;

    }



    String printPlayerCards() {

        StringBuilder stringBuilder = new StringBuilder();

        stringBuilder.append(name + " has the following cards:n");



        for (Card card : cards) {

            stringBuilder.append(card + "n");

        }



        return stringBuilder.toString();

    }



    @Override

    public String toString() {

        return name;

    }

}

CardGame class

package cardGame;



import java.util.Scanner;



public class CardGame {

    private static final int NO_OF_PLAYERS = 4;

    private final Player players = new Player[NO_OF_PLAYERS];

    private final DeckOfCards deckOfCards = new DeckOfCards();



    public static void main(String args) {

        CardGame cardGame = new CardGame();



        System.out.println("WELCOME TO THE CARD GAMEn");

        System.out.println("Enter the four players' name below");



        Scanner scan = new Scanner(System.in);

        for (int i = 0; i < NO_OF_PLAYERS; i++) {

            cardGame.players[i] = new Player(scan.next());

        }



        cardGame.deckOfCards.shuffleDeck();



        System.out.println(cardGame.deckOfCards);



        cardGame.dealCards();



        cardGame.displayCardsForAllPlayers();

    }





    private void dealCards() {

        for (int i = 0; i < DeckOfCards.SIZE; i++) {

            players[i % NO_OF_PLAYERS].giveCard(deckOfCards.getCard(i));

        }

    }



    private void displayCardsForAllPlayers() {

        for (int i = 0; i < NO_OF_PLAYERS; i++) {

            System.out.println(players[i].printPlayerCards());

        }

    }

}

Welcome on Code Review!

Addendum to what others say:

Naming

Singular & plural nouns

A player is a Player not a Players, you should use singular names when you talk about one thing, and plural when you talk about many. ie:

Player players = new Player[4];

Avoid redundancy

Try to avoid redundancy in naming, so instead of having:

DeckOfCards.shuffleDeck()

you can write:

DeckOfCards.shuffle()

Keep them simple

There's not much chance here that reader think about a loading deck if you simply named your class Deck. In this context, it's pretty obvious that's a cards' deck.

MISC

Be generic when possible

Try to be as generic as possible, avoid magic values. The size of your deck if the sum of all possible combinations, so for example, taking again the use of enums advised in one of the other answer:

private static final int SIZE = Suit.values().length * Rank.values().length;

Like that, if later you decide to change the type of deck, eg removing aces or figures, the change will be reflected automatically in the rest of your code. And you know... «Less code to refactor make developers happier».

Think about underlying types

You can maybe store just the index of the card, with a simple int. A card can be represented by an index relative to its place in a new deck. [range 0-51].

To retrieve suits and ranks from indexes, depending on how cards are ordered in the deck.

If ordered by rank (A♡, A♢, A♠, A♣, 2♡, 2♢, ..., K♡, K♢, K♠, K♣) :

Rank r = Rank.values()[i / 4];

Suit s = Suit.values()[i % 4];

If ordered by suit (A♡, 2♡, 3♡, ..., J♣, Q♣, K♣) :

Rank r = Rank.values()[i % 13];

Suit s = Suit.values()[i / 13];

(and for faster/better way to cast int to enum, check this SO post)

In addition to the other answers:

Your shuffle algorithm is unfair. It will not produce all card distributions with the same probability. Luckily someone thought about this problem before and wrote the generic solution. You just need to use it:

Collections.shuffle(Arrays.asList(cards), rand);

See Wikipedia for more details.