1
\$\begingroup\$

The algorithm comes from wiki.

The output from the algorithm is correct but not something that a person could (easily) understand. See the wiki link above.

Each team play every other team. Optionally play every team twice and reverse home / away.

Assume an even number of teams. I need to add a test for even.

Please review for style and any other advice.

public struct Match
{
    public int IDhome { get; }
    public int IDaway { get; }
    public int Week { get; }
    public override string ToString()
    {
        return $"week {Week}  IDhome {IDhome}  IDaway {IDaway}";
    }
    public Match(int idHome, int idAway, int week)
    {
        IDhome = idHome;
        IDaway = idAway;
        Week = week;
    }
}
public struct Season
{
    public int[,] ScheculeRaw { get; }
    int TeamCount { get; }
    public List<Match> Matches { get; }
    public override string ToString()
    {
        StringBuilder sb = new StringBuilder();
        foreach (Match m in Matches)
        {
            sb.AppendLine(m.ToString());
        }
        return sb.ToString();
    }
    public Season(int teamCount, bool awayHome = false)
    {
        ScheculeRaw = RoundRobinRaw(teamCount);
        TeamCount = teamCount;
        Matches = new List<Match>();
        for (int w = 0; w < TeamCount - 1; w++)
        {
            for (int p = 0; p < TeamCount/2; p++)
            {
                Matches.Add(new Match(ScheculeRaw[w, p] + 1, ScheculeRaw[w, teamCount - 1 - p] + 1, w + 1));
            }
        }
        if (awayHome)
        {
            for (int w = 0; w < TeamCount - 1; w++)
            {
                for (int p = 0; p < TeamCount / 2; p++)
                {
                    Matches.Add(new Match(ScheculeRaw[w, teamCount - 1 - p] + 1, ScheculeRaw[w, p] + 1, w + teamCount));
                }
            }
        }
    }

    public static int[,] RoundRobinRaw(int count)
    {
        int[] rr = new int[count];
        for (int i = 0; i < count; i++)
        {
            rr[i] = i;
        }
        int[,] sch = new int[count - 1, count];
        for (int w = 0; w < count - 1; w++)
        {
            for (int i = 0; i < count; i++)
            {
                sch[w, i] = rr[i];
            }

            //Debug.WriteLine("");
            //Debug.WriteLine("ww1");
            //for (int p = 0; p < count; p += 2)
            //{
            //    Debug.Write(sch[w, p] + ", ");
            //}
            //Debug.WriteLine("");
            //Debug.WriteLine("ww2");
            //for (int p = count - 1; p > 0; p -= 2)
            //{
            //    Debug.Write(sch[w, p] + ", ");
            //}

            // rotate rr 
            int temp = rr[count - 1];
            for (int r = count - 2; r > 0; r--)
            {
                rr[r + 1] = rr[r];
            }
            rr[1] = temp;

            //Debug.WriteLine("");
            //Debug.WriteLine("rr");
            //for (int r = 0; r < count; r++)
            //{
            //    Debug.Write(rr[r] + ", ");
            //}
            //Debug.WriteLine(".");
        }
        return sch;
    }
}
\$\endgroup\$
4
\$\begingroup\$
  • Use readable variable names. I can guess at the meaning of sch (schedule?) and rr (round robin), but I'm at a loss for w (week?) and p. There's no reason to shorten the names, it does not affect the execution of the code; it does affect the readability. Everything might make sense to you today, but not to other people, or you in the future.
  • Using arrays versus lists is (at least partially) subjective), but I do think that you should be using lists here. It keeps the syntax cleaner and doesn't require readers to keep track of the array indexes.
    • The for loops are adding so much to the complexity. w < TeamCount - 1 and p < TeamCount / 2 are not easy to understand the intention. If you stick to using lists, that guides you towards using a more readable foreach statement, e.g. foreach(week in weeks)
  • bool awayHome = false is a very confusing name. A better name would be generateReverseMatch (or whatever designated name you prefer)
  • Personally, I like to keep the constructor focused on property/field assignment. I would abstract the match generation into a private method, which can still be called from the constructor: Matches = GenerateMatches(teamCount);
  • While not wrong per se, be on the lookout of overzealously using ToString() instead of a nicer PrintMatches() method. Most of the time, you want a custom layout, especially if you want to have different ways of showing the same season.

Switching the code over to using lists instead of arrays is going to have a strong impact on the posted code, so I'm not sure if further code review is going to be relevant here.

\$\endgroup\$
  • \$\begingroup\$ But it uses w as an integer. I fail to see how put integers in a list to call foreach would make it less complex. \$\endgroup\$ – paparazzo Jun 1 '18 at 8:19
  • 2
    \$\begingroup\$ @paparazzo: Most arguments I made are in relation to readability. That's a compound issue. If you solve some of the readability issues, other things may already become a lot more readable too. You're right that if you rely on an increasing int value, a for makes more sense, but the terse variable names and other cases of using arrays over lists (where you don't rely on int values except for array indexes) very much obfuscates that one case where you're right. The same argument cannot be made for p, as far as I can see. \$\endgroup\$ – Flater Jun 1 '18 at 8:22
  • \$\begingroup\$ There are no two dimensional List. There is not a List replacement for int [,]. If rr was a List would still be referring to ordinal position as that is how the algorithm works. Thanks for the feedback. I will use longer names. \$\endgroup\$ – paparazzo Jun 1 '18 at 8:29
  • 1
    \$\begingroup\$ @paparazzo: If you're going to rely on the algorithm as presented and are not willing to touch or tweak it (which is fair, I'm not saying that's wrong!), then you should really abstract that algorithm to a different class. It will showcase that your int parameters are "magic values" that require you to know the internal workings of the algorithm and how its array is structured. If you couple everything that tightly, then the array-heavy approach of the algorithm will tie you into having to use the indexes as well. I'd suggest a nice wrapper around the algorithm, to prevent that from happening. \$\endgroup\$ – Flater Jun 1 '18 at 8:36
  • \$\begingroup\$ Thanks. For me a foreach is not more complex. I like being able to look as it an know exactly what is in it. \$\endgroup\$ – paparazzo Jun 1 '18 at 8:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.