How do I generate numbers that vary from a range depending on the user input

Refresh

November 2018

Views

60 time

1

I am trying to generate random numbers with varying ranges depending on the user input. In my program the user will enter the iLvl as a number between 1-100, depending on that number i will generate a number that fits the tier range and assign it to preRoll which is then printed out on the forum. The issue that I am having is that no matter which number I enter I keep getting the same numbers (that vary from 5 to 8 only). I am not sure as to why this happens but i figure it has to do something with the system clock which is generating the number. I apologize if the question is too vague or really basic but I have some trouble understanding how random number generator works in c#. Thank you for all feedback and please let me know if there are any other questions

        //Function to get random number
    private static readonly Random getrandom = new Random();

    public static int GenRan(int min, int max)
    {
        lock (getrandom) // synchronize
        {
            return getrandom.Next(min, max);
        }
    }

    private void selButt_Click(object sender, EventArgs e)
    {
        int iLvl = 0;
        int preRoll = 0;
        iLvl = int.Parse(itemLvl.Text);
        if (ringType.SelectedIndex == 0)
        {
            ringpic.Load("E:\\ProgProj\\POERingMaker  Alpha 1.0\\POERingMaker  Alpha 1.0\\Resources\\Steel_Ring.png");
            Random obj = new Random();
            if ( iLvl >= 1 || iLvl <= 4)       // teir 8
            {
                preRoll = obj.Next(5, 9);
            }
            else if (iLvl >= 5 || iLvl <= 10)  // teir 7
            {
                preRoll = obj.Next(10, 19);
            }
            else if (iLvl >= 11 || iLvl <= 17)  // teir 6
            {
                preRoll = obj.Next(20, 29);
            }
            else if (iLvl >= 18 || iLvl <= 23)  // teir 5
            {
                preRoll = obj.Next(30, 39);
            }
            else if (iLvl >= 24 || iLvl <= 29)  // teir 4
            {
                preRoll = obj.Next(40, 49);
            }
            else if (iLvl >= 30 || iLvl <= 35)  // teir 3
            {
                preRoll = obj.Next(50, 59);
            }
            else if (iLvl >= 36 || iLvl <= 43)  // teir 2
            {
                preRoll = obj.Next(60, 69);
            }
            else if (iLvl >= 44 || iLvl <= 100)  // teir 1
            {
                preRoll = obj.Next(70, 79);
            }
            else
            {
                MessageBox.Show("Item Level incorrect! ");
            }
            prefixBox.Text = preRoll.ToString();
        }
        else if (ringType.SelectedIndex == 1)
        {
            ringpic.Load("E:\\ProgProj\\POERingMaker  Alpha 1.0\\POERingMaker  Alpha 1.0\\Resources\\Opal_Ring.png");
        }
        else if (ringType.SelectedIndex == 2)
        {
            ringpic.Load("E:\\ProgProj\\POERingMaker  Alpha 1.0\\POERingMaker  Alpha 1.0\\Resources\\Prismatic_Ring.png");
        }
    }

3 answers

3

You're incorrectly using a logical OR in your if statements.

if(someNumber >= 1 || someNumber <= 4) // do things

All numbers are greater than or equal to 1 OR less than or equal to 4, so this condition is the one that always matches. Since you're establishing bounds, you want to use a logical AND.

if(someNumber >= 1 && someNumber <= 4) // do things

This only matches numbers 1..4 inclusive. 3 is greater than or equal to 1 AND less than or equal to 4, so it matches. 5 is greater than or equal to 1, but also greater than 4, so it doesn't pass your condition.

1

The system clock isn't really "used to generate the random number" - it is used to provide a one-time seed value for the random generation algorithm to base it's subsequent sequence of numbers on. Using a time-based seed value is only a problem when you create multiple Random instances very close in time, and then expect each one to use a different seed. Something like this will show the problem:

  Random rand1 = new Random();
  Random rand2 = new Random();
  Thread.Sleep(2000);
  Random rand3 = new Random();

