.NET interview, code structure and the design
Asked Answered
C

3

7

I have been given the below .NET question in an interview. I don’t know why I got low marks. Unfortunately I did not get a feedback.

Question:

The file hockey.csv contains the results from the Hockey Premier League. The columns ‘For’ and ‘Against’ contain the total number of goals scored for and against each team in that season (so Alabama scored 79 goals against opponents, and had 36 goals scored against them).

Write a program to print the name of the team with the smallest difference in ‘for’ and ‘against’ goals.

the structure of the hockey.csv looks like this (it is a valid csv file, but I just copied the values here to get an idea)

Team - For - Against

Alabama 79 36

Washinton 67 30

Indiana 87 45

Newcastle 74 52

Florida 53 37

New York 46 47

Sunderland 29 51

Lova 41 64

Nevada 33 63

Boston 30 64

Nevada 33 63

Boston 30 64

Solution:

class Program
{
    static void Main(string[] args)
    {
        string path = @"C:\Users\<valid csv path>";

        var resultEvaluator = new ResultEvaluator(string.Format(@"{0}\{1}",path, "hockey.csv"));
        var team = resultEvaluator.GetTeamSmallestDifferenceForAgainst();

        Console.WriteLine(
            string.Format("Smallest difference in ‘For’ and ‘Against’ goals > TEAM: {0}, GOALS DIF: {1}",
            team.Name, team.Difference ));

        Console.ReadLine();
    }
}

public interface IResultEvaluator
{
    Team GetTeamSmallestDifferenceForAgainst();
}

public class ResultEvaluator : IResultEvaluator
{
    private static DataTable leagueDataTable;
    private readonly string filePath;
    private readonly ICsvExtractor csvExtractor;

    public ResultEvaluator(string filePath){
        this.filePath = filePath;
        csvExtractor = new CsvExtractor();
    }

    private DataTable LeagueDataTable{
        get
        {
            if (leagueDataTable == null)
            {
                leagueDataTable = csvExtractor.GetDataTable(filePath);
            }

            return leagueDataTable;
        }
    }

    public Team GetTeamSmallestDifferenceForAgainst() {
        var teams = GetTeams();
        var lowestTeam = teams.OrderBy(p => p.Difference).First();
        return lowestTeam;
    }

    private IEnumerable<Team> GetTeams() {
        IList<Team> list = new List<Team>();

        foreach (DataRow row in LeagueDataTable.Rows)
        {
            var name = row["Team"].ToString();
            var @for = int.Parse(row["For"].ToString());
            var against = int.Parse(row["Against"].ToString());
            var team = new Team(name, against, @for);
            list.Add(team);
        }

        return list;
    }
}

public interface ICsvExtractor
{
    DataTable GetDataTable(string csvFilePath);
}

public class CsvExtractor : ICsvExtractor
{
    public DataTable GetDataTable(string csvFilePath)
    {
        var lines = File.ReadAllLines(csvFilePath);

        string[] fields;

        fields = lines[0].Split(new[] { ',' });
        int columns = fields.GetLength(0);
        var dt = new DataTable();

        //always assume 1st row is the column name.
        for (int i = 0; i < columns; i++)
        {
            dt.Columns.Add(fields[i].ToLower(), typeof(string));
        }

        DataRow row;
        for (int i = 1; i < lines.GetLength(0); i++)
        {
            fields = lines[i].Split(new char[] { ',' });

            row = dt.NewRow();
            for (int f = 0; f < columns; f++)
                row[f] = fields[f];
            dt.Rows.Add(row);
        }

        return dt;
    }
}

public class Team
{
    public Team(string name, int against, int @for)
    {
        Name = name;
        Against = against;
        For = @for;
    }

    public string Name { get; private set; }

    public int Against { get; private set; }

    public int For { get; private set; }

    public int Difference
    {
        get { return (For - Against); }
    }
}

Output: Smallest difference in for' andagainst' goals > TEAM: Boston, GOALS DIF: -34

Can someone please review my code and see anything obviously wrong here? They were only interested in the structure/design of the code and whether the program produces the correct result (i.e lowest difference). Much appreciated.

Checkerberry answered 22/6, 2012 at 3:12 Comment(5)
How do you know that you got low marks if you didn't get any feedback? Sometimes some other person just fits better for the position, that's all.Ingrowing
Out of curiosity. How long did they give you to complete this question?Blip
@ClaudioRedi that's what make me surprised as well, they just gave me low mark and said unsuccessful at this stage. Did not say why.Checkerberry
@astroboy they gave me 2hours max. I did it in just over an hour.Checkerberry
May be they did not understand what you are doing here.Saffren
C
6

I guess you miss understood the question. The interviewer asked the minimum difference between 'for' and 'against' goals and your program is calculating the best goal average. If you see the minimum difference then it's New York not Boston. Let me update fenix2222 code here.

var teamRecords = File.ReadAllLines(Path.Combine(Application.StartupPath,"teams.csv"));
            var currentLow = int.MaxValue; //just to make sure that difference is initially less than currentLow.
            foreach (var record in teamRecords.Skip(1).ToList())
            {
                var tokens = record.Split(',');
                if (tokens.Length == 3)
                {
                    int forValue = 0;
                    int againstValue = 0;

                    if (int.TryParse(tokens[1], out forValue) && int.TryParse(tokens[2], out againstValue))
                    {
                        var difference = 0;
                        if (forValue > againstValue)
                            difference = forValue - againstValue;
                        else
                            difference = againstValue - forValue;

                        if (difference < currentLow) 
                            currentLow = difference;
                    }
                }
            }
