Parsing CSV to specific format

Question

I've written this code to read CSV files written in a specific format. I would like to gather some feedback on where it could be improved. I'm trying to get into the Test-Driven-Development ideology so any way I could make the code play better with unit tests would be of particular interest to me.

An aspect I am not overly pleased with is the switch statement in the MatchHeaders function. I did try using enum representing the property names but I ran into an issue with not having constant value, and frankly it did not make the code read cleaner.

I am more interested in being pointed at what to look at and where, rather than being given code.

public class Contact
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public DateTime DateOfBirth { get; set; }
    public string FullAddress { get; set; }
    public string PostCode { get; set; }
    public string TelephoneNumber { get; set; }
    public string EmailAddress { get; set; }
}

public interface ICsvReader
{
    List<Contact> ParseCsv(string filePath);
}

public class Reader : ICsvReader
{
    public List<Contact> ParseCsv(string filePath)
    {
        List<String[]> rows = GetRows(filePath);
        string[] headers = rows[0];

        return MakeContacts(headers, rows);
    }

    private List<String[]> GetRows(string filePath)
    {
        List<Contact> users = new List<Contact>();
        string[] lines = File.ReadAllLines(filePath);
        var csv = from line in lines
                    select (line.Split(',')).ToArray();

        return csv.ToList();
    }

    private List<Contact> MakeContacts(string[] headers, List<string[]> rows)
    {
        List<Contact> contacts = new List<Contact>();
        // If we didn't know the headers we could contain values with their properties in a collection
        // List<List<Dictionary<string, object>>> csv = new List<List<Dictionary<string, object>>>();
        for (int i = 1; i < rows.Count; i++)
        {
            string[] row = rows[i];
            Contact contact = new Contact();
            for (int e = 0; e < row.Length; e++)
            {
                string header = headers[e];
                string value  = row[e];

                contact = MatchHeader(contact, header, value);
            }
            contacts.Add(contact);
        }
        return contacts;
    }

    private Contact MatchHeader(Contact user, string header, string value)
    {
        switch (header.ToLowerInvariant())
        {
            case "firstname":
                user.FirstName = value;
                break;
            case "lastname":
                user.LastName = value;
                break;
            case "dateofbirth":
                user.DateOfBirth = DateTime.Parse(value);
                break;
            case "fulladdress":
                user.FullAddress = value;
                break;
            case "postcode":
                user.PostCode = value;
                break;
            case "telephonenumber":
                user.TelephoneNumber = value;
                break;
            case "emailaddress":
                user.EmailAddress = value;
                break;
            default:
                break;
        }
        return user;
    }
}

Answer 1

Few comments:

1. Make your code more abstract, use IEnumerable instead of List

    public interface ICsvReader
    {
       IEnumerable<Contact> ParseCsv(string filePath);
    }
   

2. Provide an asynchronous version

   public interface ICsvReader
   {
       IEnumerable<Contact> ParseCsv(string filePath);
       Task<IEnumerable<Contact>> ParseCsvAsync(string filePath);
   }
   

3. Use StreamReader instead of File.ReadAllLines, it has an async version that you can use in your ParseCsvAsync method

   using(var stream = new StreamReader(filePath)){
    // read all lines
   }
   

4. Try to use the var keyword for local variables, it makes refactoring easier

   var rows = GetRows(filePath);
   

5. Use the beauty of deferred execution in C#

   private IEnumerable<Contact> MakeContacts(string[] headers, List<string[]> rows)
   {
       for (int i = 1; i < rows.Count; i++)
       {
           string[] row = rows[i];
           Contact contact = new Contact();
           for (int e = 0; e < row.Length; e++)
           {
               string header = headers[e];
               string value  = row[e];
   
               contact = MatchHeader(contact, header, value);
           }
           yield return contact;
        }
   }
   

6. You can convert ToList immediately without calling ToArray

     private IEnumerable<List<string>> GetRows(string filePath)
     {
       // why you creating this List here???
       List<Contact> users = new List<Contact>();
       lines = File.ReadAllLines(filePath);
       var csv = (from line in lines
                 select line.Split(',')).ToList();
   
       return csv;
     }
   

7. Consider @Snowbody suggestions about using a Dictionary instead of a massive switch.

Answer 2

Again just a few stylistic issues.

• Make it more LINQ-ish: Do all your list processing in one statement. Try to avoid converting between List<>s, arrays, and IEnumerable<>s unless you have to. Look for places where you're iterating over an enumerable using for or foreach, or where you're calling .Add() in a loop; these can almost always be expressed much more gracefully in LINQ.
• Code to generic interfaces; don't make the return type be List<> but instead IList<>. (Obviously when you're constructing the return value you still need to call new List<>())
• Use a Dictionary<String,...> instead of a giant switch. In this particular case the value type of the dictionary should be Action<Contact,String>.
• MatchHeader() doesn't need to return anything; its modifications to user stick.

Answer 3

I agree with a lot of the excellent points already raised. One thing I'd particularly pay attention to, though, is your use of abstractions and separation of concerns. Your overall contract is that you pass a filename and received back in turn a collection of parsed objects - this is fine to a point, but what if you got a CSV from somewhere else (a file upload, for instance)?

You could have a class that is just responsible for parsing CSVs into objects, which an interface something like this:

public interface TextParser
{
    public IEnumerable<Contact> Parse(string text);
    public IEnumerable<Contact> Parse(StreamReader textReader);
}

Your file access code is now in the calling code rather than the parser itself, which makes it a simple matter to pass in CSV text from anywhere. In addition, you can now unit test just the parsing logic without having to concern yourself with mocking the file system.

An potential further advantage of this is that you can use the strategy pattern. You might want to make a parser that separates by pipes instead of commas, or one with fixed width columns set by a schema passed into the constructor.

