Creating lists of Card objects

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?

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.

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 4×13=52 cards, your full set method only generates 4×12, 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