CODEWITHSUNDEEP
c#
jskidd3
jskidd3

Reputation: 4783

Labels text values not setting

I have 8 labels. I want each label to have its text property set to a random number. For some reason only the first label is having a number set, why is this? (Also, although not directly related, if there's a better way of doing label1.Text, label2.Text, label3.Text etc, please let me know!)

Thanks

public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
        go();
    }

    void go()
    {
        int[] numbers = new int[8];

        foreach (int number in numbers) 
        {
            numbers[number] = getRandomNumber();
        }

        label1.Text = numbers[0].ToString();
        label2.Text = numbers[1].ToString();
        label3.Text = numbers[2].ToString();
        label4.Text = numbers[3].ToString();
        label5.Text = numbers[4].ToString();
        label6.Text = numbers[5].ToString();
        label7.Text = numbers[6].ToString();
        label8.Text = numbers[7].ToString();
    }

    int getRandomNumber()
    {
        Random random = new Random();
        return random.Next(10, 1000);
    }
}

Upvotes: 3

Views: 696

Answers (4)

Tim Schmelter
Tim Schmelter

Reputation: 460138

Edit: More problems

only the first label is having a number set

The reason for this is that you have declared and initalized an int[] in this way:

int[] numbers = new int[8];

After this you have an array with Length 8 but all ints are default(int) what is 0.

Therefore the following foreach loop ...

foreach (int number in numbers) 
{
    numbers[number] = getRandomNumber();
}

... will only initialize the first int to a random number(well, not really random, more about this later). You could use a for-loop instead:

for (int iii=0; iii<numbers.Length;iii++)
{ 
    numbers[iii] = getRandomNumber();
}

But if you use my improved code below that will also solve this issue.

Use the same random instance in the loop. Otherwise it will create the same number since it's seeded with the current time.

MSDN:

The random number generation starts from a seed value. If the same seed is used repeatedly, the same series of numbers is generated. One way to produce different sequences is to make the seed value time-dependent, thereby producing a different series with each new instance of Random. By default, the parameterless constructor of the Random class uses the system clock to generate its seed value, while its parameterized constructor can take an Int32 value based on the number of ticks in the current time.

Random rnd = new Random();
foreach (int number in numbers) 
{
    numbers[number] = rnd.Next(10, 1000);
}

According to your second question how to improve the code: you could use an array with your labels:

Random random = new Random();
var labels = new[] { label1, label2, label3, label4, label5, label6, label7, label8 };
for (int i = 0; i < labels.Length; i++)
{
    labels[i].Text = random.Next(10, 1000).ToString();
}

Upvotes: 6

Derek W
Derek W

Reputation: 10026

As ShadowCat7 pointed out, the core problem here is not that Random is being misused, but that the foreach loop is being misused.

The following the line of code. 

int[] numbers = new int[8];

Declares an array of 8 int's all at the value of 0.

Hence, when you execute the following foreach loop.

foreach (int number in numbers) 
{
    numbers[number] = getRandomNumber();
}

You are iterating through each int value contained in the array of integers called numbers which as previously stated are all 0! Thus, when you use it as the argument in the array indexing operator [] you are repeatedly making this call.

numbers[0] = getRandomNumber();

Which is the real reason why you are not seeing the rest of your array populated with a pseudo-random number between 10 and 1000.

While it is true that continually creating a new instance of Random will generate the same number for you over and over again by reusing the same seed (defeating the purpose of using Random in a loop like this), the chief issue here of why 7 of your 8 labels are 0 is because of the misuse of the foreach loop.

As for a clever way of setting the Text value for each Label, see Tim Schmelter's answer as it is brilliant.

Upvotes: 0

ShadowCat7
ShadowCat7

Reputation: 824

The problem is in your foreach loop. You are using number as the index, when it is really the value at the index in the array. The effect is that only the first label.Text will have a value! Also, since you are creating a new Random every time, Random.Next will return the same value because random seeds are based on time. Try this instead:

public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
        go();
    }

    void go()
    {
        int[] numbers = new int[8];

        Random random = new Random();

        for (int i = 0; i < 8; i++)
        {
            numbers[i] = random.Next(10, 1000);
        }

        label1.Text = numbers[0].ToString();
        label2.Text = numbers[1].ToString();
        label3.Text = numbers[2].ToString();
        label4.Text = numbers[3].ToString();
        label5.Text = numbers[4].ToString();
        label6.Text = numbers[5].ToString();
        label7.Text = numbers[6].ToString();
        label8.Text = numbers[7].ToString();
    }
}

Upvotes: 5

McAden
McAden

Reputation: 13972

It's because of 

int getRandomNumber()
{
    Random random = new Random(); // <-- This line
    return random.Next(10, 1000);
}

Do this instead:

private static readonly Random random = new Random(); // <-- only set once

int getRandomNumber()
{
    return random.Next(10, 1000);
}

What's happening in the first one is that you're seeding it with the same value over and over again since time hasn't technically ticked if the loop is too tight. In the second one what I'm doing is seeding the generator once - thus getting a different number every time.

Upvotes: 1

Related Questions