What's a good way to maintain the functionality while removing the chance of stack overflow exceptions

Question

I'm doing this in a really inefficient way. Not very experienced just trying stuff hoping it will work and I'm getting (understandably) nailed with stack overflow exceptions.

So the first part of my code here is pretty obvious, there's 33 cards in the deck, I render 3 at "random" and my attempt at exception handling (the "CheckForReDraw();") method to prevent duplicates is where things get... messy

public void DrawThreeUniqueGladiatorCards()
{
    if (numberOfDiscards >= 30)
    {
        ShuffleGladCards();
    }

    Random gladCard = new Random();
    drawnGladCard1 = gladCard.Next(1, 34);

    Random gladCard2 = new Random();
    drawnGladCard2 = gladCard.Next(1, 34);

    Random questCard3 = new Random();
    drawnGladCard3 = gladCard.Next(1, 34);

    CheckForReDraw();
}

public void CheckForReDraw()
{

    if (drawnGladCard1 == drawnGladCard2 || drawnGladCard1 == drawnGladCard3 || drawnGladCard2 == drawnGladCard3)
    {
        DrawThreeUniqueGladiatorCards();
    }
    if (glad1Board == true || glad1Discard == true)
    {
        if (drawnGladCard1 == 1 || drawnGladCard2 == 1 || drawnGladCard3 == 1)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad2Board == true || glad2Discard == true)
    {
        if (drawnGladCard1 == 2 || drawnGladCard2 == 2 || drawnGladCard3 == 2)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad3Board == true || glad3Discard == true)
    {
        if (drawnGladCard1 == 3 || drawnGladCard2 == 3 || drawnGladCard3 == 3)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad4Board == true || glad4Discard == true)
    {
        if (drawnGladCard1 == 4 || drawnGladCard2 == 4 || drawnGladCard3 == 4)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad5Board == true || glad5Discard == true)
    {
        if (drawnGladCard1 == 5 || drawnGladCard2 == 5 || drawnGladCard3 == 5)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad6Board == true || glad6Discard == true)
    {
        if (drawnGladCard1 == 6 || drawnGladCard2 == 6 || drawnGladCard3 == 6)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad7Board == true || glad7Discard == true)
    {
        if (drawnGladCard1 == 7 || drawnGladCard2 == 7 || drawnGladCard3 == 7)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad8Board == true || glad8Discard == true)
    {
        if (drawnGladCard1 == 8 || drawnGladCard2 == 8 || drawnGladCard3 == 8)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad9Board == true || glad9Discard == true)
    {
        if (drawnGladCard1 == 9 || drawnGladCard2 == 9 || drawnGladCard3 == 9)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad10Board == true || glad10Discard == true)
    {
        if (drawnGladCard1 == 10 || drawnGladCard2 == 10 || drawnGladCard3 == 10)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad11Board == true || glad11Discard == true)
    {
        if (drawnGladCard1 == 11 || drawnGladCard2 == 11 || drawnGladCard3 == 11)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad12Board == true || glad12Discard == true)
    {
        if (drawnGladCard1 == 12 || drawnGladCard2 == 12 || drawnGladCard3 == 12)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad13Board == true || glad13Discard == true)
    {
        if (drawnGladCard1 == 13 || drawnGladCard2 == 13 || drawnGladCard3 == 13)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad14Board == true || glad14Discard == true)
    {
        if (drawnGladCard1 == 14 || drawnGladCard2 == 14 || drawnGladCard3 == 14)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad15Board == true || glad15Discard == true)
    {
        if (drawnGladCard1 == 15 || drawnGladCard2 == 15 || drawnGladCard3 == 15)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad16Board == true || glad16Discard == true)
    {
        if (drawnGladCard1 == 16 || drawnGladCard2 == 16 || drawnGladCard3 == 16)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad17Board == true || glad17Discard == true)
    {
        if (drawnGladCard1 == 17 || drawnGladCard2 == 17 || drawnGladCard3 == 17)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad18Board == true || glad18Discard == true)
    {
        if (drawnGladCard1 == 18 || drawnGladCard2 == 18 || drawnGladCard3 == 18)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad19Board == true || glad19Discard == true)
    {
        if (drawnGladCard1 == 19 || drawnGladCard2 == 19 || drawnGladCard3 == 19)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad20Board == true || glad20Discard == true)
    {
        if (drawnGladCard1 == 20 || drawnGladCard2 == 20 || drawnGladCard3 == 20)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad21Board == true || glad21Discard == true)
    {
        if (drawnGladCard1 == 21 || drawnGladCard2 == 21 || drawnGladCard3 == 21)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad22Board == true || glad22Discard == true)
    {
        if (drawnGladCard1 == 22 || drawnGladCard2 == 22 || drawnGladCard3 == 22)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad23Board == true || glad23Discard == true)
    {
        if (drawnGladCard1 == 23 || drawnGladCard2 == 23 || drawnGladCard3 == 23)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad24Board == true || glad24Discard == true)
    {
        if (drawnGladCard1 == 24 || drawnGladCard2 == 24 || drawnGladCard3 == 24)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad25Board == true || glad25Discard == true)
    {
        if (drawnGladCard1 == 25 || drawnGladCard2 == 25 || drawnGladCard3 == 25)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad26Board == true || glad26Discard == true)
    {
        if (drawnGladCard1 == 26 || drawnGladCard2 == 26 || drawnGladCard3 == 26)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad27Board == true || glad27Discard == true)
    {
        if (drawnGladCard1 == 27 || drawnGladCard2 == 27 || drawnGladCard3 == 27)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad28Board == true || glad28Discard == true)
    {
        if (drawnGladCard1 == 28 || drawnGladCard2 == 28 || drawnGladCard3 == 28)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad29Board == true || glad29Discard == true)
    {
        if (drawnGladCard1 == 29 || drawnGladCard2 == 29 || drawnGladCard3 == 29)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad30Board == true || glad30Discard == true)
    {
        if (drawnGladCard1 == 30 || drawnGladCard2 == 30 || drawnGladCard3 == 30)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad31Board == true || glad31Discard == true)
    {
        if (drawnGladCard1 == 31 || drawnGladCard2 == 31 || drawnGladCard3 == 31)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad32Board == true || glad32Discard == true)
    {
        if (drawnGladCard1 == 32 || drawnGladCard2 == 32 || drawnGladCard3 == 32)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
    if (glad33Board == true || glad33Discard == true)
    {
        if (drawnGladCard1 == 33 || drawnGladCard2 == 33 || drawnGladCard3 == 33)
        {
            DrawThreeUniqueGladiatorCards();
        }
    }
}

I was messing around with all kinds of stuff trying (arrays mainly) to figure out to notify the random functions when a card has been selected and only pick from the REMAINING cards in the deck without using this ludicrous method. Any pointers or reference material is greatly appreciated.

How do I keep my card game from randomly drawing the same card twice in one hand?

In your case draw the next random card from the set of available cards. If you draw a Queen of Hearts, remove that card from the available cards (the deck) and pick a random card from the remaining cards.

This means your current approach of randomly picking a value and a suit is not suitable. Instead you could e.g. shuffle the array once in the beginning and pick the first 5 cards (the analogy to how real people play cards is striking here). Each entry in the array would have to uniquely identify a card so it would be a valid {Value, Suit} combination."

I found this post useful in theory but I can't seem to figure out how to implement it myself.

This `new Random();`x3 is one reason for it, Probably the main reason — Rainbow, May 13 '18 at 20:49
Thank you, I had them running without issue previous to the CheckForReDaw(); method - and when there are several gladiator cards in play "on board" or discarded the stack overflow happens much more frequently, it seems the nature of the recursion is triggering it. I want to keep the functionality but change to iteration lol I'm such a noob. Thanks again for your feedback and I will instantiate separate version of the random class — Jae Bolton, May 13 '18 at 20:53
oh i didn't even see that you calling two functions within each other, yup that should do it, You probably cluttering the heap as well with all those random objects, you only need one to be global and call `Next()` from within the methods — Rainbow, May 13 '18 at 20:55
Als long as you don't use recursive calls, the changes to get a stack overflow are very dim. You distinguish a lot of cases but then do always the same (`DrawThreeUniqueGladiatorCards();`). Can't this be simplified? — Olivier Jacot-Descombes, May 13 '18 at 20:55

John Wu · Accepted Answer · 2018-05-13T21:50:52.443

2

Create a deck class, populate it with cards, randomize the order (not the cards), and draw from it, just as you would a real deck.

First, create a class that represents a card:

class Card
{
    private readonly int suit;
    private readonly int rank;

    public Card(int suit, int rank)
    {
        this.suit = suit;
        this.rank = rank;
    }

    public int Suit { get { return suit; } }

    public int Rank{ get { return rank; } }
}

Now that we have a Card class, we need something to hold them. Cards are dealt one at a time from the top of the deck, so it seems like a Queue is suitable.

We will wrap that queue in a class called Deck, and give it a couple methods to populate itself, randomize the order, and deal cards.

class Deck 
{
    private Queue<Card> cards = new Queue<Card>();  //Here's our queue
    private readonly Random random = new Random();

    //This is for a standard deck, but you could make it work it your special deck
    //We just loop and make sure there is one of each possible card
    private virtual IEnumerable<Card> CreateFreshDeck()
    {
        for (var suit = 1; suit<=4; suit++)
            for (var rank = 1; rank <=13; rank++)
                yield return new Card(suit,rank);
    }

    public void Shuffle()
    {
        this.cards = new Queue<Card>
        (
            CreateFreshDeck().OrderBy( a => random.Next() )  //This is where the shuffling happens
        );
    }

    public int CardsRemaining
    {
        get { return cards.Count; }  
    }

    public Card DealOne()
    {
        return this.cards.Dequeue();    
    }

    public IEnumerable<Card> DealMany(int count)
    {
        return this.cards.Take(count);
    }
}

Now you can just do this:

var d = new Deck();
d.Shuffle();
while (!gameOver)
{
    var card = d.DealOne();
    //etc.

edited May 13 '18 at 21:50

answered May 13 '18 at 20:59

John Wu

50,556
8
44
80

Thank you very much for your feedback this will take me some time to process and make sense of.. I see what you mean by the order and not the cards – Jae Bolton May 13 '18 at 21:03
@JaeBolton I don't want to underestimate you, but if you find it hard to understand this answer, read up on LINQ – Camilo Terevinto May 13 '18 at 21:04
I kind of like your approach. Realizing OP is new at programming, it would be nice if you detailed better how OrderBy works, with emphasis on Comparers and predicate-s in general. Otherwise it's going to be 100% obscure... – Attersson May 13 '18 at 21:06
1

@John Wu https://stackoverflow.com/questions/1287567/is-using-random-and-orderby-a-good-shuffle-algorithm – Eser May 13 '18 at 21:19
Does this example assume a class for the cards named Card or am I way off lol @ John Wu – Jae Bolton May 13 '18 at 21:25
Thank you so much for taking time to modify your answer to show the class, I was trying my best to implement your solution but was stuck making the card class, my cards are easier than a normal deck in that they have no suit – Jae Bolton May 13 '18 at 21:53
@Eser thanks for the valid point. You saved my day from learning something inefficient. – Attersson May 13 '18 at 22:01

Attersson · Answer 2 · 2018-05-17T17:29:00.590

0

Your code contains many "hardcoded" conditions and redundant recursions.

Consider using this construct:

bool error = false;
while(!error){
    error = someDrawFunction(args);
}

which will repeat the function if it returns true (in this example) instead to keep stacking calls recursively.

Anyway I would recommend using a list to store your cards. Every time you draw one card, you have N available (N == list::size). Draw a number from 0 to N-1 and remove the Nth indexed element from the list. Now the list::size is decreased by 1 because the list has N-1 cards. Draw another card from the smaller list. Every time you are casting a random from 0 to size-1 or from 1 to size, equivalently. No recursions are needed.

edited May 17 '18 at 17:29

answered May 13 '18 at 20:52

Attersson

4,755
1
15
29

1

Thank you that sounds much better, I will dig and see how to implement a list. My code must look just awful to an experienced coder lol, I'm self taught - wish to make a board game that doesn't exist yet and I'm quite committed – Jae Bolton May 13 '18 at 20:57
Glad to help. You can also use std::list already fully implemented – Attersson May 13 '18 at 20:58
Ah yes of course this is C# -_-'' Then use System.Collections.Generic List – Attersson May 13 '18 at 21:03

What's a good way to maintain the functionality while removing the chance of stack overflow exceptions

2 Answers2