2
\$\begingroup\$

I need to create two lists of Card objects in C#.

A Card class is defined as:

public class Card {
    public Suits Suit { set; get; }
    public Values Value { set; get; }
    public static Random rand = new Random();

    public Card() {

    }

    public Card(Suits s, Values v) {
        Suit = s;
        Value = v;
    }

    public override string ToString() {
        return Suit.ToString() + " that has a value of " + Value.ToString();
    }

    public Card RandomCardGenerator() {
        Suit = (Suits)rand.Next(4);
        Value = (Values)rand.Next(1, 14);
        return this;
    }

In a different class called Action:

   static class Action {
        static public List< Card > CreateRandomCardList() {
            List< Card > cardList1 = new List< Card >();

            for (int loop = 0; loop < 12; loop++) {
                cardList1.Add(new Card().RandomCardGenerator());
            }

            return cardList1;
        }

        static public List < Card > CreateFullSetCard() {
            List< Card > cardList2 = new List< Card >();

            for (int s = 0; s < 4; s++) {
                for (int v = 0; v < 13; v++)
                    cardList2.Add(new Card((Suits)s, (Values)v));
            }

            return cardList2;
        }
    }

CreateRandomCardList() is later called upon:

public partial class Form1 : Form {
        public Form1() {
            InitializeComponent();
            PopulateListBox1(Action.CreateRandomCardList());
        }

        private void PopulateListBox1(List< Card > l) {
            listBox1.DataSource = l;
        }
}

I used static for class Action.

It makes sense to create instances of Card, but is it necessary to create an instance of Action? Can I keep it static?

\$\endgroup\$
  • \$\begingroup\$ We should see how those classes are used. For example: how do you test class which uses Action? To have a well-known input is important for its tests and static methods cannot be easily mocked. I'd rethink about the name: it's not descriptive and it also conflicts with System.Action (not a big issue if it was the "right" name) \$\endgroup\$ – Adriano Repetti Jan 21 '18 at 10:57
  • \$\begingroup\$ @YuZhang - further to what Mr Adriano has said - it all depends on what you are using the classes for. Another issue is you gotta decide whether you want the card class to be simply a: (i) data structure, or (ii) an object (which hides its data behind abstractions). If it's a data structure, you could even let the suit classes be primitive types (e.g. strings) and expose them. one or the other is my recommendation. I'd leave it as a simple datastructure and put all methods pertaining to the card itself outside. \$\endgroup\$ – BKSpurgeon Jan 21 '18 at 11:29
  • \$\begingroup\$ @BKSpurgeon, primitive types (or any simpler data structure) offers a better performance or it is simply about a better logic? (Since it is too simple to be an object?) \$\endgroup\$ – Yu Zhang Jan 21 '18 at 17:15
  • 1
    \$\begingroup\$ @YuZhang - because it's simpler. then again I haven't seen what the Suits class does. the basic point is this: decide whether you want to create an object or a data structure. in this case i think the latter might better suit your purposes because the Card class doesn't have any meaningful methods \$\endgroup\$ – BKSpurgeon Jan 21 '18 at 22:49
1
\$\begingroup\$

It's unclear what CreateRandomCardList is intended for during gameplay. I know, technically, it's generating random cards but then why just 11 (loop < 12), why not 13?

Nevertheless, I strongly feel, the Action class is clearly misleading. I would instead prefer to have a Deck which knows what cards it holds. Also, Card type should be as simple as possible, perhaps knowing only Suit and Value. RandomCardGenerator does not belong there.

// Deck is general term. You can more appropriately
// name it as DeckOfCards, CardDeck, or anything you deem fit.
public class Deck {
    private Card[] cards;

    // Setup() is purposely part of Deck creation because without cards deck is useless.    
    public Deck() {
        // fill up 13 cards
        Setup();
    }

    private Cards[] Setup() {} // CreateFullSetCard()
    private Cards[] Shuffle() {}

    //.. so on
}

The Deck should not be static because

  1. It is going to have it's own state.
  2. If there are > 1 players, each will have their own Deck of cards. Even if it's single player, it should not be static for #1 reason.
\$\endgroup\$
  • \$\begingroup\$ I updated my question to include CreateRandomCardList. It is supposed to populate a listBox object on a windows form. \$\endgroup\$ – Yu Zhang Jan 21 '18 at 17:12
1
\$\begingroup\$

IMO, Actions is fine as static.

Though, I agree with others that Action is an unclear name.

As has also been mentioned, RandomCardGenerator is a bit odd. At the very least, it should be a static method that returns a new Card. Better yet it could be moved to a different class (maybe Action).

You have a default constructor Card(), which means it is possible to create "blank" cards which you probably do not want.

Sometimes your sets of cards will contain multiple identical cards. A more "realistic" way to randomize cards is to create a full deck to form an array or collection of all 52 cards, then randomize numbers to pick cards from the deck.

Also note that a "regular" deck of cards have 4x13=52 cards, your full set method only generates 4x12, and your random set returns 11 cards.

Some people (including me) thinks even single line for/if/etc should have curly braces

ListBox1 should be renamed...

It seems in CreateFullSetCard you are creating 0-12, while in RandomCardGenerator you are using 1-13

\$\endgroup\$

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.