Catcher answered 22/6, 2012 at 4:46 Comment(2)
Why dont you just use difference = Math.Abs(forValue - againstValue). But I get your point, I was writing my code based on j_lewis comments. I have updated my codeRoomful
yes off-course, thanks. i was just in hurry to put my thoughts here :)Catcher
R
11

Maybe because you wrote so many lines of code, when it can be just

var teamRecords = File.ReadAllLines("path");
var currentLow = int.MaxValue;
foreach (var record in teamRecords.Skip(1).ToList())
{
    var tokens = record.Split(',');
    if (tokens.Length == 3)
    {
        int forValue = 0;
        int againstValue = 0;

        if (int.TryParse(tokens[1], out forValue) && int.TryParse(tokens[2], out againstValue))
        {
            var difference = Math.Abs(forValue - againstValue);
            if (difference < currentLow) currentLow = difference;
        }
     }
 }

 Console.WriteLine(currentLow);
Roomful answered 22/6, 2012 at 3:24 Comment(10)
I was going to say a similar thing --- why did it have to include an interface, a DataTable, and so on? Then again, I couldn't write anything nearly as good as you did, so what do I know :)Johnsen
Agreed. @Checkerberry your csv reading was unnecessarily long. Suspect they were looking for short answer with limited OO concepts.Blip
If they expected use of inheritance and polymorphism that would have said so and probably gave somewhat different question. It is not common to create such a large infrastructure just to read some csv file. Efficiency of the original code is pretty bad.Roomful
Thanjks @Johnsen and asto boy, agree csv reading can be shortened up a little bit. CptSupermrkt, fenix2222 - I was just wanted to make my solution testable. That's why I introduced interface, so things can be tested in isolation.Checkerberry
@Checkerberry I understand, and indeed you produced quite a good code, but if you are not asked to write testable/reusable code, then do not do it, make it nice and simple. Sometimes simplicity is all you need.Roomful
@Roomful I completely agree with you. According to the question, they are interested in the class design/structure of the code. If I did not introduce interfaces, they might have said, hey your solution works, but your classes are harder to test, so it is not a good design.Checkerberry
@Checkerberry It is hard to tell, unless they give you proper feedback.Roomful
@j_lewis, for what it's worth, using DataTable would have really furrowed my brows if I were interviewing. It seems totally unnecessary and a needless dependency on a rather complex and fraught API. In particular, all that int.Parse stuff seems to strongly suggest it was the wrong data structure to choose.Dregs
@KirkWoll I think he used it to make it generic so it could adopt to any format and number of tokens. It is ugly thoughRoomful
I don't think this answer provides any constructive feedback to the OP other than a simpler way to solve the problem. Would be good if you could add reasons why what you did is better than what the OP did.Recrement
C
6

I guess you miss understood the question. The interviewer asked the minimum difference between 'for' and 'against' goals and your program is calculating the best goal average. If you see the minimum difference then it's New York not Boston. Let me update fenix2222 code here.

var teamRecords = File.ReadAllLines(Path.Combine(Application.StartupPath,"teams.csv"));
            var currentLow = int.MaxValue; //just to make sure that difference is initially less than currentLow.
            foreach (var record in teamRecords.Skip(1).ToList())
            {
                var tokens = record.Split(',');
                if (tokens.Length == 3)
                {
                    int forValue = 0;
                    int againstValue = 0;

                    if (int.TryParse(tokens[1], out forValue) && int.TryParse(tokens[2], out againstValue))
                    {
                        var difference = 0;
                        if (forValue > againstValue)
                            difference = forValue - againstValue;
                        else
                            difference = againstValue - forValue;

                        if (difference < currentLow) 
                            currentLow = difference;
                    }
                }
            }
Catcher answered 22/6, 2012 at 4:46 Comment(2)
Why dont you just use difference = Math.Abs(forValue - againstValue). But I get your point, I was writing my code based on j_lewis comments. I have updated my codeRoomful
yes off-course, thanks. i was just in hurry to put my thoughts here :)Catcher
R
3

Just a few things from a cursory overview:

  1. There are 2 interfaces, but there is no value in either of their uses.
  2. There is zero reason to introduce a DataTable to the problem statement.
  3. The code is overly-complex.
  4. The use of IList and IEnumerable in the GetTeams method look to be used 'just because'.
  5. The ResultEvaluator class is not reusable, i.e., as soon as you instantiate the class, you can never re-set the csv file. You can only keep calling the same method (GetTeamSmallestDifferenceForAgainst) over and over; no other public properties are available.
  6. In the GetDataTable method, the string[] fields is declared on one line and then the value is set on the next line.
  7. There is less than zero reasons to use the @ symbol for the 'for' paramater in the Team class' constructor; just rename the reserved word 'for' to something else.
  8. There are many constructs from 3.5+ .NET that could be used to solve the problem much easier; this just shows a lack of understanding of the language.

From the looks, it really appears you were trying to show you knew quite a bit more than what was being asked in the problem statement. But, how the knowledge you knew is being used in this exercise is quite scary, and not in a good way.

In the future, I'd recommend just solving the problem at hand and don't over-think it. Keep it simple.

Recrement answered 22/6, 2012 at 4:57 Comment(1)
1+ for your solution. I agree that I might have overly complicated. Thx :)Checkerberry

© 2022 - 2024 — McMap. All rights reserved.