The card game war

Question

I'm a beginner programmer looking for a review of my simple war card game program I made. Each player gets a deck and you pull 1 card at a time and compare the 2. Highest value wins. Do that until the deck is gone and whoever has the most points wins. Any advice is appreciated! Thank you for your time looking at this!

enum Suit
{
    Hearts,
    Diamonds,
    Spades,
    Clovers
}

enum Face
{
    Two,
    Three,
    Four,    
    Five,
    Six,
    Seven,
    Eight,
    Nine,
    Ten,
    Jack,
    Queen,
    King,
    Ace
}

// MAKING WAR CARD GAME

class Program
{
    public static bool isPlaying = true;
    public static Player human = new Player();
    public static Player cpu = new Player();
    public static int round = 1;
    static void Main(string[] args)
    {
        GameFlow.Start(human, cpu);
        while (isPlaying)
        {
            for (int i = 0; i < 52; i++)
            {
                GameFlow.Gameplay(human, cpu, round);
                round++;
            }

            round = 1;
            GameFlow.Ending(human, cpu);
        }
    }
}
class Player
{
    public string name = "CPU";
    public Card currentCard = new Card(); // the current card in play
    public int wins = 0 , drawCount = 0;
    public Deck myDeck = new Deck();

    public void DrawOne()
    {
        currentCard = myDeck.CurrentlyInDeck[drawCount];
        drawCount++;
        if (drawCount == 52)
        {
            drawCount = 0;
        }
    }

    public void SetName()
    {
        while (name == "CPU" || name == "")
        {
            Console.WriteLine("What is your name human?");
            name = Console.ReadLine();
            if(name == "CPU" || name == "")
            {
                Console.WriteLine("Invalid name!! Try a real name you assclown!");
                Console.ReadLine();
            }
            Console.Clear();
        }
    }
class Card
{
    public Suit suit;
    public Face face;

    public void PrintCard()
    {
        Console.Write("{0} of {1}", face, suit);
    }
}
class Deck
{
    public List<Card> CurrentlyInDeck;

    public void GenerateDeck() // Must use this to populate deck
    {
        CurrentlyInDeck = new List<Card>(52);
        int place = 0; // tracks which card number
        for (int i = 0; i < 4; i++)
        {
            for (int f = 0; f < 13; f++)
            {
                Card card = new Card();
                CurrentlyInDeck.Add(card);
                CurrentlyInDeck[place].suit = (Suit)i;
                CurrentlyInDeck[place].face = (Face)f;
                place++;
            }
        }
    }

    private static readonly Random rand = new Random();

    public void Shuffle() 
    {
        Card holder = new Card();
        int random;

        for (int i = 0; i < 52; i++)
        {
            random = rand.Next(0, 51);
            holder = CurrentlyInDeck[i];
            CurrentlyInDeck[i] = CurrentlyInDeck[random];
            CurrentlyInDeck[random] = holder;
        }
    }

    public void PrintDeck() // prints all cards in the deck , used for testing
    {
        foreach (var card in CurrentlyInDeck)
        {
            card.PrintCard();
        }
    }
}
static class Create
{
    public static void Name(Player a)
    {
        a.SetName();
    }

    public static void TheirDecks(Player hum , Player cpu)
    {
        hum.myDeck.GenerateDeck();
        cpu.myDeck.GenerateDeck();

        hum.myDeck.Shuffle();
        cpu.myDeck.Shuffle();
    }
}
static class GameFlow
{
    static public void Start(Player hum, Player cpu) //Creates initial situation 
    {
        Words.Begin();
        Create.TheirDecks(hum, cpu);
        Create.Name(hum);
    }

    static public void ShuffleDecks(Player hum, Player cpu)
    {
        hum.myDeck.Shuffle();
        cpu.myDeck.Shuffle();
    }

    static public void DrawCards(Player hum, Player cpu) // changes the current card
    {
        hum.DrawOne();
        cpu.DrawOne();
    }

    static public void Gameplay(Player hum, Player cpu, int round) // The normal gameplay loop
    {
        GameFlow.DrawCards(hum, cpu);
        Words.Template(hum, cpu, round);
        GameFlow.CheckWin(hum, cpu);
    }

