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
– Rawling, Commented Nov 23, 2012 at 8: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
– Thorsten Dittmar, Commented Nov 23, 2012 at 8: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
– igrimpe, Commented Nov 23, 2012 at 8:27

Thorsten Dittmar · Accepted Answer · 2012-11-23 08:14:17Z

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, 2012 at 8:14

answered Nov 23, 2012 at 8:08

Thorsten Dittmar

56.9k10 gold badges95 silver badges146 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

emartel Over a year ago

This is not the problem, the new Random() every time is

Thorsten Dittmar Over a year ago

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.

emartel Over a year ago

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

Rawling Over a year ago

The spec don't specify for colors not to come again - can you show us the spec?

Community · Accepted Answer · 2017-05-23 12:18:43Z

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, 2017 at 12:18

CommunityBot

11 silver badge

answered Nov 23, 2012 at 8:08

Nikhil Agrawal

48.8k25 gold badges134 silver badges212 bronze badges

2 Comments

AlexH Over a year ago

I think random field should be static to be used in a static method

Guffa Over a year ago

This still doesn't prevent repeating colors, as a new list of colors is created each time the function is called.

Guffa · Accepted Answer · 2012-11-23 08:14:46Z

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;
}

Collectives™ on Stack Overflow

Changing method call every time loop executes C#

3 Answers 3

4 Comments

2 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

4 Comments

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related