Trying to get random values from an array, but instead getting the first 4

Question

I'm creating a poker game in Unity using C#. I have an array card sprites which I assign values to, and then randomly deal. Despite getting the correct random cards, my code is setting their value to 1, 2, 3, etc.

public class DeckScript : MonoBehaviour
{
    public Sprite[] cardSprites;
    int[] cardValues = new int[53];
    int currentIndex = 0;
    public int cardValueNum = 0;

    [...]

    void GetCardValues()
    {
        int num = 0;
        // Loop to assign values
        for (int i = 0; i < cardSprites.Length; i++)
        {
            num = i;
            // Count up to the amount of cards, 52
            num %= 13;
            // If there is a remainder after x/13, then remainder
            // is used as the value, unless over 10, then use 10
            if (num > 10 || num == 0)
            {
                num = 10;
            }
            cardValues[i] = num++;
        }
        currentIndex = 1;
    }

    public void Shuffle()
    {
        for(int i = cardSprites.Length -1; i > 0; --i)
        {
            int j = Mathf.FloorToInt(Random.Range(0.0f, 1.0f) * cardSprites.Length - 1) + 1;
            Sprite face = cardSprites[i];
            cardSprites[i] = cardSprites[j];
            cardSprites[j] = face;

            int value = cardValues[j];
            cardValues[j] = value;
        }
    }

    public int DealCard(CardScript cardScript)
    {
        cardScript.SetSprite(cardSprites[currentIndex]);
        cardScript.SetValue(cardValues[currentIndex]);
        Debug.Log("Card value is " + cardScript.GetValueOfCard());
        currentIndex++;
        return cardScript.GetValueOfCard();
    }
    
    [...]

    public int GetCard()
    {
        // Get a card, use deal card to assign sprite and value to card on table
        int cardValue = deckScript.DealCard(hand[cardIndex].GetComponent<CardScript>());
        // Show card on game screen
        hand[cardIndex].GetComponent<Renderer>().enabled = true;
        // Add card value to running total of the hand
        handValue += cardValue;
        cardIndex++;
        return handValue;
    }

This is one of 4 scripts I'm using, but I believe it has everything relevant. (This is also my first time using StackOverflow, so if I missed something I apologize)

The intended result should be 4 random numbers, all between 1 and 10.

I've traced through my functions to find where the shuffle stops working and starts just listing 1-4. The current placement of the debug in DealCard prints 1, 2, 3, 4, so I'm guessing there may be an issue here.

I loosely followed a tutorial for a lot of the this code, but I'm fairly certain I got everything correct. My best guess is there is something wrong in either GetCard or DealCard.

Answer 1

Well, it looks like you set yourself up for trouble by splitting your logic into "Sprites" and "values".

Unless you made a copy and paste error, the last two lines of your Shuffle loop don't do anything because you're using j on both lines - and not i and j as you probably intended to.

To prevent these kinds of issues, I would use a data-type that holds both the "Sprites" and the "values", then initialize a read-only, immutable array of those at program start.

Your Shuffle could then just simply use an array of 1..53 interger indices into that array - and you could then use something like Interlocked.Exchange to perform an atomic swap on those integers. Depending on whether your Sprite is a class or valuetype, this could also have quite a bit of a performance impact.

Atomically swapping an index into that array prevents you from ever having the situation where the "Sprite" and "value" of a card may somehow gotten out-of-sync. Your typo in that Shuffle loop just demonstrated how such things might possibly happen.

Please be also aware that Random is not a cryptographically secure random number generator. And while it won't make any difference in this particular example, try to avoid "upscaling" if possible. It is completely irrelevant when dealing with small numbers - such as 1..53 poker cards - but if you call some function that returns a floating-point number such as Random.Range and then multiply the result by a large number, then you could possibly lose precision, so it'd be better to ask that function for a number in the "correct" range in the first place.

In your DealCards, you have a similar issue - you're calling two separate functions to change it's state. Which means that it's state is invalid between these two calls. Again, you are setting yourself up for failure. Even if you don't want to merge your data types as I suggested, at least replace those two functions with a single one that takes two inputs. Doing so will cause a compiler error should you ever forget to call both of them.

In that same function, you call GetValueOfCard() twice - with currentIndex++ in between - make that function a property to make in clear that it doesn't have any side-effects.

In your GetCard, you pass a "complicated" parameter into some function that first performs a complex and destructive operation on that parameter, and then a much smaller modifying operation on itself. This is really counter-intuitive and really bad programming practice.

That entire DealCard method should probably be moved into the CardScript class, using a read-only property on DeckScript, as it essentially does an in-place replacement of the CardScript instance.

It could even use some sort of an iterator on DeckScript to automatically advance the index.