    static public void Ending(Player hum, Player cpu) // Once all the cards have been played
    {
        string answer = "";
        bool correctForm = false;
        Words.LastWords(hum, cpu);
        while (!correctForm)
        {
            Console.WriteLine("Would you like to play again? Enter y for yes or n for no.");
            answer = Console.ReadLine();
            answer = answer.ToLower();

            if(answer != "y" && answer != "n")
            {
                Console.WriteLine("Thats not a valid option you idiot!");
                Console.WriteLine("Press enter to continue.");
                Console.ReadLine();
                Console.Clear();
                Words.LastWords(hum, cpu);
            }

            if(answer == "y" || answer == "n")
            {
                correctForm = true;
            }
        }

        if (answer == "y")
        {
            PlayingAgain(hum ,cpu);
            Console.Clear();
        }

        if (answer == "n")
        {
            Console.WriteLine("Thanks for playing!");
            Console.WriteLine("Press enter to exit.");
            Environment.Exit(0);
        }
    }

    static public void PlayingAgain(Player hum , Player cpu)
    {
        ShuffleDecks(hum, cpu);
        hum.wins = 0;
        cpu.wins = 0;
    }

    static public void CheckWin(Player hum , Player cpu)
    {
        Console.WriteLine("");
        if ((int)hum.currentCard.face > (int)cpu.currentCard.face)
        {
            hum.wins++;
            Console.WriteLine();
            Console.WriteLine("You win this one! Nice!");
            Console.WriteLine("Press enter to keep going.");
            Console.ReadLine();
            Console.Clear();
        }

        else if ((int)hum.currentCard.face < (int)cpu.currentCard.face)
        {
            cpu.wins++;
            Console.WriteLine();
            Console.WriteLine("Looks like the CPU won this one. You suck!");
            Console.WriteLine("Press enter to keep going.");
            Console.ReadLine();
            Console.Clear();
        }

        else
        {
            Console.WriteLine();
            Console.WriteLine("Its a draw!");
            Console.WriteLine("Press enter to keep going.");
            Console.ReadLine();
            Console.Clear();
        }
    }
}
static class Words // The dialogue of the program
{
    public static void Begin()
    {
        Console.WriteLine("Hello , welcome to war!");
        Console.WriteLine("The rules are simple. Its you vs a cpu");
        Console.WriteLine("You each pull 1 card from your decks and see which is highest.");
        Console.WriteLine("Your card will be the one under your name , while the other is the CPUs.");
        Console.WriteLine("The one with the most points at the end wins all the glory!");
        Console.WriteLine("Understand?");
        Console.WriteLine("Press enter if you're ready for the showdown.");
        Console.ReadLine();
        Console.Clear();
    }

    public static void Template(Player hum , Player cpu , int round) //Prints the normal screen
    {
        int distance = 17; //distance between the 2 names
        Console.Write("{0} wins = {1}", hum.name, hum.wins);
        Console.Write("          R{0}", round);
        distance -= hum.name.Length;
        while (distance > 0)
        {
            Console.Write(" ");
            distance--;
        }
        Console.Write("{0} wins = {1}" , cpu.name , cpu.wins);
        Console.WriteLine();
        Console.WriteLine();
        hum.currentCard.PrintCard();
        Console.Write("     vs     ");
        cpu.currentCard.PrintCard();
    }

