3
\$\begingroup\$

So I am trying to create a small C# application which generates a deck of cards, and ask to the user if they want to shuffle the deck, and then display 1 card at a time to the user. Would this be effective use of classes 4 rules of oop? Thanks

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Assignment2OOP
{
    class Program
    {
        static void Main(string[] args)
        {
            // Call method to start the program
            DrawCards();

            Console.ReadKey();
        }

        // Short program which deals one card from deck at a time and also shuffles the deck
        private static void DrawCards()
        {
            Deck newDeck = new Deck();

            Console.WriteLine("Would you like to shuffle your deck of cards? (type yes)");
            string option = Console.ReadLine();
            if(option == "yes" || option == "Yes")
            {
                newDeck.Shuffle();
                Console.Clear();
            }

            while(true)
            {
                Console.WriteLine("Would you like to draw a card?" +
                                  "\n> Type 'yes' for yes\n> Type 'anything else' for no");
                string answer = Console.ReadLine();
                
                if (answer == "yes" || answer == "Yes" || answer == "y")
                {
                    Console.Clear();
                    Console.WriteLine($"> {newDeck.Deal()}\n");
                }
                else
                {
                    Console.WriteLine("No more cards will be drawn..");
                    break;
                }
            }
        }
    }
}
namespace Assignment2OOP
{
    class Card
    {
        // Fields 
        public List<string> Suit { get; private set; }
        public List<string> Value { get; private set; }

        // Constructor
        public Card()
        {
            Suit = new List<string> { "Spades", "Clubs", "Hearts", "Diamonds" };
            Value = new List<string> { "2", "3", "4", "5", "6", "7", "8", "9", "10",
                                       "Ace", "Queen", "Jack", "King"};
        }

        // Makes a set of unqiue cards
        public List<string> GetSetOfCards()
        {
            List<string> cardsSet = new List<string>();

            foreach (var suit in Suit) foreach (var value in Value) cardsSet.Add(value + " of " + suit);

            return cardsSet;
        }
    }
}
namespace Assignment2OOP
{
    class Deck
    {
        // Fields
        public List<string> cards { get; private set; }

        // Constructor 
        public Deck()
        {
            cards = new List<string>();
            FillDeck();
        }
        
        // Fill up the deck when a new object is instansiated
        private void FillDeck()
        {
            Card setOfCards = new Card();
            foreach(string card in setOfCards.GetSetOfCards()) cards.Add(card);
        }

        public void Shuffle()
        {
            // Use Guid to shuffle the list
            //cards = cards.OrderBy(x => Guid.NewGuid()).ToList();

            // Or use more common way to shuffle
            Random rnd = new Random();
            int count = cards.Count;
            
            while(count > 1)
            {
                count--;
                int rng = rnd.Next(count);
                var value = cards[rng];
                cards[rng] = cards[count];
                cards[count] = value;
            }
        }

        // Keep an index of the top card of the deck
        private int topCardIndex = 51;
        public string Deal()
        {
            // Mark sure the deck isnt empty
            if(topCardIndex > 0)
            {
                // Get card at top and return it and decrease topCardIndex
                string topCard = cards[topCardIndex];
                topCardIndex--;
                return topCard;
            }
            else
            {
                return "No more cards left in the deck!";
            }
        }

        // Method which shows user all cards in current deck
        public void DisplayDeck()
        {
            foreach(var card in cards) Console.WriteLine(card);
        }
    }
}
```
\$\endgroup\$
6
  • \$\begingroup\$ At the first glance I would say the abstractions and encapsulation are nailed. On the other hand it does support only one type of card set (French-suited cards). In case of OOP it might make sense to use inheritance and polymorphism to support different type of cards. \$\endgroup\$ Mar 23 at 14:59
  • \$\begingroup\$ @PeterCsala thanks for the feedback,yeh thats a good idea i will look to implement that. \$\endgroup\$
    – Mj _
    Mar 23 at 15:06
  • \$\begingroup\$ I disagree with @PeterCsala that you nailed it. Card is not really a card but a list of cards, which suggests it should be in Deck. And DisplayDeck is a UI feature, so it should not be in Deck. \$\endgroup\$
    – Rick Davin
    Mar 23 at 15:07
  • \$\begingroup\$ @RickDavin How would go about making the Cards class so that you can have a list of cards in the Deck class,as i struggled on that part. \$\endgroup\$
    – Mj _
    Mar 23 at 15:09
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see What should I do when someone answers my question? as well as what you may and may not do after receiving answers. \$\endgroup\$ Mar 24 at 18:08
2
\$\begingroup\$

Sorry for quick review. Work is pressing but rather than a conversation in comments, this will have to suffice.

There are lots of examples of C# and card games here at CR. Maybe you could search for them and review since many of those answers are quite good.

The better answers use enum for Suit and Rank. A Card class or struct is therefore a simple pairing of a Suit and Rank. Granted later one must assign a value to a card based on rules of a particular game, e.g. is an Ace a value of 1 or 13 in Poker, or is it a 1 or 11 in BlackJack.

A Deck is a collection of cards. I am not thrilled with names like GetSetofCards or FillDeck. I am partial to CreateStandardDeck.

For the Shuffle method, the Fisher-Yates shuffle is the most highly recommended. Typically it is wise to include a comment that you are using it just so any reviewer giving it a fast glance will see it.

One of the messiest non-OOP things I see in your code is mixing UI related things in with the logic. Deal is returning a string. To me, a deal should deal a hand, or else a set of hands to different players. Maybe GetNextCard is a more appropriate name, or perhaps GetTopCard. But that method should return a Card, not a string. The UI is what displays a card, so move that logic back to what controls the console display.

Likewise DisplayDeck should be moved out of the Deck class and back to your UI class. Except you really don't have a separate UI-related class other than the 2 methods in Program. You may consider having a UI only class and move DrawCards into it, as well as these other UI methods I have touched upon.

I see a bit of repetition when asking for console input and checking an answer. You may want to streamline this into a single method where you pass in a question and the acceptable answer. If some answers are case sensitive, then you would also want to pass in a flag to ignore case so that "yes", "YES", and "Yes" are all the same good answer.

But again, my best advice is to search on [C#] and Card here on Code Review. There is a wealth of information in some of those answers.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the overall help, but some of the stuff you said was actually asked by the task - ibb.co/3hsTXVD Here is link the task image. And we also havent been taught to use enums at all,so iam trying to find a way to create the cards without enums,cause surely there must be away. \$\endgroup\$
    – Mj _
    Mar 23 at 18:47
  • \$\begingroup\$ @Mj_ Do not take refuge in what being limited by what the task stated. This will limit your progress as a developer. Plus, you fall short of some of the directives about the task, e.g. that the Card needs to be a card and not a list of cards. \$\endgroup\$
    – Rick Davin
    Mar 23 at 20:47
  • \$\begingroup\$ I updated the code would you say this is slighty better way? \$\endgroup\$
    – Mj _
    Mar 24 at 18:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.