1
\$\begingroup\$

I made a dynamic CSV formatter as a personal challenge after having to hardcode a behavior to convert some data to CSV earlier. You input any List that contains supported types as properties (all primitive types, strings, or collections of primitive types or strings). And it will spit out a list of strings that are each a line of a CSV.

It currently ignores non-accepted types and anything that is not a property, if there is no valid input it will throw an ArgumentException. How can I improve this? Is there any major flaws you notice? What would you do differently? Would you split any of this up into different classes?

public class CSVFormatter<T>
{

    public List<string> GetCSV(ICollection<T> input)
    {
        List<List<string>> dataStrings = GetCSVDataStrings(input);
        List<string> output = new List<string>();

        foreach (List<string> row in dataStrings)
        {
            output.Add(FormatCSVRow(row));
        }
        return output;

    }

    #region Property Retrieval
    //Retrieves a 2-Dimensional array of strigns that represent the input classes
    private List<List<string>> GetCSVDataStrings(ICollection<T> input)
    {
        List<List<string>> output = new List<List<string>>();
        List<ValidType> properties = FilterProperties(new List<PropertyInfo>(typeof(T).GetProperties()));
        if (properties.Count != 0)
        {
            output.Add(GetHeaders(properties));

            foreach (T item in input)
            {
                output.Add(GetDataRowAsStrings(item, properties));
            }
            return output;
        }
        else
            throw new ArgumentException("There was no valid input to format as a CSV");

    }

    //Converts each type T into a list of  based on it's data
    private List<string> GetDataRowAsStrings(T input, List<ValidType> properties)
    {
        List<string> output = new List<string>();
        foreach(ValidType property in properties)
        {
            if(!property.IsCollection)
            {
                output.Add(MakeStringSafe(CleanString(property.PropertyInformation.GetValue(input).ToString())));
            }
            else
            {
                output.Add(MakeStringSafe(CleanString(FormatMultiItemCSVCell(GetStringDataFromGenericCollection(property.PropertyInformation, input)))));
            }
        }
        return output;
    }

    //Filters out non accpted Types
    private List<ValidType> FilterProperties(List<PropertyInfo> properties)
    {
        List<ValidType> output = new List<ValidType>();

        foreach(PropertyInfo property in properties)
        {
            if (property.PropertyType.IsGenericType)
            {
                Type interfac1e = property.PropertyType.GetInterface(typeof(ICollection<>).Name);
                if(interfac1e != null)
                {
                    if (interfac1e.Name == typeof(ICollection<>).Name)
                    {
                        if (property.PropertyType.GenericTypeArguments[0].IsPrimitive || property.PropertyType.GenericTypeArguments[0] == typeof(string))
                            output.Add(new ValidType(true, property));
                    }
                }
                //bool testType = property.PropertyType.GetGenericTypeDefinition().IsSubclassOf(typeof(ICollection<>));

            }
            else if(property.PropertyType.IsPrimitive || property.PropertyType == typeof(string))
            {
                output.Add(new ValidType(false, property));
            }
        }

        return output;
    }

    //Gets the string headers for each applicable type in T
    private List<string> GetHeaders(List<ValidType> properties)
    {
        List<string> propertyStrings = new List<string>();
        foreach(ValidType validType in properties)
        {
            propertyStrings.Add(validType.PropertyInformation.Name);
        }
        return propertyStrings;
    }

    #endregion

    //Takes a list of strings and formats them in a CSV style
    private string FormatCSVRow(List<string> strings)
    {
        string formatString = "{0}";
        string outputString = "";
        for (int i = 0; i < strings.Count; i++)
        {
            if (i == strings.Count - 1)
            {
                outputString += string.Format(formatString, strings[i]);
            }
            else
            {
                outputString += string.Format(formatString + ",", strings[i]);
            }
        }
        return outputString;
    }

    private List<string> GetStringDataFromGenericCollection(PropertyInfo info, T item)
    {
        List<string> output = new List<string>();
        IEnumerable collectionObject = (IEnumerable) info.GetValue(item);
        if(collectionObject != null)
        {
            output = collectionObject.Cast<object>().Select(e => e.ToString()).ToList();
        }
        return output;
    }

    //Takes a list of strings a puts them into a single string that be a single CSV item
    private string FormatMultiItemCSVCell(ICollection<string> input)
    {
        string output = "";
        for(int i = 0; i < input.Count; i++)
        {
            if(i == input.Count - 1)
            {
                output += input.ElementAt(i);
            }
            else
            {
                output += input.ElementAt(i) + ", ";
            }
        }
        return output;
    }

    //Cleans a string of any new lines or line breaks
    private string CleanString(string input)
    {
        if (String.IsNullOrEmpty(input))
        {
            return input;
        }
        string lineSeparator = ((char)0x2028).ToString();
        string paragraphSeparator = ((char)0x2029).ToString();

        return input.Replace("\r\n", string.Empty).Replace("\n", string.Empty).Replace("\r", string.Empty).Replace(lineSeparator, string.Empty).Replace(paragraphSeparator, string.Empty);
    }