Next up, as the new structure suggests you will want a method to split up and return each line. If you're using strategy pattern with inheritance, this will be the main variant between the different implementations, so you can make it an abstract method in the base and call it from a shared method. I'd probably make this yield return an IDictionary of heading to cell for each line, so you have an intermediate data structure between the text and the Contact business objects.

Rather than writing that big switch statement you probably want to use reflection to find property setters that share a column name. For strings this is pretty easy, but you'll need to check the type for numbers and dates so you can apply the relevant Parse operation. This gets you away from manually mapping Contact, so that later you may modify it without necessarily having to alter the parser.

Finally, once you've made one CSV parser, it's incredibly likely that you'll want to make another. The obvious takeaway from this is that actually you want something like this:

public interface TextParser<T> where T : class
{
    public IEnumerable<T> Parse(string text);
    public IEnumerable<T> Parse(StreamReader textReader);
}

If you already have the aforementioned reflection based property setting, this will actually be child's play. You may now parse CSV files into corresponding business objects until you get bored and do something else instead. Have fun!

Answer 4

It's A Matter Of Personal Preference (AMOPP) but I would include an overload that takes a stream as the input

public interface IContactReader {
   IEnumerable<Contact> Parse(string filePath);
   IEnumerable<Contact> Parse(Stream inStream);
}

Hard-coding the input to be a filePath is limiting. Being able to read from a MemoryStream is very useful for unit testing, and who knows, we might want to embed the file as a resource, or read it from a network. By making the input a stream we can do any these. I would push for getting rid of the filePath overload but that might be a step too far. :D

Unless we have a strong need for a List, I would generally return an IEnumerable(AMOPP)

Naming
It is a Contact reader that happens (currently) to read from a CSV. The implementation can be a CsvContactReader, but the contract (interface) should be an IContactReader.

Performance
As implemented, we iterate through the file twice. Once when reading and once when processing each line. We also have to store each contact as a string and as a Contact. This is not necessary and would have performance/memory implications if working with very large files.

One can read and then process each line, rather than reading all and processing all. By using yield, in the read we get past the need to store all the input lines.

private IEnumerable<string[]> ReadLines(Stream inStream) {
    using (var reader = new StreamReader(inStream)) {
        string line;
        while ((line = reader.ReadLine()) != null) {
            yield return line.Split(',');
        }
    }
}

Fields and Properties
Is the file format (the contents and the layout) likely to change? Often?

Unless we have a lot of different CSV files to parse, coming up with a generalised solution is usually more effort than warranted. If we have only one file with a fixed format, then hard-coding the field positions to the class properties is usually a good enough solution.

As written, all we can really change in the file format, without having to change the code, is the order.

• If we add a new field then we have to change the definition of Contact and add a new entry in the switch.
• If we change a field name, we need to change the switch.
• If we remove a field, we should change the switch and change the definition of Contact

Given all this, it seems that MatchHeaders doesn't save us much (if anything) over a simple positional match

e.g.

private const int FirstNameCol = 0;
private const int LastNameCol = 1;
private const int DateOfBirthCol = 2;
//...

private Contact MakeContact(string[] line) {
    return new Contact {
       FirstName = line[FirstNameCol],
       LastName = line[LastNameCol],
       DateOfBirth = DateTime.Parse(line[DateOfBirthCol]),
       //...
    };
}

Other options
If the file format is more variable, it is possible to use Expressions to update a property by name (though it will AFAIK be case sensitive, which might be a problem) or a Dictionary<string, Action<Contact, object>> can be used to tidy things up a bit but, unless there is a strong need, I would go with the positional update first and see if it suffices. It is simpler to implement and maintain.

Answer 5

Naming vs Functionality

As the name suggest, ICsvReader should be common interface for most of CSV Reader. However, the contract shows that it is more of a contact reader.

An interface should be as generic as possible, so it could be reused elsewhere like on AppointmentReader :

public interface ICsvReader<T>
{
    IEnumerable<T> ParseCsv(string filePath);
}

Modularization

While it is good to split large block of code into smaller blocks for re-organization and ease of understanding, but don't overachieve. You have 4methods, ParseCsv, GetRows MakeContacts and MatchHeader to convert a csv to many contacts.

Performance

MatchHeader is rather a very inefficient way of affecting properties. No only it affects one property at time, but also it performs a header matching per property per row.

You could store the headers matching result somewhere and reuse that later :

var indexes = new
{
    FirstName = headers.IndexOf("firstname"),
    LastName = headers.IndexOf("lastname"),
    DateOfBirth = headers.IndexOf("dateofbirth"),
    //...
};

Final Result

public interface ICsvReader<T>
{
    IEnumerable<T> ParseCsv(string filePath);
}

public class Reader : ICsvReader<Contact>
{
    private const char Delimiter = ',';
    private const int HeaderIndex = 0, HeaderRowCount = HeaderIndex + 1;

    public IEnumerable<Contact> ParseCsv(string filePath)
    {
        var rows = File.ReadAllLines(filePath);
        var headers = rows.ElementAt(HeaderIndex).Split(Delimiter)
                          .Select(x => x.ToLowerInvariant()).ToList();

        var indexes = new
        {
            FirstName = headers.IndexOf("firstname"),
            LastName = headers.IndexOf("lastname"),
            DateOfBirth = headers.IndexOf("dateofbirth"),
            //...
        };

        foreach(var row in rows.Skip(HeaderRowCount)
                               .Select(x => x.Split('|').Select(y => y.Trim()).ToList()))
            yield return new Contact
            {
                FirstName = row[indexes.FirstName],
                LastName = row[indexes.LastName],
                DateOfBirth = DateTime.Parse(row[indexes.DateOfBirth]),
                //...
            };
    }
}