Changing method call every time loop executes C#

Question

Alright, so I want to call

for (int i = 0; i < b; b--)
{
    color += ChooseColor() + " ";
}

the important part is that it gets called several times. My ChooseColors() is

private static string ChooseColor()
{
    Random random = new Random();
    var colors = new List<string> { "Blue", "Red", "Green", "Indigo", "Black", "White", "Violet", "Turquoise", "Pink", "Lavender", "Cinder", "Fuschia", "Orange" };
    int index = random.Next(colors.Count);
    string colorName = colors[index];
    colors.RemoveAt(index);
    return colorName;
}

the issue is, I want it to call a new instance of ChooseColor everytime. For instance, it would print Black White instead of Black Black. Right now it prints the exact same thing over and over instead of dumping current and calling again (which is what I thought loops did >.<) any suggestions?

Do you want to never return the same colour twice? What happens when you run out of colours? — Rawling, Nov 23 '12 at 08:12
@Rawling: You're right, I didn't even notice that at first, but this is a great way of implementing a `for` loop :-D — Thorsten Dittmar, Nov 23 '12 at 08:15
@Angel BTW: What you really want (probably) is to **shuffle** your list and then **take** x elements from it. With LINQ it would be a one-liner. — igrimpe, Nov 23 '12 at 08:27

Thorsten Dittmar · Accepted Answer · 2012-11-23T08:14:17.917

2

The array is declared within the method, so it is redeclared and re-filled when you call the method again. You'd have to declare the list of colors statically outside the method.

Also, the randomizer is initialized again on every method call - initialize the randomizer just once outside the method, too.

private static Random random = new Random();
private static List<string> colors = new List<string> { ... };

private static string ChooseColor()
{
    if (colors.Count > 0)
    {
        int index = random.Next(colors.Count);
        string colorName = colors[index];
        colors.RemoveAt(index);
        return colorName;
    }

    return String.Empty;
}

Please note that this method returns an empty string now when called with no colors left in the list of colors. I think in addition to fixing the randomizer issue, you should really re-think the design (especially why it should be necessary to remove the colors from the list).

edited Nov 23 '12 at 08:14

answered Nov 23 '12 at 08:08

Thorsten Dittmar

55,956
8
91
139

This is not the problem, the `new Random()` every time is – emartel Nov 23 '12 at 08:09
2

It is *one* of the problems, as he tries to limit the possible number of returned colors by removing the returned color from the array - and that does not work if the list is filled again upon every call. – Thorsten Dittmar Nov 23 '12 at 08:10
The spec don't specify for colors not to come again, so no need to remove it from the array. Otherwise, his code will only work for `b <= 13`. He just didn't want the same value over and over (not random), which is caused by the reinitialization of his `Random` – emartel Nov 23 '12 at 08:15
`The spec don't specify for colors not to come again` - can you show us the spec? – Rawling Nov 23 '12 at 08:19

score 1 · Answer 2 · edited May 23 '17 at 12:18

1

In that case creat instance of Random class static.

this way

static Random random = new Random(); // Global Declaration

private static string ChooseColor()
{
    var colors = new List<string> { "Blue", "Red", "Green", "Indigo", "Black", "White", "Violet", "Turquoise", "Pink", "Lavender", "Cinder", "Fuschia", "Orange" };
    int index = random.Next(colors.Count);
    string colorName = colors[index];
    colors.RemoveAt(index);
    return colorName;
}

For More read this

Generated random numbers are always equal

edited May 23 '17 at 12:18

Community

1
1

answered Nov 23 '12 at 08:08

Nikhil Agrawal

47,018
22
121
208

I think random field should be static to be used in a static method – AlexH Nov 23 '12 at 08:11
This still doesn't prevent repeating colors, as a new list of colors is created each time the function is called. – Guffa Nov 23 '12 at 08:17

score 1 · Answer 3 · answered Nov 23 '12 at 08:14

You are creating Random instances too close in time. As they are seeded from the clock, they will all start at the same number. Create one instance outside the function, and send it aloing whey you call it.

Also, you are creating a new list each time, so removing items from the list has no effect. Create the list outside the function:

Random random = new Random();
List<string> colors = new List<string> { "Blue", "Red", "Green", "Indigo", "Black", "White", "Violet", "Turquoise", "Pink", "Lavender", "Cinder", "Fuschia", "Orange" };
for (int i = 0; i < b; b--) {
  color += ChooseColor(colors, random) + " ";
}

private static string ChooseColor(List<string> colors, Random random) {
  int index = random.Next(colors.Count);
  string colorName = colors[index];
  colors.RemoveAt(index);
  return colorName;
}

Changing method call every time loop executes C#

3 Answers3