2
\$\begingroup\$

Problem Statement

I'm reading each line from a .csv file and parsing each comma-delimited value and casting it to the appropriate type:

string[] words = filelines[i].Split(delimiter); // "i" is the 0-based line number

// Remove double quotes encasing the timestamp
DateTime tmStamp = Convert.ToDateTime(words[0].Trim('\"'));

int recNum.Parse(words[1]); // Although unlikely, it's ok to throw FormatException here. There is a try/catch-all around this method call. 
string SSBRecvdTim = words[2].Replace("\"", "\'");
double batt_volt3,
       UWMBatt,
       SysId3,
       SIMId3,
       UWMId3,
       pTemp3,
       NumMdmOnOff,
       NumMdmSync,
       ADCP1Run,
       NoData1,
       BRK_sent1,
       UnkRecCnt1,
       ADCP1RecCnt,
       ADCP2Run,
       NoData2,
       BRK_sent2,
       UnkRecCnt2,
       ADCP2RecCnt,
       NoAQDData,
       AQDRecCnt;
try
{
    recNum       = Int32.Parse(words[1]);
    batt_volt3   = Convert.ToDouble(words[3]);
    UWMBatt      = Convert.ToDouble(words[4]);
    SysId3       = Convert.ToDouble(words[5]);
    SIMId3       = Convert.ToDouble(words[6]);
    UWMId3       = Convert.ToDouble(words[7]);
    pTemp3       = Convert.ToDouble(words[8]);
    NumMdmOnOff  = Convert.ToDouble(words[9]);
    NumMdmSync   = Convert.ToDouble(words[10]);
    ADCP1Run     = Convert.ToDouble(words[11]);
    NoData1      = Convert.ToDouble(words[12]);
    BRK_sent1    = Convert.ToDouble(words[13]);
    UnkRecCnt1   = Convert.ToDouble(words[14]);
    ADCP1RecCnt  = Convert.ToDouble(words[15]);
    ADCP2Run     = Convert.ToDouble(words[16]);
    NoData2      = Convert.ToDouble(words[17]);
    BRK_sent2    = Convert.ToDouble(words[18]);
    UnkRecCnt2   = Convert.ToDouble(words[19]);
    ADCP2RecCnt  = Convert.ToDouble(words[20]);
    NoAQDData    = Convert.ToDouble(words[21]);
    AQDRecCnt    = Convert.ToDouble(words[22]);
}
catch (FormatException fe)
{
    throw fe;
}

I've inherited this code and can't do anything about the number of variables. The problem is that the code is repetitive, verbose, and doesn't tell me which particular index causesFormatException to be thrown. The long list of doubles is also unwieldy.

Those values are used to create the query:

string query =
          $"INSERT INTO [dbo].[{tablename}] VALUES "
         +$"( '{tmStamp}', {recNum}, {SSBRecvdTim}, {batt_volt3}, {UWMBatt}, {SysId3}, {SIMId3}, {UWMId3}, {pTemp3},"
         +$" {NumMdmOnOff}, {NumMdmSync}, {ADCP1Run}, {NoData1}, {BRK_sent1}, {UnkRecCnt1}, {ADCP1RecCnt},"
         +$" {ADCP2Run}, {NoData2}, {BRK_sent2}, {UnkRecCnt2}, {ADCP2RecCnt}, {NoAQDData}, {AQDRecCnt} )";

Solution Attempt

The original developer didn't know to use a database ORM framework so queries were constructed using strings. It isn't worthwhile to introduce it now since the code is on its way to retirement.

I thought to use a dictionary to map the token # to the variable that would be assigned. I could then use a for-loop to loop over each token number. Unfortunately, this does not work because Fields stores copies of the doubles and aren't references to the doubles themselves. If you print Fields[0] you might get a double but the backing variable batt_volt will contain the value null.

