1
\$\begingroup\$

I am reading in a file which consists of scores between two teams. The team that wins gets 2 points, if the teams tie both teams get one point. The result is ranked by points and if there is a tie ranked by alphabetic order. Then this is written to an output file. Any suggestions to improve the code?

Input file: Blues 3, Coyotes 0

Output Blues 2 pts Coyotes 0 pts

using System;
using System.Collections.Generic;
using System.Linq;

namespace skateboard
{
    class MainClass
    {
        //If team does not exists add to library and award 0 points
        static void DoesTeamExist(Dictionary<string, int> dictionary, string team)
        {
            if (!dictionary.ContainsKey(team))
            {
                dictionary.Add(team, 0);
            }
        }

        //If draw add one point to each team, else awards the winner 2 points
        static void AwardPoints(Dictionary<string, int> dictionary, string teamOne, string teamTwo, int teamOneScore, int teamTwoScore)
        {
            if (teamOneScore == teamTwoScore)
            {
                dictionary[teamOne] += 1;
                dictionary[teamTwo] += 1;
            }
            else if (teamOneScore > teamTwoScore)
            {
                dictionary[teamOne] += 2;
            }
            else 
            {
                dictionary[teamTwo] += 2;
            }
        }


        public static void Main(string[] args)
        {

            int counter = 0;
            string line;
            System.IO.StreamReader file =
                new System.IO.StreamReader(@"/Users/aaronmk2/Projects/skateboard/skateboard/sample-input.txt");
            Dictionary<string, int> dictionary = new Dictionary<string, int>();


            while ((line = file.ReadLine()) != null)
            {
                string []lines = line.Split(',');
                string teamOne = lines[0].Substring(0, lines[0].Length - 2);
                int teamOneScore = Convert.ToInt32(lines[0][lines[0].Length - 1] - '0');
                DoesTeamExist(dictionary, teamOne);
                string teamTwo = lines[1].Substring(1, lines[1].Length - 2);
                int teamTwoScore = Convert.ToInt32(lines[1][lines[1].Length - 1] - '0');
                DoesTeamExist(dictionary, teamTwo);
                AwardPoints(dictionary, teamOne, teamTwo, teamOneScore, teamTwoScore);
                counter++;
            }

            file.Close();

            var items = from pair in dictionary
                orderby pair.Value descending, pair.Key ascending
                select pair;


            System.IO.StreamWriter write = new System.IO.StreamWriter(@"/Users/aaronmk2/Projects/skateboard/skateboard/test-output.txt");

            // Display results.
            foreach (KeyValuePair<string, int> pair in items)
            {
                Console.WriteLine("{0}: {1}", pair.Key, pair.Value);
                if (pair.Value == 1)
                {
                    write.WriteLine("{0}: {1} pt", pair.Key, pair.Value);
                }
                else
                {
                    write.WriteLine("{0}: {1} pts", pair.Key, pair.Value);
                }

            }
            write.Close();
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ A team never scores double digits? I don't get the - '0' \$\endgroup\$
    – paparazzo
    Sep 30, 2016 at 15:29

2 Answers 2

1
\$\begingroup\$

In this console application it might not be that important but later you should always take care of any resources and dispose them propertly. Closing it isn't enough so you should wrap the streams in usings:

using (var file = new StreamReader(@"/Users/aaronmk2/Projects/skateboard/skateboard/sample-input.txt"))
{
    ...
}

Be consistent. Either use var everywhere or nowhere. You also don't always need the entire namespace. Add a using System.IO; to other usings to avoid lengthy lines.


string teamOne = lines[0].Substring(0, lines[0].Length - 2);
int teamOneScore = Convert.ToInt32(lines[0][lines[0].Length - 1] - '0');
DoesTeamExist(dictionary, teamOne);
string teamTwo = lines[1].Substring(1, lines[1].Length - 2);
int teamTwoScore = Convert.ToInt32(lines[1][lines[1].Length - 1] - '0');

You should define a class for the results. It would help to explain all the magic indexes [0] above and encapsulate some logic. It's extremely hard to follow in its current form.

lines[0].Substring(0, lines[0].Length - 2)

Assuming that the score is always a space and a number like 1 is a very bad idea. Use regex instead. The TeamScore class could look like this:

class TeamScore
{
    public string Name { get; set; }
    public int Points { get; set; }

    public static TeamScore Parse(string value)
    {
        var scoreMatch = Regex.Match(value, @"(?<name>.+?)(?<points>\d+)");
        return new TeamScore
        {
            Name = scoreMatch.Groups["name"].Value.Trim(),
            Points = Convert.ToInt32(scoreMatch.Groups["points"].Value)
        };
    }
}

This has now huge implications as it will affect the entire application and make it much easier to understand:

The new Main becomes:

public static void Main(string[] args)
{
    var counter = 0;
    var teamScores = new Dictionary<string, TeamScore>();

    // create constants for indicies
    const int teamOneIndex = 0;
    const int teamTwoIndex = 1;

    using (var file = new StreamReader(@"/Users/aaronmk2/Projects/skateboard/skateboard/sample-input.txt"))
    {
        // define the variable as close to the usage as possible
        string line;
        while ((line = file.ReadLine()) != null)
        {
            // name it appropriately, it's "teams" not just a "line"
            var teams = line.Split(',').Select(TeamScore.Parse).ToList();

            // add both teams to the dictionary if not already there
            foreach (var team in teams)
            {
                if (!teamScores.ContainsKey(team.Name))
                {
                    teamScores.Add(team.Name, team);
                }
            }

            AwardPoints(teamScores, teams[teamOneIndex], teams[teamTwoIndex]);
            counter++;
        }
    }

    // don't name it just items, write what items they are
    var sortedTeamScores = 
        from teamScore in teamScores
        orderby teamScore.Value.Points descending, teamScore.Key ascending
        select teamScore;


    using (var write = new System.IO.StreamWriter(@"/Users/aaronmk2/Projects/skateboard/skateboard/test-output.txt"))
    {
        // Display results.
        foreach (var teamScore in sortedTeamScores)
        {
            Console.WriteLine("{0}: {1}", teamScore.Key, teamScore.Value);
            if (teamScore.Value.Points == 1)
            {
                write.WriteLine("{0}: {1} pt", teamScore.Key, teamScore.Value.Points);
            }
            else
            {
                write.WriteLine("{0}: {1} pts", teamScore.Key, teamScore.Value.Points);
            }

        }
    }
}

and the helper method is:

static void AwardPoints(Dictionary<string, TeamScore> dictionary, TeamScore teamOne, TeamScore teamTwo)
{
    if (teamOne.Points == teamTwo.Points)
    {
        dictionary[teamOne.Name].Points += 1;
        dictionary[teamTwo.Name].Points += 1;
    }
    else if (teamOne.Points > teamTwo.Points)
    {
        dictionary[teamOne.Name].Points += 2;
    }
    else
    {
        dictionary[teamTwo.Name].Points += 2;
    }
}

There still other improvements possible like: - separate the saving from the console output into two methods - encapsulate the file reading in another method

The final version should look like this:

void Main(string[] args)
{
    var fileName = @"...";    
    var teamScores = ImportTeamScores(fileName);
    var sortedTeamScores = SortTeamScores(teamScores);
    SaveTeamScore(sortedTeamScores);
    PrintTeamScore(sortedTeamScores);
}

It will make the maintenace much easier as you can now adjust only one feature at a time witout affecting the others.

I hope I have the logic correct after all the refactoring.

\$\endgroup\$
1
\$\begingroup\$

I suppose there are many ways to accomplish what you're doing and many ways in which it can be improved further. As it's a very simple program it's fine to keep it basic and not care too much about elaborate design. At the same time it can certainly be structured a lot better, and there is at least one definite no-no you should be aware of.

  • Your method DoesTeamExist sounds like it should return a boolean telling you whether the team exists, and have no side effects. So it looks very strange when you simply call it like DoesTeamExist(dictionary, teamOne);. My first thought is, "why would he ask whether the team exists but not do anything with the result?" Lo and behold, the method doesn't tell you whether the team exists, and instead actually produces a side effect. This is very misleading and should be avoided. See Command/Query Separation. It should either return a boolean stating whether the team exists, or renamed to something that describes the side effect it performs.
  • It's good practice to open streams in using blocks (shorthand for try/finally, where the finally block disposes the object).
  • Your variable naming isn't bad, but could use improvement. I like teamOneScore for example. But what's teamOne? Team One What? Name? Call it teamOneName then. And of course dictionary, given its use throughout (and as a parameter name to boot), should be named for what it holds, rather than its type.
  • There is no validation of the lines you're reading; a missing comma or unexpected lengths would result in an exception being thrown. And what happens if the score number is two digits?
  • Generally you put the methods that are called under the calling methods, not before.
  • Finally I would like to separate it into three distinct steps.
    1. Reading and parsing file
    2. Updating scores.
    3. Writing results to output file.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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