Return to Answer

• 1

• 50.9k
• 5
• 83
• 177

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?