    public static void LastWords(Player hum , Player cpu)
    {
        Console.WriteLine("Thats all the cards!");
        Console.WriteLine("So this is the final score: {0} = {1} and {2} = {3}" , hum.name, hum.wins, cpu.name, cpu.wins);

        if(hum.wins > cpu.wins)
        {
            Console.WriteLine("You have beaten the CPU in this game of luck!!!! Congrats!!!");
        }

        else if(cpu.wins > hum.wins)
        {
            Console.WriteLine("The CPU has detroyed you and everything you have loved. Congrats!!!");
        }

        else
        {
            Console.WriteLine("Oh my! Its a draw. You both are equal warriors of luck.");
        }
    }
}

Answer 1

The other answer already mentions the Fisher-Yates Shuffle which is definitely what you want to use. I thought I'd break down your Shuffle a bit to show you why it's flawed.

This is what I consider to be one of the best web pages on the entire internet, and it has a section on shuffling that you might find interesting: Visualizing Algorithms by Mike Bostock

Mike has also created this resource: Will it shuffle? Go to this page and change the selection to "naïve swap (i ↦ random)" which is what you've implemented. It will show you that the shuffle is quite biased:

If that weren't bad enough, you have accidentally added an additional bias. I've translated your algorithm to JS and run it for you:

Do you see that very dark purple square at the bottom right? That shows a very negative bias for the last element. Let's look at why:

public void Shuffle() 
{
    Card holder = new Card();
    int random;

    for (int i = 0; i < 52; i++)
    {
        random = rand.Next(0, 51); // !!!
        holder = CurrentlyInDeck[i];
        CurrentlyInDeck[i] = CurrentlyInDeck[random];
        CurrentlyInDeck[random] = holder;
    }
}

The documentation for Random.Next says (emphasis mine):

A 32-bit signed integer greater than or equal to minValue and less than maxValue; that is, the range of return values includes minValue but not maxValue.

That means you are only generating a random number between 0 and 50 inclusive. That means you can never end up with the last item in its original place as it is only swapped on the very last iteration of the loop. That's very bad for a Shuffle.

Answer 2

Let me first say that I admire the thought you've put into class and function names. For example, Create.TheirDecks() has a sort of fluent feel to it, which I appreciate. "Your code should read like a sentence" is advice I often give, but it seems you've already taken that to heart.

This isn't a complete answer (yet), but here are my notes on just the Deck class.

1. There is a temporal coupling for users of the Deck class. If I instantiate a new Deck(), and then Shuffle() it, I will get a null reference exception because I failed to call GenerateDeck() in between. Is there ever value in having a new deck with no cards? I would say no, new decks should always have cards. That would mean the code to add cards belongs in (or should be called from) the constructor.

2. The code to fill the deck with cards uses two for loops. I am against use the use of for, for anything but generating a sequence of numbers. I would highly recommend using foreach to iterate through your Suits and Faces; you can do this with Enum.GetValues.

3. The Cards in the deck are exposed as a public list. This means that when I want to draw from the deck, I have to manage the list myself. This is an excellent opportunity to practice encapsulation. You can make things simpler for users of the class, and at the same time make it easier for yourself to refactor in the future, by providing a public Card Draw() method and making CurrentlyInDeck private. Then, if you later decide that you want to make CurrentlyInDeck a Stack, or a Queue? You can make that change without touching any code outside of the class (and be sure you haven't broken code anywhere else either).

4. The Shuffle method is pretty good! This is one of those rare cases where I agree that a for loop is the right choice. If you want to implement a proper Fisher-Yates shuffle, I believe you'll only need to change to rand.Next(i, 51). The other change I might make here is to add a private void SwapCards(int i, int j) method, so that the outline of the algorithm is as readable as possible.

5. The PrintDeck method is a bit odd.

   1. It's a method of the Deck class, and it's also got Deck in the name. If nothing else, I'd rename this to just Print.
   2. It prints (because of the behavior of Card) directly to Console. Say I want to print to a file, or log to a database? I think you'd be better off returning a string representation the deck, so that I can do what I want with it. In fact, the best way to do that would probably be to override ToString()
   3. If it's code used only for testing, now that your class is developed, you might as well just delete it.

6. This is a very minor point, but I would much rather see i += 1 than i++. The fact that the ++ hieroglyph adds one to the variable it comes after (or sometimes before!) is not obvious or explicit, and typing 6 characters instead of 3 will certainly not harm any developer. The fact that ++ even exists in C# is a major regret (#3) of one of the language's designers.

I know that's a lot of notes, but I want to emphasize that this is pretty good code for a beginner! I will check back in with some more notes when I get some more time.

Answer 3

Let me add something that no one else seems to have mentioned before. You should not be using public fields, unless its a constant or readonly, but instead should use public properties. Consider the Card class for example, but you are guilty with your other classes.

class Card
{
    public Suit suit;
    public Face face;

    public void PrintCard()
    {
        Console.Write("{0} of {1}", face, suit);
    }
}

I would want to see:

• explicit access modifier
• readonly properties instead of fields
• Property names should be PascalCased.
• method name PrintCard could be simplified to be Print but really could be ToString or a Name property.

So that would be rewritten as:

public class Card
{
    public Suit Suit { get; }
    public Face Face { get; }

    public Card(Face face, Suit suit)
    {
        Face = face;
        Suit = suit; 
    }

    public string Name => $"{Face}{Suit}";
    public override string ToString() => Name;
}

You don't need to populate them based on magic numbers or the deck size. Rather set them directly. Example:

public void GenerateDeck()
{
    CurrentlyInDeck = new List<Card>();  // Not concerned with count.
    var suits = Enum.GetValues(typeof(Suit)).Cast<Suit>().ToList();
    var faces = Enum.GetValues(typeof(Face)).Cast<Face>().ToList();
    foreach (var suit in suits)
    {
        foreach (var face in faces)
        {
            CurrentlyInDeck.Add(new Card(face, suit));
        }
    }
}

Answer 4

I don't like public void SetName(). This makes unit testing problematic. Pass the user name in the constructor.

Bad shuffle use Fischer Yates.

Assign suit and face directly to card and then add it.

I would rather you pass the deck to public void Shuffle() rather than reference global variables.

isPlaying is always true. What purpose does it serve?

This can be done with less code

namespace War
{
    class Program
    {
        static void Main(string[] args)
        {
            War();
        }
        private static Random rand = new Random();
        public static void War()
        {
            Console.WriteLine("What is you name?");
            string name = Console.ReadLine();
            Player human = new Player(name);
            Player computer = new Player("computer");
            bool play = true;
            while (play)
            {
                for (int i = 0; i < 52; i++)
                {
                    if (human.Deck[i].RankI > computer.Deck[i].RankI)
                    {
                        human.Wins++;
                    }
                    else
                    {
                        computer.Wins++;
                    }
                }
                Console.WriteLine($"computer {computer.Wins}  {name} {human.Wins}");
                Console.WriteLine("Enter c to play again");
                string playS = Console.ReadLine();
                if (playS == "c" || playS == "C")
                {
                    human.Deck = Shuffle(human.Deck);
                    computer.Deck = Shuffle(computer.Deck);
                    human.Wins = 0;
                    computer.Wins = 0;
                }
                else
                {
                    play = false;
                }
            }
        }
        public enum suit { Hearts, Diamonds, Clubs, Spades };
        public enum rank { Two, Three, Four, Five, Six, Seven, Eight, Nine, Ten, Jack, Queen, King, Ace };
        public class Card
        {
            private int I;
            public suit Suit { get { return (suit)(I / 13); } }
            public rank Rank { get { return (rank)(RankI); } }
            public int RankI { get { return I % 13; } }
            public override string ToString()
            {
                return $"{Rank} {Suit}";
            }
            public Card (int i)
            {
                I = i;
            }
        }
        public class Player
        {
            public string Name { get; }
            public int Wins { get; set; }
            public List<Card> Deck { get; set; } = new List<Card>();
            public Player(string name)
            {
                Name = name;
                for (int i = 0; i < 52; i++)
                {
                    Card card = new Card(i);
                    Deck.Add(card);
                    //Debug.WriteLine(card.ToString());
                }
                Deck = Shuffle(Deck);
                //Debug.WriteLine("");
                //for (int i = 0; i < 52; i++)
                //{
                //    Debug.WriteLine(Deck[i].ToString());
                //}
            }
        }
        public static List<Card> Shuffle(List<Card> deck)
        {
            List<Card> shuffled = new List<Card>(deck);
            for (int i = shuffled.Count - 1; i > 0; i--)
            {
                int random = rand.Next(i + 1);
                if(random != i)
                {
                    Card cardR = shuffled[random];
                    shuffled[random] = shuffled[i];
                    shuffled[i] = cardR;
                }
            }
            return shuffled;
        }          
    }  
}