It's almost certain that rand1 and rand2 will get the same sequence of random numbers as the other, whereas rand3 won't. But that's not the problem - it is avoided simply by creating a single Random instance, and using it for all the random numbers, rather than multiple instances (if you can't assure they won't have the same time-based seed value).

In the code, you create the getrandom instance, and a function to generate the ranged random number, at the beginning, with this:

private static readonly Random getrandom = new Random();

public static int GenRan(int min, int max)
{
    lock (getrandom) // synchronize
    {
        return getrandom.Next(min, max);
    }
}

But you never use GenRan. Instead, you create a new random object within the Click event:

Random obj = new Random();

While this looks like some kind of error, it isn't going to cause the problem you describe, because you do then just use that object to generate each random number. So internally, at least, it seems this would likely work.

Since the code in question is within a _Click method, I presume it's called for the event. You generate at most one random number per method call, and on each call, you create a new Random instance. It seems unlikely a UI event handler would be called close enough, back to back, to have this problem. Besides, the problem isn't that you get the same sequence of random numbers (which is what would happen with multiple Random instances created too close in time), but that it doesn't appear to be respecting the Next method's parameters that indicate the range for the random number. So, that has nothing whatsoever to do with the time-based seed value at all.

So all of that is what is NOT really wrong here. Instead, it's wrong because of the conditional statements being wrongly written. Here's a few from your code:

        if ( iLvl >= 1 || iLvl <= 4)       // teir 8
        {
            preRoll = obj.Next(5, 9);
        }
        else if (iLvl >= 5 || iLvl <= 10)  // teir 7
        {
            preRoll = obj.Next(10, 19);
        }
        else if (iLvl >= 11 || iLvl <= 17)  // teir 6
        {
            preRoll = obj.Next(20, 29);
        }

This is how the block of conditionals starts. Let's walk through with a value of 3.

First we hit this:

        if ( iLvl >= 1 || iLvl <= 4)       // teir 8

And the very first condition is satisfied - iLvl >= 1. And the rest of the condition is "OR iLvl <= 4". So, either condition being met will execute the following statement:

        {
            preRoll = obj.Next(5, 9);
        }

Note that this describes exactly the range of the results you are getting.

If you haven't seen the problem yet, let's move on, to a value of 6.

        if ( iLvl >= 1 || iLvl <= 4)       // teir 8
        {
            preRoll = obj.Next(5, 9);
        }
        else if (iLvl >= 5 || iLvl <= 10)  // teir 7
        {
            preRoll = obj.Next(10, 19);
        }

If you expect the first block to be false, and the second to be true, there's the error. The first block is satisfied by ALL values that are either greater than zero, or any value < 5. And of course, our new value of "6" is greater than zero, and so it takes that first condition. In fact, ALL values > 0 will use that condition, and that's your problem.

To fix, you should use "&&" rather than "||", to assure that BOTH conditions are met for the block to execute.

0

You are checking if iLvl >= 1 OR iLvl <= 4, so unless iLvl is less than one, this will always be true. Use && (and) instead of || (or) to ensure both conditions are true.

Alternatively, you could simplify your statements like so:

if (iLvl < 1 || iLvl > 100)
{
    MessageBox.Show("Item Level incorrect! ");
}
else if (iLvl <= 4)   // teir 8
{
    preRoll = obj.Next(5, 9);
}
else if (iLvl <= 10)  // teir 7
{
    preRoll = obj.Next(10, 19);
}
else if (iLvl <= 17)  // teir 6
{
    preRoll = obj.Next(20, 29);
}
else if (iLvl <= 23)  // teir 5
{
    preRoll = obj.Next(30, 39);
}
else if (iLvl <= 29)  // teir 4
{
    preRoll = obj.Next(40, 49);
}
else if (iLvl <= 35)  // teir 3
{
    preRoll = obj.Next(50, 59);
}
else if (iLvl <= 43)  // teir 2
{
    preRoll = obj.Next(60, 69);
}
else  // teir 1
{
    preRoll = obj.Next(70, 79);
}