5
\$\begingroup\$

As part of a solution I have the following class createCSV. It is working as expected but I'm very much aware of my lack of experience with c# and OO programming: are there any obvious potential problems or simple refactors?

(note: the use of a guid is a user requirement)

using System.Text;
using System.IO;
using System.Data;
using System;
using System.Collections.Generic;

namespace createCSV
{
  class Csv
  {

    private string guid;

    public Csv(string aGuid)
    {
      guid = aGuid;
    }

    public void CreateCSV(DataTable dt,string aDestPath, string aTitle)
    {

      string filePath = string.Concat(aDestPath, aTitle, @"-", this.guid, @".csv");
      string delimiter = ",";


      StringBuilder sb = new StringBuilder();
      List<string> CsvRow = new List<string>();


      //write headers
      foreach(DataColumn c in dt.Columns)
      {
        CsvRow.Add(c.ColumnName.ToString());
      }
      sb.AppendLine(string.Join(delimiter,CsvRow));


      //write data
      foreach(DataRow r in dt.Rows)
      {
        CsvRow.Clear();

        //go through each column adding to a list of strings
        foreach(DataColumn c in dt.Columns)
        {
          CsvRow.Add(r[c.ColumnName].ToString());
        }

        sb.AppendLine(string.Join(delimiter,CsvRow));
      }



      File.WriteAllText(filePath,sb.ToString());

      Console.WriteLine();
      Console.WriteLine(String.Concat(@"just completed: ",aTitle,@"-",this.guid,@".csv"));
    }


  }
}
\$\endgroup\$

5 Answers 5

3
\$\begingroup\$

See my inline comments below:

  class Csv
  {

    // By having this guid provided through the constructor (and the rest of the file path as arguments for the CreateCSV-function) you limit the use of the class to a very special case
    // Instead create the file path elsewhere and make it an argument to the CreateCSV: CreateCSV(DataTable table, string filePath)
    private string guid;


    public CsvOriginal(string aGuid)
    {
      guid = aGuid;
    }

    // As others have mentioned make some simple names like table, path, title
    public void CreateCSV(DataTable dt, string aDestPath, string aTitle)
    {
      // Check if the destination folder exists. If not, throw an exception now instead of waste the clients time by iterating say 10,000 lines just to let File.WriteAllText() throw a DiretoryNotFoundException at the end of the method
      if (!Directory.Exists(aDestPath))
        throw new DirectoryNotFoundException($"Directory not found: {aDestPath}");

      // The @"" operator is only necessary when dealing with paths like in @"C:\My Folder\My SubFolder". Without you'll have to do this: "C:\\My Folder\\My SubFolder"
      // Instead of this:
      //string filePath = string.Concat(aDestPath, aTitle, @"-", this.guid, @".csv");
      // .. I would do it like this:
      string filePath = Path.Combine(aDestPath, $"{aTitle}-{guid}.csv");
      string delimiter = ",";

      StringBuilder sb = new StringBuilder();
      List<string> CsvRow = new List<string>();


      //write headers
      foreach (DataColumn c in dt.Columns)
      {
        // The c.ColumnName is a string itself so no need for ToString()
        CsvRow.Add(c.ColumnName);
      }
      sb.AppendLine(string.Join(delimiter, CsvRow));


      //write data
      foreach (DataRow r in dt.Rows)
      {
        CsvRow.Clear();

        //go through each column adding to a list of strings
        foreach (DataColumn c in dt.Columns)
        {
          // You can actual use the column directly as index:
          CsvRow.Add(r[c].ToString());
          //CsvRow.Add(r[c.ColumnName].ToString());
        }

        sb.AppendLine(string.Join(delimiter, CsvRow));
      }

      File.WriteAllText(filePath, sb.ToString());

      // Don't write messages to standard output inside a function. Let the caller/client do the talking...
      //Console.WriteLine();
      //Console.WriteLine(String.Concat(@"just completed: ", aTitle, @"-", this.guid, @".csv"));
    }
  }

If you plan to continue programming in C#, you should spend some effort on LINQ.

By using LINQ you could end up doing something like this:

