11

I need help with the programming of a game.

You open a chest and with a given probability you find an item.

Item / Chance

A / 10%
B / 30%
C / 60%

Random random = new Random();
int x = random.Next(1, 101);

if (x < 11) // Numbers 1..10 ( A -> 10% )
{ 
     do_something1(); d
} 
else if (x < 41) // Numbers 11..40 ( B -> 30 % )
{ 
     do_something2();
}
else if (x < 101) // Numbers 41..100 ( C -> 60 % ) 
{ 
     do_something3();
}

Does this example really make sense, in terms of probability? Do you have another solution?

Thank you in advance!

Paski7
  • 183
  • 2
  • 3
  • 16
  • 1
    Looks like a reasonable way to me! – EpicKip Oct 04 '17 at 11:20
  • what happens when `x == 101` in this case? – Timothy Groote Oct 04 '17 at 11:21
  • The same solution got 33 upvotes here https://stackoverflow.com/questions/1522208/weighted-random-number-generation-in-c-sharp so you should be good – Lennart Oct 04 '17 at 11:22
  • 4
    @TimothyGroote not possible the second parameter is _The exclusive upper bound of the random number returned_ https://msdn.microsoft.com/en-us/library/2dx6wyd4(v=vs.110).aspx – fubo Oct 04 '17 at 11:22
  • 1
    @fubo true, but you shouldn't forget to update every single "magic value" in this example when changing the boundaries of your distribution. if you do, that's where the bugs will sneak in. – Timothy Groote Oct 04 '17 at 11:23
  • seems like it's an accepted solution: https://stackoverflow.com/questions/1522208/weighted-random-number-generation-in-c-sharp – Alexandru Pupsa Oct 04 '17 at 11:23
  • Do you have problems? No? A working code which you want to improve? [Codereview](http://codereview.stackexchange.com). – Sinatr Oct 04 '17 at 11:35

4 Answers4

9

I realize this is a tad late, but here's an example of doing it without consts, laborious if/else and/or switch statements ;

public class WeightedChanceParam
{
    public Action Func { get; }
    public double Ratio { get; }

    public WeightedChanceParam(Action func, double ratio)
    {
        Func = func;
        Ratio = ratio;
    }
}

public class WeightedChanceExecutor
{
    public WeightedChanceParam[] Parameters { get; }
    private Random r;

    public double RatioSum
    {
        get { return Parameters.Sum(p => p.Ratio); }
    }

    public WeightedChanceExecutor(params WeightedChanceParam[] parameters)
    {
        Parameters = parameters;
        r = new Random();
    }

    public void Execute()
    {
        double numericValue = r.NextDouble() * RatioSum;

        foreach (var parameter in Parameters)
        {
            numericValue -= parameter.Ratio;

            if (!(numericValue <= 0))
                continue;

            parameter.Func();
            return;
        }

    }
}

usage example :

WeightedChanceExecutor weightedChanceExecutor = new WeightedChanceExecutor(
    new WeightedChanceParam(() =>
    {
        Console.Out.WriteLine("A");
    }, 25), //25% chance (since 25 + 25 + 50 = 100)
    new WeightedChanceParam(() =>
    {
        Console.Out.WriteLine("B");
    }, 50), //50% chance
    new WeightedChanceParam(() =>
    {
        Console.Out.WriteLine("C");
    }, 25) //25% chance
);

//25% chance of writing "A", 25% chance of writing "C", 50% chance of writing "B"        
weightedChanceExecutor.Execute(); 
Timothy Groote
  • 8,614
  • 26
  • 52
  • in your case, decent values for the ratios would be 1, 3 and 6 (or 10, 30 and 60) – Timothy Groote Oct 05 '17 at 11:38
  • I actually thought about callback methods aswell but I couldn't remember what they are called in C#/Unity xD But your solution has the clear advantage that you will never forget to implement a method for one of the chances. – Rafiwui Oct 05 '17 at 12:33
  • 1
    you will also not (so easily) accidentally reconstruct the random number generator, forget to update your comparisons, or produce unnecessarily deep nested code, forget to implement a switch case (or implement one that is unneeded). If your Action in the `WeightedChanceParam` gets too big for comfort, just implement a method and pass it as a reference, and your "chance" related code will remain readable. – Timothy Groote Oct 06 '17 at 07:54
6

I agree with @Timothy, I'd go for a more maintainable solution, where you're not relying on magic numbers to split your probabilities. Also, it's personal preference, but I'd also call it ratio rather than percent, otherwise "100" becomes another magic number, and you limit yourself to a minimum probability of 1%. This way you can split it 1:10:200 or however you please:

public static readonly int RATIO_CHANCE_A = 10;
public static readonly int RATIO_CHANCE_B = 30;
//                         ...
public static readonly int RATIO_CHANCE_N = 60;

public static readonly int RATIO_TOTAL = RATIO_CHANCE_A
                                       + RATIO_CHANCE_B
                                         // ...
                                       + RATIO_CHANCE_N;

Random random = new Random();
int x = random.Next(0, RATIO_TOTAL);

if ((x -= RATIO_CHANCE_A) < 0) // Test for A
{ 
     do_something1();
} 
else if ((x -= RATIO_CHANCE_B) < 0) // Test for B
{ 
     do_something2();
}
// ... etc
else // No need for final if statement
{ 
     do_somethingN();
}

EDIT: More generalised solution

pcdev
  • 2,852
  • 2
  • 23
  • 39
  • You should change `x < RATIO_CHANCE_B` to `x < RATIO_CHANCE_A + RATIO_CHANCE_B` because with the current solution the chances are 10/20/70 – Rafiwui Oct 04 '17 at 12:40
  • I can see that getting a little messy if you have more than 3 possible paths, if that's the case I'd probably look to decrement `x` by the previous ratio amount in each `else` block instead of adding each subsequent one in the `if()` clause – pcdev Oct 04 '17 at 13:05
  • 1
    I wouldn't even decrement it by the previous one but by the current one: `/*else*/ if ((x -= RATIO_CHANCE_CURRENT) < 0) { do_sth(); }` – Rafiwui Oct 04 '17 at 13:21
2

So to conclude the solutions here is a solution for any number of chances without a lot of if-else statements but a switch-case instead:

int[] chances = { 1, 23, 14, 49, 61 };
int totalRatio = 0;

foreach(int c in chances)
    totalRatio += c;

Random random = new Random();
int x = random.Next(0, totalRatio);

int iteration = 0; // so you know what to do next
foreach(int c in chances)
{
    iteration++;
    if((x -= c) < 0)
        break;
}

switch(iteration)
{
case 1:
case 2:
//...
default:
}
Rafiwui
  • 544
  • 6
  • 19
  • 1
    replacing if/else with a switch statement is kind of like putting lipstick on a pig ;) – Timothy Groote Oct 05 '17 at 08:51
  • In this case I would say it is ways easier to identify specific actions because you can easily replace the integers with en enum and IMO switch-case is easier to read than if-else ;) – Rafiwui Oct 05 '17 at 08:55
  • I like this one, because I need an index as a result of selection. So I can basically use this one and remove the switch statement. – Paiman Roointan May 09 '22 at 13:28
