CODEWITHSUNDEEP
c#unity-game-engine
Shaun White
Shaun White

Reputation: 1

For Loop isnt being fully completed

I have a For Loop which handles taking cards from the master deck, and putting them in a random order into the players deck. The code is:

for(int a = 0; a < deckManager.DeckAllCardsPlayer.Count; a++){
        int b = Random.Range(0, deckManager.DeckAllCardsPlayer.Count);
            if(!PlayerDeck.Contains(deckManager.DeckAllCardsPlayer[b])){
            PlayerDeck.Add(deckManager.DeckAllCardsPlayer[b]);
            deckManager.DeckAllCardsPlayer.RemoveAt(b);
        } 
    }

There are 16 cards in the master deck, but this for loop only does 8. Can someone figure out why? Originally, it was adding some cards multiple times, which is why I added the '!PlayerDeck.Contains' statement. I have no idea why it's only doing 8 of 16.

Upvotes: 0

Views: 80

Answers (5)

Enigmativity
Enigmativity

Reputation: 117064

Assuming PlayerDeck & deckManager.DeckAllCardsPlayer are both lists then just do this:

PlayerDeck.AddRange(deckManager.DeckAllCardsPlayer.OrderBy(x => Random.value));
deckManager.DeckAllCardsPlayer.Clear();

Then you don't have to worry about removing elements while you iterate (which you should never do).

Upvotes: 1

Tien Nguyen Ngoc
Tien Nguyen Ngoc

Reputation: 1555

int count = deckManager.DeckAllCardsPlayer.Count;
for(int a = 0; a < count; a++){
        int b = Random.Range(0, deckManager.DeckAllCardsPlayer.Count);
            if(!PlayerDeck.Contains(deckManager.DeckAllCardsPlayer[b])){
            PlayerDeck.Add(deckManager.DeckAllCardsPlayer[b]);
            deckManager.DeckAllCardsPlayer.RemoveAt(b);
        } 
    }

You must use count variable. Because deckManager.DeckAllCardsPlayer.Count will be changed after run deckManager.DeckAllCardsPlayer.RemoveAt(b). I hope it will work for you.

Upvotes: 0

Sunil Purushothaman
Sunil Purushothaman

Reputation: 9491

The upperbound or limit of the loop is changing as you remove cards from deck. To fix it with just 2 line code change, Try this

int count = deckManager.DeckAllCardsPlayer.Count;
for(int a = 0; a < count; a++){
        int b = Random.Range(0, deckManager.DeckAllCardsPlayer.Count);
            if(!PlayerDeck.Contains(deckManager.DeckAllCardsPlayer[b])){
            PlayerDeck.Add(deckManager.DeckAllCardsPlayer[b]);
            deckManager.DeckAllCardsPlayer.RemoveAt(b);
        } 
    }

Upvotes: 1

matt_t_gregg
matt_t_gregg

Reputation: 192

You start off with 16 cards, but remove one each time.

As a result, although a decrease by one each iteration, the value of deckManager.DeckAllCardsPlayer.Count goes down.

After 8 iterations, a is 7, but the size of DeckAllCardsPlayer has been reduced to 8. Hence the loop terminates on the next turn.

One way to work around would be to take the count up front and store in an integer:

int totalCards = deckManager.DeckAllCardsPlayer.Count;
for(int a = 0; a < totalCards; a++){
    ... etc.

Although there are many other ways, depending on the logic that you want to expose.

This question may be enlightening : Is the condition in a for loop evaluated each iteration?

Upvotes: 3

user94559
user94559

Reputation: 60143

The issue is that deckManager.DeckAllCardsPlayer.Count is getting smaller at each iteration. Try this instead:

while (deckManager.DeckAllCardsPlayer.Count > 0) {
    int b = Random.Range(0, deckManager.DeckAllCardsPlayer.Count);
    PlayerDeck.Add(deckManager.DeckAllCardsPlayer[b]);
    deckManager.DeckAllCardsPlayer.RemoveAt(b);
}

I removed the conditional, because it shouldn't be necessary. (Unless the starting deck has duplicates? If so, just put it back in.)

Upvotes: 4

Related Questions