Using LINQ to shuffle a deck

Question

I am attempting to write a simple card game. In an effort to come up with a good shuffling algorithm I came across Jeff Atwood's post on Coding Horror.

However When I view the contents of the object after calling the Constructor they are not shuffled.

Here is my attempt to use Jeff's Solution:

class MainDeck : List<Card>
{
   public MainDeck()
    {
        this.Add(new Card(1, "Hearts"));
        this.Add(new Card(2, "Hearts"));
        this.Add(new Card(3, "Hearts"));
        ...

        this.OrderBy(a => Guid.NewGuid());
    }
}

here is the code for Card:

class Card
    {
        string suit;
        int value;

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

        bool isFaceCard()
        {
            if (value >= 11 || value == 1)
                return true;
            else
                return false;
        }

        public override string ToString()
        {
            return (value +", " + suit);
        }
    }

What should I change to make the shuffling work?

Please show the code for Card. Maybe it's just me, but without that I can't see how we can help. — David Arno, Oct 05 '13 at 18:58
You need to understand that `OrderBy` *returns* an ordered collection - it doesn't sort in-place. I'd also argue that Jeff is just shifting from one source of pseudo-randomness to another - using a modified Fisher-Yates shuffle with a decent source of randomness is neater than using OrderBy, IMO. — Jon Skeet, Oct 05 '13 at 19:00
Code Added. However the question has nothing to do with the Card Class, it has to do with any kind of enumerable object — jth41, Oct 05 '13 at 19:01
@JonSkeet How could I order the contents of my object by Guid then? — jth41, Oct 05 '13 at 19:03
Using Guid for sorting may be a bad idea. See Eric Lippert's comment on [a different answer](http://stackoverflow.com/a/3169165/945456) that states "**Use guids to generate uniqueness, never randomness**". — Jeff B, Apr 11 '16 at 20:51

score 32 · Accepted Answer · edited May 23 '17 at 12:03

32

LINQ methods are not mutating existing collections. So this statement does nothing at all: this.OrderBy(a => Guid.NewGuid()); Also, I'm pretty sure you can't assign to this, so you have to either don't inherit from List<T> (which is good), or do something like this:

var sorted = this.OrderBy(a => Guid.NewGuid()).ToList();
this.Clear();
this.AddRange(sorted);

Also look at this SO answer, there is more correct shuffling algorithm.

edited May 23 '17 at 12:03

Community

1
1

answered Oct 05 '13 at 19:01

Display Name

8,022
3
31
66

Where did you get the `AddAll` function? – jth41 Oct 05 '13 at 19:09
@jth41 this was a mistake (didn't remember the name of that method). Edited. – Display Name Oct 05 '13 at 19:11
using your solution, `this` ends up with 0 elements after attempting `this.AddRange(sorted);` – jth41 Oct 05 '13 at 19:14
@SargeBorsch your query does not work without `ToList()`, because it's not executed until `AddRange` is being called, so you're running the query on empty list (as `Clear` was already called). You always have to remember that LINQ queries are lazy and deferred. – MarcinJuraszek Oct 05 '13 at 19:36
@MarcinJuraszek oops, you catched me! (I did know this) :) – Display Name Oct 05 '13 at 19:37
7

Found an interesting comment by Eric Lippert on [a different answer](http://stackoverflow.com/a/3169165/945456) explaining why you shouldn't use Guid to sort: "it is legal for guids to be generated sequentially from an initial random element.... **Use guids to generate uniqueness, never randomness**" (edited for brevity, emphasis by me). – Jeff B Apr 11 '16 at 20:49
I only "used" it to keep the code obviously similar to the code in the question. *of course,* this is inadequate solution. – Display Name Apr 11 '16 at 22:24

score 16 · Answer 2 · edited Apr 19 '20 at 23:24

16

Use this extension method

public static class Extensions
{
    public static IEnumerable<T> Randomize<T>(this IEnumerable<T> source)
    {
        Random rnd = new Random();
        return source.OrderBy((item) => rnd.Next());
    }
}

edited Apr 19 '20 at 23:24

RJFalconer

10,890
5
51
66

answered Dec 20 '17 at 06:59

Masoud Darvishian

3,754
4
33
41

score 6 · Answer 3 · answered Oct 05 '13 at 19:04

6

Try this

 public void Shuffle()
 {
     Random r = new Random();
     this.Sort((x, y) => r.Next(-1, 1));
 }

Because of Linq's deffered execution following line doesn't get executed.

this.OrderBy(a => Guid.NewGuid());

This just creates the query but never executed. Even if executed it won't change your collection.

Don't forget Linq is a way to query data, not mutate it.

answered Oct 05 '13 at 19:04

Sriram Sakthivel

72,067
7
111
189

3

You shouldn't use non-pure functions in Sort, because (by contract) `.Sort()` is free to evaluate your lambda multiple times. Instead you should use `.Select()` to add a random number into a anonymous struct, and then sort by that. – Simon Nov 10 '16 at 14:06

Using LINQ to shuffle a deck

3 Answers3

Linked