3

Using C#/Asp.Net.

I'm trying to achieve the following:

I have a list of price quotes - sometimes there are multiple products with the same price.

Also, some of the results are affiliated (sponsored), so we need to give a preference to those too.

Here's the method that is called:

    public IEnumerable<PriceQuote> BestQuote(int take = 0)
    {
        var q = Quotes.Where(x => x.TotalRepayable == MinPrice)
            .Shuffle()
            .OrderByDescending(x => x.ProductDetail.Product.IsSponsored);

        return take == 0 ? q : q.Take(take);
    }

The code selects items that have the lowest available price. The idea is then to sort them into a completely random order, then sort again by the sponsored flag descending (sponsored = 1 as opposed to 0), then take however many results are required.

I first shuffle them to get a random order - from the random list I want to take the sponsored items first - then if necessary fill the spaces with non sponsored items. The theory is that both sponsored and non-sponsored will be in a random order each time.

Example in natural order:

product1 (not sponsored)
product2 (sponsored)
product3 (not sponsored)
product4 (sponsored)
product5 (not sponsored)
product6 (sponsored)

Shuffle randomly:

product3 (not sponsored)
product1 (not sponsored)
product2 (sponsored)
product6 (sponsored)
product5 (not sponsored)
product4 (sponsored)

Order by sponsored first keeping randomness:

product2 (sponsored) <-- pick these first
product6 (sponsored)
product4 (sponsored)
product3 (not sponsored)
product1 (not sponsored)
product5 (not sponsored)

Here's my Shuffle method:

    public static IEnumerable<T> Shuffle<T>(this IEnumerable<T> @this)
    {
         if (@this.Count() <= 1) return @this;

        return @this.ShuffleIterator(new Random());
    }

    static IEnumerable<T> ShuffleIterator<T>(this IEnumerable<T> source, Random rng)
    {
        var buffer = source.ToList();

        for (int i = 0; i < buffer.Count; i++)
        {
            int j = rng.Next(i, buffer.Count);
            yield return buffer[j];

            buffer[j] = buffer[i];
        }
    }

The issue I have is that when I call the BestQuote method a number of times in succession for different quotes, I tend to get the same results returned. For instance, my list contains 6 products and I make 3 calls selecting the first result each time, chances are that the order is the same for all 3 calls. It' is not always the case - there are some variances, but there are more matches than non matches.

Call 1: product2 <-- 
Call 2: product2 <--
Call 3: product2 <-- this is a common scenario where there seems to be no randomness
John Ohara
  • 2,821
  • 3
  • 28
  • 54
  • 3
    http://stackoverflow.com/questions/767999/random-number-generator-only-generating-one-random-number – Henrik Jan 17 '17 at 12:11
  • 2
    What happens when you use a single static Random instance? Without params `new Random()` uses the current time as seed, and if you instantiate those close together, you get the same time = same seed = same values. – Hans Kesting Jan 17 '17 at 12:11
  • 1
    First, check [this almost-duplicate question](http://stackoverflow.com/questions/273313/randomize-a-listt/1262619#1262619) for correct shuffle implementations. Second, Random returns a float that is *scaled* to the range you request. If your list contains 4 items, there's a 25% chance that the next double will be scaled to the same integer. You'd get better results if you requested a *large* integer then took its modulo against the count, eg `rng.Next(int.MaxValue)%buffer.Count` – Panagiotis Kanavos Jan 17 '17 at 12:11
  • @Henrik this is no longer true. Check the [source](https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Random.cs#L136). `Random()` uses a random seed generated by a global random generator – Panagiotis Kanavos Jan 17 '17 at 12:16
  • 1
    Random() uses a time based seed. If you call your method quickly, you will get the same result. You should use a static Random() class. – Carra Jan 17 '17 at 12:17
  • 1
    @PanagiotisKanavos No longer true where - in NET Core. Still [applies](https://referencesource.microsoft.com/#mscorlib/system/random.cs,5d22f8880fc9f8d9,references) to full .NET Framework. – Ivan Stoev Jan 17 '17 at 12:25
  • @IvanStoev oops, wrong source. Still, the chance of repeatedly picking the same number out of eg four is far greater than the probability of getting the same TickCount. For 4 items, the probability of getting the same product is 25% – Panagiotis Kanavos Jan 17 '17 at 12:27
  • Henrik's link though not very informative points to the correct answer. In my instance, a single page calls the BestQuote method in quick succession and I'm guessing that's why I get the same results back. Made one of the recommended changes in the link and now seems to be far more random. – John Ohara Jan 17 '17 at 12:35
  • Possible duplicate of [Random number generator only generating one random number](http://stackoverflow.com/questions/767999/random-number-generator-only-generating-one-random-number) – Chris Jan 17 '17 at 12:36

2 Answers2

2

Try this :

        public static IEnumerable<T> Shuffle<T>(this IEnumerable<T> @this)
        {
            if (@this.Count() <= 1) return @this;
            Random rand = new Random();
            return @this.Select(x => new { x = x, r = rand.Next() }).OrderBy(x => x.r).Select(x => x.x);
        }
jdweng
  • 33,250
  • 2
  • 15
  • 20
-1

I do random sorting like this:

().OrderBy(p => Guid.NewGuid())

So there is a unique and random Guid per item and in every call you can get totally different sorted IEnumerable.

In your case here is how i would do, without any extension methods:

public IEnumerable<PriceQuote> BestQuote(int take = 0)
{
    var q = Quotes.Where(x => x.TotalRepayable == MinPrice)
        .OrderBy(x => Guid.NewGuid())
        .ThenByDescending(x => x.ProductDetail.Product.IsSponsored);

    return take == 0 ? q : q.Take(take);
}

I am not sure in which order the orderings should be, maybe both the same but if the above code doesn't work you can try like this way:

public IEnumerable<PriceQuote> BestQuote(int take = 0)
{
    var q = Quotes.Where(x => x.TotalRepayable == MinPrice)
        .OrderBy(x => x.ProductDetail.Product.IsSponsored)
        .ThenByDescending(x => Guid.NewGuid());

    return take == 0 ? q : q.Take(take);
}

EDIT

Thanks to @Maarten, here is the final solution with using an extension:

public static IEnumerable<T> Shuffle<T>(this IEnumerable<T> @this)
{
    if (@this.Count() <= 1) return @this;
    return @this.Select(x => new { x = x, g = Guid.NewGuid() }).OrderBy(x => x.g).Select(x => x.x);
}

If you have a few items in your list, it doesn't matter if you use my first or last solution. But as @Maarteen's warning in the comments, there could be a lot more unnecessary Guids than the items' count. And it could be a problem doing multiple comparisons with many items. So i combined @jdweng's answer with mine.

er-han
  • 1,859
  • 12
  • 20
  • This will work, but be aware that a new guid will be created for every object for every comparison. So a lot more guids will be created then objects. Better is to first generate a guid for each objects, then use that to sort it. – Maarten Jan 17 '17 at 12:43
  • @Maarten you are right. A Guid property in the Quote class would be better. – er-han Jan 17 '17 at 13:32
  • Better would be to use an anonymous type with the guid and the object. See jdweng's answer: http://stackoverflow.com/a/41697115/261050 – Maarten Jan 17 '17 at 13:33
  • Thank you @Maarten. This is much better now. I was always lazy to make an extension method for random sorting and now i have one :) – er-han Jan 17 '17 at 13:50
  • `Guid.NewGuid()` is **not** guaranteed to be random. It is only guaranteed to be unique. It should not be used to generate randomness. – Enigmativity Feb 21 '17 at 23:37