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
    }
}

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);
  

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.

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?

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++)