public void CreateCSV(DataTable dataTable, string filePath, string delimiter = ",")
{
  if (!Directory.Exists(Path.GetDirectoryName(filePath)))
    throw new DirectoryNotFoundException($"Destination folder not found: {filePath}");

  DataColumn[] columns = dataTable.Columns.Cast<DataColumn>().ToArray();
  List<string> lines = new List<string>();
  lines.Add(string.Join(delimiter, columns.Select(c => c.ColumnName)));
  lines.AddRange(dataTable.Rows.Cast<DataRow>().Select(row => string.Join(delimiter, columns.Select(c => row[c]))));
  File.WriteAllLines(filePath, lines);
}

Appended by @Insta:

Or, further, as a real-time streaming solution (no double-iteration):

public void CreateCSV(DataTable dataTable, string filePath, string delimiter = ",")
{
  if (!Directory.Exists(Path.GetDirectoryName(filePath)))
    throw new DirectoryNotFoundException($"Destination folder not found: {filePath}");

  var columns = dataTable.Columns.Cast<DataColumn>().ToArray();

  var lines = (new [] { string.Join(delimiter, columns.Select(c => c.ColumnName)) })
    .Union(dataTable.Rows.Cast<DataRow>().Select(row => string.Join(delimiter, columns.Select(c => row[c]))))

  File.WriteAllLines(filePath, lines);
}
\$\endgroup\$
7
  • 2
    \$\begingroup\$ This could be a nice extension for the DataTable like .ToCsv(). (/OT: +1 tomorrow, no more votes today). \$\endgroup\$
    – t3chb0t
    Feb 8, 2017 at 20:29
  • 2
    \$\begingroup\$ WriteAllLines takes an IEnumerable -- you can do this whole thing with a Union for the header row and the casted datatable rows, without double-buffering it to a List first. \$\endgroup\$ Feb 8, 2017 at 22:33
  • \$\begingroup\$ @insta: You may be right, but I can't see how you can avoid a local instantiation of some kind to do the union? \$\endgroup\$
    – user73941
    Feb 9, 2017 at 5:43
  • \$\begingroup\$ @HenrikHansen: may I append to your answer? You can keep or delete it as you feel necessary. It's a bit more than will fit in a comment, but not answer-worthy on its own. \$\endgroup\$ Feb 9, 2017 at 15:11
  • \$\begingroup\$ @insta: You are welcome to append. \$\endgroup\$
    – user73941
    Feb 9, 2017 at 16:24
3
\$\begingroup\$

Before you do other things you should separate and encapsulate, this means you should put certain logic into their own methods that specialize in doing only one thing. If you put e.g. the loops in such method methods, you won't need any comments. Here's an example of how it could be done with C# 7. Also notice the changed names. There's no need for the a/an/the prefixes.

This is just a start and there is much more to improve. I left it out intentionally.

public void CreateCsv(DataTable data, string path, string title)
{
    string CreateFileName() => string.Concat(path, title, @"-", guid, @".csv");

    void WriteHeader(StringBuilder csv)
    {
        var header = new List<string>();
        foreach (var column in data.Columns.Cast<DataColumn>())
        {
            header.Add(column .ColumnName);
        }
        csv.AppendLine(string.Join(Delimiter, header));
    }

    void WriteData(StringBuilder csv)
    {
        foreach (var row in data.Rows.Cast<DataRow>())
        {
            var values = new List<string>();
            foreach (var column in data.Columns.Cast<DataColumn>())
            {
                values.Add(row[column.ColumnName].ToString());
            }
            csv.AppendLine(string.Join(Delimiter, values));
        }
    }

    string WriteCsv() 
    {
        var csv = new StringBuilder();  
        WriteHeader(csv);
        WriteData(csv);
        return csv.ToString();
    }           

    File.WriteAllText(CreateFileName(), WriteCsv());

    Console.WriteLine();
    Console.WriteLine(String.Concat(@"just completed: ", title, @"-", this.guid, @".csv"));
}
\$\endgroup\$
8
  • \$\begingroup\$ Thanks for the help. You've separated concerns very nicely. What is the purpose of this operator => ? \$\endgroup\$
    – whytheq
    Feb 8, 2017 at 18:47
  • 2
    \$\begingroup\$ @whytheq sometimes when a method has only a single line you might want and like to use Expression-bodied functions instead of using the {}. \$\endgroup\$
    – t3chb0t
    Feb 8, 2017 at 18:57
  • \$\begingroup\$ why wouldn't we move WriteHeader and WriteData outside of the CreateCsv method and just have them as private helper methods of the class? \$\endgroup\$
    – whytheq
    Feb 8, 2017 at 19:21
  • \$\begingroup\$ the solution is currently using .NET 4.5.2 - should I specify a more recent version for the above to work? I guess => will not work unless I'm using a more recent version? Apologies for ignorance but how do i determine the minimum version of the framework I need for all functionality of a particular version of the C# language to work ok? \$\endgroup\$
    – whytheq
    Feb 8, 2017 at 19:27
  • \$\begingroup\$ @whytheq no particular reason; they can perfectly be private helper methods and with additional parameters they would work exacly the same; I just like the new local methods so I put them there. \$\endgroup\$
    – t3chb0t
    Feb 8, 2017 at 19:30
