FOR cycle works badly

Question

here is my problem: I'm working on a player grouper (it divides players into groups). I do it by a for cycle, but it doesn't divide all players :(.

Here is the code:

namespace Grouper
{
public partial class Form1 : Form
{
    List<string> players=new List<string>(); 

    public Form1()
    {
        InitializeComponent();
        LoadPlayers();
    }

    private void But_rnd_Click(object sender, EventArgs e)
    {
        LoadPlayers();
        bool isOdd = players.Count % 2 == 1;
        List<string> results=new List<string>();
        if(!isOdd) // Count of players is even
        {
            Grouping(ref results);
        }
        if(isOdd) // Count of players is odd
        {
            Grouping(ref results);
            results.Add("Remained: " + players[0]);
            ShowResults(ref results);
        }
    }

    private void Grouping(ref List<string> results)
    {
        Random r=new Random();
        for (int i = 0; i < players.Count() / 2 + 1; i++)
        {
            int randomPlr = r.Next(players.Count() / 2 + 1, players.Count());
            results.Add(i + 1 + ".: " + players[i] + " + " + players[randomPlr]);
            players.RemoveAt(i);
            players.RemoveAt(randomPlr - 1);
        }
    }

    private void ShowResults(ref List<string> results)
    {
        string write = "";
        foreach (string result in results)
        {
            write += result + "\n";
        }
        MessageBox.Show(write);
    }

    private void LoadPlayers()
    {
        players.Clear();
        players.Add("p1");
        players.Add("p2");
        players.Add("p3");
        players.Add("p4");
        players.Add("p5");
        players.Add("p6");
        players.Add("p7");
    }
}
}

The method ShowResults() shows only 2 groups and 1 player, who is remaining (2 groups and 1 remaining = 5 players, but I have there 7 players!).

Unlike forum sites, we don't use "Thanks", or "Any help appreciated", or signatures on [so]. See "[Should 'Hi', 'thanks,' taglines, and salutations be removed from posts?](http://meta.stackexchange.com/questions/2950/should-hi-thanks-taglines-and-salutations-be-removed-from-posts). — John Saunders, May 10 '13 at 15:10
your `for` loop is going from `i=0` to `i=4`, which, by my count is 5. so you're adding 5 players into `results` — ethorn10, May 10 '13 at 15:14
As you remove players from the list, the position of the other players in the list changes. Try iterating your for loop backwards. — mbeckish, May 10 '13 at 15:15

score 4 · Accepted Answer · edited May 23 '17 at 10:25

There are several issues in your code.

`if` statement use

This is really odd:

bool isOdd = players.Count % 2 == 1;
…
if(!isOdd)
{
    …
}
if(isOdd) // Count of players is odd
{
    …
}

Use if / else, your code will be more readable:

if (players.Count % 2 == 0)
{
    …
}
else
{
    …
}

Further, you can omit this check at all, do the grouping, and then figure out what to do with the rest (Who says at most one will remain each time? What if you will need to divide them among three groups in the future?):

Grouping(…);
if (players.Count > 0)
{
    … process remaining players …
}

Number of iterations in the `for` loop

As others have pointed out, you are 'overiterating' here:

for (int i = 0; i < players.Count() / 2 + 1; i++)

This will always make two more iterations than you want. You should change it like this:

for (int i = 1; i < players.Count() / 2; i++)

That way it will work for even or odd count, and for corner cases too (0 or 1 player only). Or, providing you take two players out of the list each iteration, just use:

while (Players.count() > 1)

Your combination of indexed and random access is wrong

In the following code you can't be sure a) the i-th element exist, b) the random index won't match i, which would effectively place the same player into both groups:

int randomPlr = r.Next(players.Count() / 2 + 1, players.Count());
results.Add(i + 1 + ".: " + players[i] + " + " + players[randomPlr]);
players.RemoveAt(i);
players.RemoveAt(randomPlr - 1);

Either take one half, then the other half as they are in the players list (in which case you don't need to do any fors at all), or randomly pick one player each time. For example, like this:

public string PickRandomPlayer(List<string> players)
{
    int random = … generate random index …;
    string player = players[random];
    players.RemoveAt(random);
    return player;
}

Then call this method twice in the for look to pick a player for each half.

Invalid use of `ref` parameter

The following declaration contains an unnecessary and unwanted ref:

private void Grouping(ref List<string> results)

To put it simply: Object, like a List<string> are passed by reference by default. That means, when you access the results parameter in your code and modify it (add / remove an item), the affected instance is the one the caller provided:

void Grouping(List<string> results) { … }

…

List<string> results = new … ;
…
Grouping(results);
…
… here results contains what Grouping put in

On the other hand, when you specify ref, you can pass out of the method a new instance of, in your case, List<string>:

void Grouping(List<string> results)
{
    results = new … ; // this instance will be returned out of the method!
}

…

List<string> results = new … ;
…
Grouping(ref results); // here, whatever is in results currently, is lost
…

Use of `goto`

This is just awful. See this SO question and its accepted answer. Also, Google for 'GOTO Statement Considered Harmful` for further reading on this topic.

@Dennis15 That's why you are here, asking other developers, aren't you? Also check out [codereview.stackexchange.com](http://codereview.stackexchange.com) for situations when you just need to know opinions about your code. — Ondrej Tucny, May 10 '13 at 15:35

score 2 · Answer 2 · answered May 10 '13 at 15:21

Your condition, for the loop, is i < players.Count() / 2 + 1. This said, since you remove 2 players in each iteration of the loop, you change the value of your condition.

Let's say you start with 7 players.

Iteration 1 : i = 0, condition = 4, 2 players processed total, 5 players remaining
Iteration 2 : i = 1, condition = 3, 4 players processed total, 3 players remaining
Iteration 3 : i = 2, condition = 2, Condition is not satisfied

At the end, you have 2 groups and 3 remaining players, but since you do "Remained: " + players[0], you only show 1 of the remaining players.

score 0 · Answer 3 · answered May 10 '13 at 15:25

This isn't pretty, but should work (just typing, so adjust the syntax):

   private void But_rnd_Click(object sender, EventArgs e)
    {
        LoadPlayers();
        List<string> results=new List<string>();
        Grouping(ref results);
        ShowResults(ref results);
    }

    private void Grouping(ref List<string> results)
    {
        Random r=new Random();
        while (Players.count() > 1)
        {
           int p1 = r.next(players.count());
           String p1s = players.get(p1);
           players.removeat(p1);
           int p2 = r.next(players.count());
           String p2s = players.get(p2);
           players.removeat(p2);

           results.add(//whatever string you use to combine p1 + p2);
        }
        if (players.count() > 0)
        {
            // add remaining players
        }
    }

score 0 · Answer 4 · answered May 10 '13 at 15:33

If I am understanding correctly, it looks like you are attempting to generate pairs of players. I don't really understand your method of choosing a random element or the exit condition on your loop.

One problem you are facing is that as you run through your loop in the Grouping function, you are removing elements, and thus changing the value of players.count(), and the positions of the elements.

ie. if you have 5 players in the array, say they are "A", "B", "C", "D", "E". On the first iteration of the loop, i=0, count=5. The element at index 0 is "A", and say we randomly select "D" as the pair. We remove those two, and the list becomes "B","C","E". On the next iteration, i = 1, and the element at index 1 is "C" (not "B").

SUGGESTION: Try a simpler algorithm, such as the following (in pseudocode):

 while (list.count() > 1)
 {

        randomElementIndex = random(1, list.count())
        firstElem = list[0]; // the first one will always be different, since we remove the old first one
        secondElem = list[randomElementIndex]

        print output

        list.remove(firstElem)
        list.remove(secondElem)
 }

score 0 · Answer 5 · answered May 10 '13 at 15:36

Thanks really to all! I corrected my code error. My Grouping() method:

    private void Grouping(ref List<string> results)
    {
        Random r=new Random();
        int index = 0;
        for (int i = players.Count /2; i > 0;i--)
        {
            index++;
            back:
            int rand1 = r.Next(players.Count);
            int rand2 = r.Next(players.Count);
            if (rand1 == rand2)
            {
                // Random numbers are same :(
                goto back; // I know - it'll be slowly :(
            }
            results.Add(index + ".: " + players[rand1] + " - " + players[rand2]);
            players.Remove(players[rand1]);
            players.Remove(players[rand2]);
        }
    }

There's no need to pass `results` as `ref`. Objects are passed by reference by default. — Ondrej Tucny, May 10 '13 at 15:39

score 0 · Answer 6 · answered May 10 '13 at 15:52

I'm not sure of your intent, but it looks like you're trying to take a list of players and to create two teams with the same number of players randomly chosen.

Others have replied as to why your code did not work. I just wanted to suggest a different way to do what I think you're trying to do. Given your list of players:

//create a new list of players mixing your original list in random order 
var shuffled = players.OrderBy(item => rnd.Next()).ToList();

int n = shuffled.Count() / 2; // there will be n elements per team
bool isOdd=(shuffled.Count() % 2 == 1); // is the number of players odd?

// the first team will have the first n players in the "shuffled" list
var firstTeam = shuffled.Take(n).ToList();
// the second team will have the next n players in the "shuffled" list
var secondTeam= shuffled.Skip(n).Take(n).ToList();

// if the players were in odd number take the last one
// who wasn't chosen in any team
// (that was me in school, that's why I'm a developer now!! :) )    
string didNotPlay=(isOdd)? shuffled.Last() : null;

FOR cycle works badly

6 Answers6

if statement use

Number of iterations in the for loop

Your combination of indexed and random access is wrong

Invalid use of ref parameter

Use of goto

`if` statement use

Number of iterations in the `for` loop

Invalid use of `ref` parameter

Use of `goto`