Dictionary<int, double> Fields = new Dictionary<int, double>()
{
    {3,  batt_volt3  = 0},
    {4,  UWMBatt     = 0},
    {5,  SysId3      = 0},
    {6,  SIMId3      = 0},
    {7,  UWMId3      = 0},
    {8,  pTemp3      = 0},
    {9,  NumMdmOnOff = 0},
    {10, NumMdmSync  = 0},
    {11, ADCP1Run    = 0},
    {12, NoData1     = 0},
    {13, BRK_sent1   = 0},
    {14, UnkRecCnt1  = 0},
    {15, ADCP1RecCnt = 0},
    {16, ADCP2Run    = 0},
    {17, NoData2     = 0},
    {18, BRK_sent2   = 0},
    {19, UnkRecCnt2  = 0},
    {20, ADCP2RecCnt = 0},
    {21, NoAQDData   = 0},
    {22, AQDRecCnt   = 0}
};

List<int> TokenIndices = new List<int>(Fields.Keys);
foreach (var tokenIdx in TokenIndices)
{
    try
    {
        Fields[tokenIdx] = double.Parse(words[tokenIdx]);
    }
    catch (FormatException fe)
    {
        string errorMsg =
            $"FormatException thrown while trying to parse Line #{i}, Token #{tokenIdx}.";
        throw new FormatException(errorMsg, fe);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Why reinvent the wheel when there's already joshclose.github.io/CsvHelper ? Combine that with dapper-tutorial.net and your code is far easier to read and up to date. \$\endgroup\$ – BCdotWEB Mar 2 at 17:13
  • \$\begingroup\$ The code was written around 2010 by another developer who quit who likely didn't know about ORMs. Also, I'd never heard of CsvHelper nor dapper. Dapper looks like a clone of Entity Framework Extension -- also with a hefty price tag. \$\endgroup\$ – Minh Tran Mar 2 at 19:58
  • \$\begingroup\$ In real life are the variables declared locally as you show, or are they defined as class fields? \$\endgroup\$ – Henrik Hansen Mar 2 at 20:48
  • 2
    \$\begingroup\$ @MinhTran Sometimes maintaining code means throwing out the old and replacing it with new and better code. Both Dapper and CsvHelper are free, easy to use and well-documented. \$\endgroup\$ – BCdotWEB Mar 3 at 11:54
2
\$\begingroup\$

I think the best approach instead of storing the raw values in a dictionary would be to create DbParameters (assuming you are using ADO.Net).

You would need to change the sql insert statement to be parameterized - which you should as inserting raw values into a sql statement has been a bad practice for a long time. With SSBRecvdTim being a string the code is open for a sql injection.

Even with your implementation you would need to change the sql insert statement to use the dictionary since updating the dictionary isn't going to update the variables or you need to map back the dictionary to the variable.

While making it use parameters will be a tad bit more work it will be better even if the code is on the way out.

as example you could make a method like - just an example not sure what would work best in your code base.

static SqlParameter CreateParameter(string name, string value, TypeCode typeCode, int line)
{
    try
    {
        return new SqlParameter(name, Convert.ChangeType(value, typeCode));
    }
    catch (FormatException fe)
    {
        string errorMsg =
                    $"FormatException thrown while trying to parse Line #{line}, Token #{name}.";
        throw new FormatException(errorMsg, fe);
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You're right. The dictionary approach doesn't work because updating the dictionary value doesn't update the variable. \$\endgroup\$ – Minh Tran Mar 2 at 17:14
  • \$\begingroup\$ @Charles - can't you also use "[System.Runtime.CompilerServices.CallerLineNumber] int line = 0" so it would insert line number automatically? \$\endgroup\$ – yob Mar 2 at 20:54
  • \$\begingroup\$ @yob he wants the line number of the csv file that is being parsed as part of the error message not a code line number. I wouldn't put this example method in code as I'm sure there are better ways to make it more elegant but hard to know what that is without the surrounding code \$\endgroup\$ – CharlesNRice Mar 2 at 21:23

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.