Strongly-typed reading values from CSV DataTable

Question

Is there a way to do this using parameters so the value is automatically converted to whatever datatype the keyfield has in the datatable?

This code should be reusable for future bulk update applications hence the constant and the check on multiple datatypes.

private const string DBKEYFIELDNAME = "CustomerNr";
... 
for (int i = 1; i <= csvLines.Length - 1; i++) // i=1 => skip header line
{
 string[] csvFieldsArray = csvLines[i].Split(';');
 int indexKeyField = csvHeaders.IndexOf(CSVKEYFIELDNAME.ToLower());
 object csvKeyValue = csvFieldsArray[indexKeyField];
 // ... some more code here that is not relevant
 // Find the matching row for our csv keyfield value
 Type keyType = parameters.DataTableOriginal.Columns[DBKEYFIELDNAME].DataType;
 DataRow[] rowsOriginal = null;
 if (keyType.IsAssignableFrom(typeof(string)))
 rowsOriginal = parameters.DataTableOriginal.Select(DBKEYFIELDNAME + "='" + csvKeyValue.ToString() + "'");
 else if (keyType.IsAssignableFrom(typeof(Int16)) || keyType.IsAssignableFrom(typeof(Int32)) || keyType.IsAssignableFrom(typeof(Int64)) || keyType.IsAssignableFrom(typeof(bool)))
 rowsOriginal = parameters.DataTableOriginal.Select(DBKEYFIELDNAME + "=" + csvKeyValue);
 if (rowsOriginal != null && rowsOriginal.Length == 1)
 {
 // Do some processing of the row here
 }
}

• Saeed: @Peter, where did csvKeyValue come from? Who generates it? Do you have control over it? From its name, it sounds like it was read from a csv file at some point, and whoever read it also converted it to a string, int, or bool. Is that correct? And how is it passed in to this chunk of code? As an object? Is this code in a separate method? If so, consider posting the signature of this method.
• Peter: It's quite a big method so I'd rather not as it takes the focus of the code I want reviewed, I'll update my question with more info on the csvKeyValue variable.
• Peter: Okay... The datatype is always going to be a string in the CSV... I still need to write some logic to get the datatype of the matching column in the DataTableOriginal.
• Michael K: What is DBSLEUTELVELDNAAM?
• Peter Taylor: Is i <= csvLines.Length - 1 idiomatic in this codebase? I think most people would consider i < csvLines.Length more idiomatic, and the fewer surprises the better.

Answer 1

A few comments:

• This method does way too much. It is difficult to understand and will be difficult to debug and maintain. Break it down into smaller methods that each have a single responsibility.
• Use curly braces after your if and else statements. This improves readability of the code and makes it less likely for other code to sneak in there in the future.
• Can't your else statement just be a plain else with no if after it? It seems like you want to put quotes around a string, and use the plain value for everything else. Are there other requirements here?
• The type of csvKeyValue could just be string, since it's pulling a value out of a string[]
• No need for the call to .ToString() in your if branch

• I would try to write the call to parameters.DataTableOriginal.Select only once. Consider using your if/else to set a delimeter variable to either string.Empty or ', then write your query once like so:

  DataRow[] rowsOriginal = parameters.DataTableOriginal.Select(
   DBSLEUTELVELDNAAM + "=" + delimeter + csvKeyValue + delimeter);
  

• Peter: I agree on all point except for using brackets on the if statement. Especially when just using it to set a delimiter variable as you suggested. But that's personal opinion. Thanks for the pointers! I will split this method into different pieces as you are right, it is big and clunky. And you've only seen one 5th of it :-)

Answer 2

I noticed this code: parameters.DataTableOriginal.Select(DBKEYFIELDNAME + "='" + csvKeyValue.ToString() + "'"). Just putting quotes around an user-input screams SQL Injection to me.

The problem is that your code assumes that csvKeyValue has no quotes in it. That assumption is never checked anywhere, while csvKeyValue is read from an external file so cannot be trusted.

Suppose the csvKeyValue equals "a' or TRUE or 'x' ='x". Do you really want to execute that select?

Although I cannot judge whether this is a real danger, I thought I should mention it.

Answer 3

Yup, that method does definitely need breaking down into smaller pieces.

You may like to use an iterator method to loop through the lines. Something along the lines of :

IEnumerable<string> EachCSVKey(string[] csvLines)
{
 int indexKeyField = csvHeaders.IndexOf(CSVKEYFIELDNAME.ToLower());
 for (int i = 1; i <= csvLines.Length - 1; i++) // i=1 => skip header line
 {
 string[] csvFieldsArray = csvLines[i].Split(';');
 yield return csvFieldsArray[indexKeyField];
 }
}

Then in your method :

foreach ( var csvKeyValue in EachCSVKey ( csvLines) )
{ 
 ...

I find (in my opinion) that this makes the intent clearer.

Note also you should get the indexKeyField value outside of the loop. This value won't change, and so it is unnecessary to keep getting it.

• Peter: It looks cleaner but it can't be as performant because you are looping the entire csv twice?
• Mongus Pong: No, you only loop once - thats the clever part! Each time the EachCSVKey method hits the "yield return" statement it will transfer control to the calling methods foreach loop. When the calling foreach loop finishes an iteration, it returns control to the EachCSVKey method. This continues until EachCSVKey finishes its loop or the calling loop calls "break".

Answer 4

Just one small comment in addition to those provided by Saeed and Mongus, you have defined a constant called CSVKEYFIELDNAME but in the only place it is (shown to be) used, you call ToLower() on it. Should the constant be provided as lower case to begin with, or should you be doing a case insensitive search?

• Peter: Well, the thing is, the csvheaders array has received a tolower upon initialisation. I do a tolower on the constant to allow someone to change the value into a readable string without having to know it needs to be in lowercase always.
• Brian Reichle: @Peter, sounds like a case insensitive search is in order then :). Ultimately it's up to you, creating a new string and preforming multiple case insensitive string comparisons will both have performance impacts, though which is greater would probably depend on the number of columns you are comparing against. in any case, you might want to consider the effects of culture settings on your comparisons (stackoverflow.com/questions/2256453/…).
• Peter Taylor: @Peter, this is C#, isn't it? So make the string a const and then wrap it in a property which does the ToLower() and you get the best of both worlds.

Answer 5

Is there a way to do this using parameters so the value is automatically converted to whatever datatype the keyfield has in the datatable?

You could use something like this:

object csvKeyValue = Convert.ChangeType(csvKeyValue, keyType);

to convert a string to the type of the datatable column.

Answer 6

Prefer var in for loop variable initializers:

for (int i = 1; i <= csvLines.Length - 1; i++)

Should be

for (var i = 1; i <= csvLines.Length - 1; i++)