0

When I combine all your answers, then this should work here as well, right?

double number;
Random x = new Random();
number = x.NextDouble();

double RATIO_CHANCE_A = 0.10;
double RATIO_CHANCE_B = 0.30;
double RATIO_CHANCE_C = 0.60;
double RATIO_TOTAL = RATIO_CHANCE_A + RATIO_CHANCE_B + RATIO_CHANCE_C;


if ( number < RATIO_CHANCE_A ) // A -> 10%
{
do_something1();
}
else if ( number < RATIO_CHANCE_B + RATIO_CHANCE_A ) // B -> 30%
{
do_something2();
}
else if ( number < RATIO_TOTAL ) // C -> 60%
{
do_something3();
}
Paski7
  • 183
  • 2
  • 3
  • 16
  • Same problem here with `RATIO_CHANCE_B`. Make sure it is `A + B` else you only have a chance as high as the difference between `A` and `B` for `B` – Rafiwui Oct 04 '17 at 12:43
  • 2
    Two minor issues with this over my answer, first is that you should technically use `<` instead of `<=` (`number` will never be equal to 1.0, for example), second is that when maintaining those ratios you need to make sure that they always add up to exactly 1. If not, either your probabilities will not be accurate, or if `RATIO_TOTAL < 1.0` you may occasionally find that nothing happens at all due to the final `if` statement – pcdev Oct 04 '17 at 13:20