3
\$\begingroup\$

There are several other things to consider when creating csv files. Values can contain commas or new line characters. In those cases the value needs to be quoted. Also if values contain quotes the quotes need to be doubled to escape them.

I think all values can be quoted to simplify the logic, or you can quote only those values that need to be.

It may be better to use some existing function or library that has been tested more thoroughly.

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

Pretty minor

Accessing a DataRow column by index (integer) is a little more efficient but if these are relatively small DataTables then ColumnName is OK

Add some error checking on if filepath is valid

At first I thought sb.AppendLine(string.Join(delimiter,CsvRow)); was odd but it seems fine

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

Since you are already using System.IO, I would create the file name string with Path.Combine(). This would delegate the need to check if the proper directory separator is already at the end of the destination path.

I'm not sure why you are using string literals(@). I think that the use of an interpolated string($) would improve readability for that part.

Another issue that seems to happen with csv creation is the presence of delimiters in the data. This can be solved by either stripping the data of the delimiter or by implementing a text qualifier, if the requirements permit. This can be implemented with an optional parameter if the qualifier is constant.

To improve reusability, I would give the option to include/exclude the column header row. This can be done with an optional parameter as well.

class Csv
{
    private string delimiter = ",";
    private string qualifier = "'";

    private DataTable data;
    private string guid;
    private string outputpath;
    private string outputFileName;
    private bool useHeader;
    private bool textQualified;

    public Csv(string guid, DataTable data, string path, string title,
        bool useHeader = true, bool textQualified = false)
    {
        this.guid = guid;
        this.data = data;
        this.outputPath = path;
        this.outputFileName = $"{title}-{guid}.csv";
        this.useHeader = useHeader;
        this.textQualified = textQualified;
    }

    public void WriteToFile()
    {
        string CreateFileName() => Path.Combine(path, $"{outputFileName}");
        StringBuilder sb = new StringBuilder();
        WriteHeader(sb);
        WriteData(sb);
        File.WriteAllText(CreateFileName() ,sb.ToString());

        Console.WriteLine();
        Console.WriteLine($"just completed: {outputFileName}");
    }

    private void WriteHeader(StringBuilder sb)
    {
        if(!useHeader)
            return;

        var header = new List<string>();
        foreach (var column in data.Columns.Cast<DataColumn>())
        {
            header.Add(column.ColumnName);
        }
        sb.AppendLine(string.Join(delimiter , header));
    }

    private void WriteData(StringBuilder sb)
    {
        foreach (var row in data.Rows)
        {
            var elements= new List<string>();
            for (int i = 0; i < data.Columns.Count; i++)
            {
                string element = row[0].ToSTring();
                if(textQualified)
                {
                    element = $"{textQualifier}{element}{textQualifier}";
                }
                else
                {
                    element = element.Replace(delimiter, string.Empty);
                }
                elements.Add(element)
            }
            sb.AppendLine(string.Join(delimiter , elements));
        }
    }
}

}

\$\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.