2
\$\begingroup\$

A generic class to get object collection from csv string. I would like to know if there's any better way?

Converter class

public class CsvConverter<T>:List<T> where T:class
{
    private readonly string _csvText;
    private readonly string _separetor;

    /// <summary>
    /// Get object collection from CSV string
    /// </summary>
    /// <param name="csvText">CSV string with Title</param>
    /// <param name="separator">CSV line separator</param>
    public CsvConverter(string csvText, string separator= "\r\n")
    {
        this._csvText = csvText;
        _separetor = separator;
        BuildObject();
    }

    /// <summary>
    /// Get CSV string from collection of objects
    /// </summary>
    /// <param name="collection">Pass collection of objects</param>
    public CsvConverter(IEnumerable<T> collection)
    {
        foreach (var item in collection)
        {
            this.Add(item);
        }
    }

    /// <summary>
    /// Create new Instance of T object
    /// </summary>
    /// <returns></returns>
    protected T GetObject()
    {
        return (T)Activator.CreateInstance(typeof(T),true);
    }

    /// <summary>
    /// Build object from CSV string to collection
    /// </summary>
    private void BuildObject()
    {
        if (!string.IsNullOrEmpty(_csvText))
        {
            //Get first line from CSV string, than split by comma to get column names
            var _csvLines = _csvText.Split(_separetor);
            var _columnName =_csvLines[0].Split(',');


            for (int i = 1; i < _csvLines.Length; i++)
            {
                var line = _csvLines[i].Split(',');

                T obj = GetObject();
                Type t = obj.GetType();
                PropertyInfo[] props = t.GetProperties();

                int colIndx = 0;
                foreach (string str in line)
                {
                    PropertyInfo find = props.First(x => x.Name == _columnName[colIndx]);
                    find.SetValue(obj, Convert.ChangeType(str,find.PropertyType),null);

                    //propertyInfo.SetValue(propertyInfo.PropertyType, line[0]);
                    colIndx++;
                }
                this.Add(obj);
            }
        }
        else
        {
            throw new System.NullReferenceException("No CSV string has passed in the parameter");
        }
    }

    /// <summary>
    /// Returns CSV string from collection of T object
    /// </summary>
    /// <returns>string</returns>
    public string GetCsvString()
    {
        StringBuilder sb = new StringBuilder();
        foreach (var item in this)
        {
            Type t = item.GetType();
            PropertyInfo[] props = t.GetProperties();
            string ln = "";
            foreach (PropertyInfo prp in props)
            {
                ln= ln+($"{prp.GetValue(item)},");
            }
            sb.AppendLine($"{ln.TrimEnd(',')}");
        }
        return sb.ToString();
    }
}

get collection of object from csv string

        var csv = await System.IO.File.ReadAllTextAsync("test.csv");
        List<TestC> csvBarcode = new CsvConverter<TestC>(csv.TrimEnd());

get csv string from collection of object

        CsvConverter<TestC> b = new CsvConverter<TestC>(list);
        return Content(b.GetCsvString());
\$\endgroup\$
  • \$\begingroup\$ Why does your class inherit from List<T>? \$\endgroup\$ – Pieter Witvoet Oct 9 '18 at 13:26
  • \$\begingroup\$ Yeah, it also useless when I simplify with static class ;) \$\endgroup\$ – Siraj M Oct 10 '18 at 6:26
5
\$\begingroup\$

I would suggest using the new() generic type constraint. Using Activator means you can match a private constructor which you would no longer be the case with the generic constraint but you would be able to catch incorrect use at compile time. Currently, if I tried to use a class like:

 public class Foo
 {
     public Foo(string bar, double baz) { /* ... */ }
 }

Your class would throw at runtime (a MethodMissingException I believe). If you added where T : class, new() instead of just where T : class, trying to use your class with the Foo type would produce a compile time error. An error I can fix before having to run the code is the most helpful kind.


Doing reflection in a loop when you don't need to is not a good thing. You do this for every line in the input file:

 T obj = GetObject();
 Type t = obj.GetType();
 PropertyInfo[] props = t.GetProperties();

You can create this all before processing the file and you don't need to create an instance to get the type information:

PropertyInfo[] props = typeof(T).GetProperties();

for (int i = 1; i < _csvLines.Length; i++)
{
    T obj = GetObject(); // or T obj = new T(); with the constraint added.

You probably want the PropertyInfo[] to be an IDictionary<string, PropertyInfo>. to simplify the lookup by column name.


As for whether this should be a class or not... If you do the reflection in the constructor and keep the instance around to process multiple files then I think it is okay to be a class to save on having to do the reflection over and over again.

If you are going to create an instance to process a file and then throw it away, or if you don't move the reflection to the constructor, I think it would be better as a static class with 2 methods as the other review suggests.


It's spelt: "separator" not "separetor". You've used both spellings. It's a word that I almost always spell wrong too :)

\$\endgroup\$
  • \$\begingroup\$ Creating new instance of T to set property value here, \$\endgroup\$ – Siraj M Oct 10 '18 at 4:16
3
\$\begingroup\$

I would rather see 2 static methods to convert a CSV to a collection of objects, and vice versa.

A constructor should be as quick as possible to construct a new instance. You violate this with the call to BuildObject().

In GetCsvString, the variable ln should be a StringBuilder since you alter in within the innermost foreach loop. The inner loop could probably be reduced to one line using string.Join.

\$\endgroup\$
  • 1
    \$\begingroup\$ I did change it to static class it's better, thanks. \$\endgroup\$ – Siraj M Oct 10 '18 at 6:14
1
\$\begingroup\$
throw new System.NullReferenceException("No CSV string has passed in the parameter");

You should not be throwing this excepiton in the BuildObject method (actually nowhere). It's not only that it is used only by the framework itself but it's also misleading. What parameter does it mean? Especially that BuildObject does not have any! If your utility crashes and someone else would be looking for the cause, it would be super confusing for him.

You have two choices how to fix it. First, and this is the best one, validate the parameters in the constructor so it's not possible to create it with invalid parameters. Second, slightly worse but still more helpful than the current implementation, you can throw the InvalidOperationException and tell the user that the object has been initialized with invalid data. Either way the user would instantly know what is wrong.

\$\endgroup\$
  • \$\begingroup\$ The problem(what you suggested) is actually solved when I am using a static class with two simple method. \$\endgroup\$ – Siraj M Oct 11 '18 at 11:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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