3
\$\begingroup\$

I have a csv parsing method in my Windows App that reads headers and information from the subsequent lines. I am not sure if I am doing in an efficient way. Please comment.

public List<(string IPN, string FPTech)> GetIPNFPTechFromCSV(string filePath)
    {
        List<(string, string)> IPNFPTechPairs = new List<(string, string)>();
        using (StreamReader csvfileReader = new StreamReader(filePath))
        {
            List<string> headings = new List<string>();
            if (HasHeaderRow)
            {
                headings = LoadFieldNamesFromHeaderRow(csvfileReader.ReadLine());
                if (headings.Count != 2)
                    return null;
                else
                {
                    int IPNPos = headings.IndexOf("PartNumber");
                    int FPPos = headings.IndexOf("FPTech");
                    if (IPNPos != -1 && FPPos != -1)
                    {
                        string line;
                        while ((line = csvfileReader.ReadLine()) != null)
                        {
                            var IPNFPTechPair = (IPN: line.Split(Delimiter)[IPNPos], FPTech: line.Split(Delimiter)[FPPos]);
                            IPNFPTechPairs.Add(IPNFPTechPair);
                        }
                    }
                    else
                        return null;
                }
            }
            else
                return null;
        }
        return IPNFPTechPairs;
    }

In the caller I am doing

var InputPairs = inputcsvfile.GetIPNFPTechFromCSV(txt_filePath.Text);

            if (InputPairs == null)
            {
                MessageBox.Show("Please make sure input file is in valid format with PartNumber,FPTech Headers", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return;
            }

To me, it looks like I am returning nulls from too many paths in my csv parsing method. Please let me know how can I improve this code. I am also not sure how do we efficiently log or report eceptions though out the Windows App. Any ideas are welcome please.

Sample Input CSV File

FPTech,PartNumber
TK0,H19873-001
TK1,H19873-001
TK2,H19872-001
TK1,H19874-001
TK0,H19872-001

The input .csv should be in this format and the FPTech and PartNumber Headers can interchange and sometime smissing. When missing, any of these I want to report to user.

\$\endgroup\$
3
  • \$\begingroup\$ In order to fully examine the effectiveness of your code, a sample of the data that includes any variations that you expect, would be helpful. \$\endgroup\$
    – user33306
    Mar 7, 2019 at 18:50
  • \$\begingroup\$ updated it with sample input with 6 lines \$\endgroup\$ Mar 7, 2019 at 18:57
  • \$\begingroup\$ Does the return type have to be a list? \$\endgroup\$
    – user33306
    Mar 7, 2019 at 20:35

2 Answers 2

2
\$\begingroup\$
        if (HasHeaderRow)
        {

It seems strange, that you know in advance, if the file has headers or not? If that is right, why then call the method in the first place if HasHeaderRow is false - when you do nothing in that case?


public List<(string IPN, string FPTech)> GetIPNFPTechFromCSV(string filePath)

  1. I would return IEnumerable<..> instead and then use yield return to return each data pair (see below).
  2. I would define a struct or class instead of using a named tuple. IMO named tuples are only useful very locally, because they can be harder to maintain, if their definition changes and you distribute them across the application (see below).

  var IPNFPTechPair = (IPN: line.Split(Delimiter)[IPNPos], FPTech: line.Split(Delimiter)[FPPos]);

Here you split the same string twice. It would be better to do:

string[] cells = line.Split(Delimiter);
var IPNFPTechPair = (IPN: cells[IPNPos], FPTech: cells[FPPos]);

You should handle all the heading stuff in a separate method in order to make the design and responsibility more clear.


All in all I would do something like:

public IEnumerable<IpnItem> Read(string filePath)
{
  using (StreamReader reader = new StreamReader(filePath))
  {
    (int ipnColumn, int fptechColumn) = ReadHeaders(reader);

    string line;
    while ((line = reader.ReadLine()) != null)
    {
      string[] cells = line.Split(Delimiter);
      yield return new IpnItem(cells[ipnColumn], cells[fptechColumn]);
    }
  }
}

private (int ipnColumn, int fptechColumn) ReadHeaders(StreamReader reader)
{
  string[] headers = reader.ReadLine().Split(Delimiter);
  if (headers.Length != 2) throw new Exception("Number of column headers should be 2");
  (int ipnColumn, int fptechColumn) columnIndices = (Array.IndexOf(headers, "PartNumber"), Array.IndexOf(headers, "FPTech"));
  if (columnIndices.ipnColumn == -1 || columnIndices.fptechColumn == -1) throw new Exception("Invalid or missing column names");
  return columnIndices;
}

Where IpnItem is declared as:

  public class IpnItem
  {
    public IpnItem(string ipn, string fpTech)
    {
      IPN = ipn;
      FPTech = fpTech;
    }

    public string IPN { get; private set; }
    public string FPTech { get; private set; }
  }
\$\endgroup\$
1
  • \$\begingroup\$ I'd also re-use the ReadHeaders function to read the data rows (and rename accordingly) - the function presumably already contains the required logic to split a line, as there's no difference in the syntax. \$\endgroup\$
    – user190892
    Mar 7, 2019 at 23:01
1
\$\begingroup\$

Another option you have is to read the data into a DataTable. This represents the data basically the same as a list of a custom type without having to create that type.

Once the data is read, it can be searched for null values and the return set accordingly, or if it might be desired it is relatively simple to return a set of rows that contain the incomplete data.

Simple function for this could look like this:

public DataTable GetIPNFPTechFromCSV(string fileName)
{
    const string BOM = "";
    string directory = System.IO.Path.GetDirectoryName(fileName);
    if( directory == "")
    {
        directory = @".\";
    }
    string file = System.IO.Path.GetFileName(fileName);
    OdbcConnection conn = new OdbcConnection($@"Driver={{Microsoft Text Driver (*.txt; *.csv)}};Dbq={directory};FMT=Delimited','");
     using (OdbcDataAdapter da = new OdbcDataAdapter($@"Select * from [{file}]", conn))
    DataTable dt = new DataTable();
    using (OdbcDataAdapter da = new OdbcDataAdapter(com))
    {
        da.Fill(dt);
    }
    if(dt.Columns[0].ColumnName.StartsWith(BOM))
    {
        dt.Columns[0].ColumnName = dt.Columns[0].ColumnName.Trim(BOM.ToArray());
    }
    bool goodData = dt.Select().All(x => !x.ItemArray.Contains(DBNull.Value));
    if(goodData)
    {
        return dt;
    }
    else
    {
        return null;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ You can use Encoding.UTF8.GetPreamble() instead of hardcoding the BOM :-) \$\endgroup\$
    – t3chb0t
    Mar 8, 2019 at 8:13

Your Answer

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

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