    //Encases any comma containing strings in quotes
    private string MakeStringSafe(string input)
    {
        if(input.Contains(","))
            return "\"" + input + "\"";
        return input;

    }

    private class ValidType
    {
        public ValidType(bool isCollection, PropertyInfo propertyInfo)
        {
            IsCollection = isCollection;
            PropertyInformation = propertyInfo;
        }

        public bool IsCollection { get; set; }
        public PropertyInfo PropertyInformation { get; set; }
    }
}

Here is a paste-able console application that will do a basic test of the formatter:

   class Program
{
    static void Main(string[] args)
    {

        List<TestObject> testList = new List<TestObject>();
        CSVFormatter<TestObject> formatter = new CSVFormatter<TestObject>();

        for (int i = 0; i < 50; i++)
        {
            TestObject test = new TestObject()
            {
                Calls = 90,
                Name = "Fredrick",
                Times = new List<int>() { 1, 2, 3, 4 }
            };

            testList.Add(test);
        }

        List<string> output = formatter.GetCSV(testList);
        string testPaste = "";
        using (StreamWriter writer = new StreamWriter("testoutput.csv"))
        {
            foreach (string item in output)
            {
                writer.WriteLine(item);
                writer.Flush();
                testPaste += item + "\n\b";
                Console.WriteLine(item);
            }
            writer.Close();
        }

        Console.ReadLine();
    }
}

public class TestObject
{
    public int Calls { get; set; }
    public string Name { get; set; }
    public List<int> Times { get; set; }
    public List<string> TestEmptyCollection { get; set; }
    public CSVFormatter<TestObject> TestNonSupportedGeneric { get; set; }
}
\$\endgroup\$
2
\$\begingroup\$
private string FormatCSVRow(List<string> strings)
{
    string formatString = "{0}";
    string outputString = "";
    for (int i = 0; i < strings.Count; i++)
    {
        if (i == strings.Count - 1)
        {
            outputString += string.Format(formatString, strings[i]);
        }
        else
        {
            outputString += string.Format(formatString + ",", strings[i]);
        }
    }
    return outputString;
}  
  • there is no need to use string.Format here, it only has some negative impact regarding performance.

  • one shouldn't use string concatenation in a loop, use a StringBuilder instead. By using += with strings in a loop, you are each time creating a new string object because strings are immutable.

  • use the right tool for the job, which would be the string.Join() method like so

    private string FormatCSVRow(List<string> strings)
    {
        return string.Join(",", strings);
    }  
    

private List<string> GetDataRowAsStrings(T input, List<ValidType> properties)
{
    List<string> output = new List<string>();
    foreach(ValidType property in properties)
    {
        if(!property.IsCollection)
        {
            output.Add(MakeStringSafe(CleanString(property.PropertyInformation.GetValue(input).ToString())));
        }
        else
        {
            output.Add(MakeStringSafe(CleanString(FormatMultiItemCSVCell(GetStringDataFromGenericCollection(property.PropertyInformation, input)))));
        }
    }
    return output;
}

IMO it is ok to pass the result of a method call as a parameter to a method but IMO it should be at max one method call. Sam the maintainer would become desparate at seeing the instruction of the else of this method.

One shouldn't do anything what is allowed. Coding is a lot about readability which this method misses because of the reasons stated.


private string CleanString(string input)
{
    if (String.IsNullOrEmpty(input))
    {
        return input;
    }
    string lineSeparator = ((char)0x2028).ToString();
    string paragraphSeparator = ((char)0x2029).ToString();

    return input.Replace("\r\n", string.Empty).Replace("\n", string.Empty).Replace("\r", string.Empty).Replace(lineSeparator, string.Empty).Replace(paragraphSeparator, string.Empty);
}

For the sake of readability the return statement should be written like so

private string CleanString(string input)
{
    if (String.IsNullOrEmpty(input))
    {
        return input;
    }
    string lineSeparator = ((char)0x2028).ToString();
    string paragraphSeparator = ((char)0x2029).ToString();

    return input.Replace("\r\n", string.Empty)
                .Replace("\n", string.Empty)
                .Replace("\r", string.Empty)
                .Replace(lineSeparator, string.Empty)
                .Replace(paragraphSeparator, string.Empty);
}

which is much easier to grasp at first glance without any scrolling.


General

  • you should always use braces {} although they might be optional for single instructions of if's. Using them will lead to less error prone code. Right now you are sometimes using them and sometimes you don't.

  • commented out code is dead code and should be deleted because it is only adding noise.

  • Are #regions an antipattern or code smell?

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! I accidentally let the commented out code in there and it has been removed. I've went ahead and used a string builder instead of the loop,cleaned up my if/else formatting..etc I used the region to differentiate that set of methods from the rest so it was easier on the eyes (at least for me it was) and made it easier to find. Should I instead split some of it into a different class? I don't think that there a significant enough change in responsibilities to warrant that though. \$\endgroup\$ – Douglas Gaskell Nov 13 '15 at 8:17

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.