1
\$\begingroup\$

This code basically checks for four types of alarm flags in .ItemArray[5] to .ItemArray[8]. If an alarm flag is true then its corresponding setpoint is checked for realtime value. The setpoints are in .Itemarray[1] to .Itemarray[5] and the real time value is in engValues[i]

If an alarm exists a row is added to a table (temporarily commented).

A stopwatch is used to see the time taken by the code.

if (connection1.State != ConnectionState.Open)
    connection1.Open();
for (int i = 0; i < alarmSet.Count; i++)
{
    Stopwatch st = Stopwatch.StartNew();
    int index = listOfTags.FindIndex(x => x.Equals(ds_alarms.Tables[0].Rows[i].ItemArray[0].ToString().Trim()));

    bool isAlarm = bool.Parse(ds_alarms.Tables[0].Rows[i].ItemArray[5].ToString()) == true;
    bool isAlarmValue;
    try
    {
        isAlarmValue = engValues[index] > float.Parse(ds_alarms.Tables[0].Rows[i].ItemArray[1].ToString());

    }
    catch (Exception excp)
    {
        isAlarmValue = false;
    }

    if (isAlarm && isAlarmValue)
    {
        sql_Al.CommandText = "INSERT INTO [MCRCDA].[dbo].[Alarms]([DateTime],[TagName],[AlarmType],[Value],[Units],[Address])    VALUES(" + time.ToString(format) + ", '" + ds_alarms.Tables[0].Rows[i].ItemArray[0].ToString().Trim() + "','HiHi'," + engValues[index] + ",'" + ds_alarms.Tables[0].Rows[i].ItemArray[9].ToString().Trim() + "','" + ds_alarms.Tables[0].Rows[i].ItemArray[10].ToString().Trim() + "')";
        try
        {

            //sql_Al.ExecuteNonQuery();

        }
        catch (Exception exp)
        {
            MessageBox.Show(exp.Message);
        }
    }

    isAlarm = bool.Parse(ds_alarms.Tables[0].Rows[i].ItemArray[6].ToString()) == true;
    try
    {
        isAlarmValue = engValues[index] > float.Parse(ds_alarms.Tables[0].Rows[i].ItemArray[2].ToString());
    }
    catch (Exception excp)
    {
        isAlarmValue = false;
    }
    if (isAlarm && isAlarmValue)
    {
        sql_Al.CommandText = "INSERT INTO [MCRCDA].[dbo].[Alarms]([DateTime],[TagName],[AlarmType],[Value],[Units],[Address])    VALUES(" + time.ToString(format) + ", '" + ds_alarms.Tables[0].Rows[i].ItemArray[0].ToString().Trim() + "','HiHi'," + engValues[index] + ",'" + ds_alarms.Tables[0].Rows[i].ItemArray[9].ToString().Trim() + "','" + ds_alarms.Tables[0].Rows[i].ItemArray[10].ToString().Trim() + "')";
        try
        {

            //sql_Al.ExecuteNonQuery();

        }
        catch (Exception exp)
        {
            MessageBox.Show(exp.Message);
        }
    }

    isAlarm = bool.Parse(ds_alarms.Tables[0].Rows[i].ItemArray[7].ToString()) == true;

    try
    {
        isAlarmValue = engValues[index] < float.Parse(ds_alarms.Tables[0].Rows[i].ItemArray[3].ToString());
    }
    catch (Exception excp)
    {
        isAlarmValue = false;
    }
    if (isAlarm && isAlarmValue)
    {
        sql_Al.CommandText = "INSERT INTO [MCRCDA].[dbo].[Alarms]([DateTime],[TagName],[AlarmType],[Value],[Units],[Address])    VALUES(" + time.ToString(format) + ", '" + ds_alarms.Tables[0].Rows[i].ItemArray[0].ToString().Trim() + "','HiHi'," + engValues[index] + ",'" + ds_alarms.Tables[0].Rows[i].ItemArray[9].ToString().Trim() + "','" + ds_alarms.Tables[0].Rows[i].ItemArray[10].ToString().Trim() + "')";
        try
        {

            //sql_Al.ExecuteNonQuery();

        }
        catch (Exception exp)
        {
            MessageBox.Show(exp.Message);
        }
    }

    isAlarm = bool.Parse(ds_alarms.Tables[0].Rows[i].ItemArray[8].ToString()) == true;
    try
    {
        isAlarmValue = engValues[index] < float.Parse(ds_alarms.Tables[0].Rows[i].ItemArray[4].ToString());
    }
    catch (Exception excp)
    {
        isAlarmValue = false;
    }

    if (isAlarm && isAlarmValue)
    {
        sql_Al.CommandText = "INSERT INTO [MCRCDA].[dbo].[Alarms]([DateTime],[TagName],[AlarmType],[Value],[Units],[Address])    VALUES(" + time.ToString(format) + ", '" + ds_alarms.Tables[0].Rows[i].ItemArray[0].ToString().Trim() + "','LoLo'," + engValues[index] + ",'" + ds_alarms.Tables[0].Rows[i].ItemArray[9].ToString().Trim() + "','" + ds_alarms.Tables[0].Rows[i].ItemArray[10].ToString().Trim() + "')";
        try
        {

            //sql_Al.ExecuteNonQuery();

        }
        catch (Exception exp)
        {
            MessageBox.Show(exp.Message);
        }
    }

    st.Stop();
}
\$\endgroup\$
  • \$\begingroup\$ On Code Review, we like titles that explain what the code does and not what you are looking for in a review, please edit your question so that what you want for a review is listed in the post's content and not the title. \$\endgroup\$ – Malachi Aug 12 '15 at 13:41
  • \$\begingroup\$ It could be beneficial to include more of the surrounding code, there are lots of objects being used here that are declared and instantiated outside the provided code. I would suggest including the whole method, including its signature. \$\endgroup\$ – Mathieu Guindon Aug 12 '15 at 13:48
  • 3
    \$\begingroup\$ Exceptions are (relatively) slow. Use TryParse instead of Parse and catching an exception. \$\endgroup\$ – RobH Aug 12 '15 at 13:49
  • 1
    \$\begingroup\$ Just a small stylistic note, catching all exceptions is generally considered bad practice because it can conceal bugs you never knew you had within your code. If you decide to keep the exception catching as is rather than going with what @RobH suggested, try catching something a bit more specific than a general exception. \$\endgroup\$ – llee94 Aug 12 '15 at 13:57
  • \$\begingroup\$ I work with process data too. Anytime someone says there are 4 types of alarms and I see HiHi, I immediately think of {HiHi, Hi, Lo, LoLo} for one tag at a time. Is this what you are trying to do with your code? You have HiHi 3 times and LoLo once, so I really can't tell what you want to do. Plus your stopwatch would also be measuring time to read and dismiss the MessageBox. \$\endgroup\$ – Rick Davin Sep 5 '15 at 12:09
2
\$\begingroup\$

Here're my suggestions to your code:

  • Avoid repetitions. If you access the same index several times it's better to create a helper variable.
  • Name the indexes to give them a meaning so that you (or someonelse) know what column it actualy is. You may know that now but in two months you'll be debugging your own code.
  • Usually it's better to use a string.Format that lengthy string concatenations.

(see c# comments):

if (connection1.State != ConnectionState.Open)
{
    connection1.Open();
}

for (int i = 0; i < alarmSet.Count; i++)
{
    // don't write the same code everywhere, use a helper 
    var currentRow = ds_alarms.Tables[0].Rows[i];

    Stopwatch st = Stopwatch.StartNew();

    // don't just use indexes, name your columns/properties accordingly
    var column0 = currentRow.ItemArray[0];
    int index = listOfTags.FindIndex(x => x.Equals(column0.ToString().Trim()));

    var someOtherColumn = currentRow.ItemArray[5];
    bool isAlarm = bool.Parse(someOtherColumn.ToString()) == true;
    bool isAlarmValue;

    var column1 = currentRow.ItemArray[1];
    float column1Value = 0.0;
    if (!float.TryParse(column1.ToString(), out column1Value))
    {
        isAlarmValue = false;
    }                

    if (isAlarm && isAlarmValue)
    {                    
        // use string.Format instead of that terrible lengthy concatenation... add other columns as shown
        sql_Al.CommandText =
            string.Format(
                "INSERT INTO [MCRCDA].[dbo].[Alarms]([DateTime],[TagName],[AlarmType],[Value],[Units],[Address]) VALUES({0}, {1})",
                time.ToString(format),
                column0.ToString().Trim());
        try
        {

            //sql_Al.ExecuteNonQuery();

        }
        catch (Exception exp)
        {
            MessageBox.Show(exp.Message);
        }
    }

    // the same applies to the rest of your code... 
    // always use meaningfull names so that you still understand your code in two months 
}
\$\endgroup\$
1
\$\begingroup\$

Sql Injection Vunerablility

sql_Al.CommandText = "INSERT INTO [MCRCDA].[dbo].[Alarms]([DateTime],[TagName],[AlarmType],[Value],[Units],[Address])    VALUES(" + time.ToString(format) + ", '" + ds_alarms.Tables[0].Rows[i].ItemArray[0].ToString().Trim() + "','HiHi'," + engValues[index] + ",'" + ds_alarms.Tables[0].Rows[i].ItemArray[9].ToString().Trim() + "','" + ds_alarms.Tables[0].Rows[i].ItemArray[10].ToString().Trim() + "')";

Don't do this. Just... don't. You're taking values from a table and injecting them into a query as a plain string. I'm reminded of this.

xkcd - Exploits of a Mom

Courtesy xkcd.com

Very bad things can happen when you concatenate unknown string values together to create a SQL query. You should be using Command and Parameters instead.

If you don't know what sql injection is, I recommend watching this Computerphile video.

\$\endgroup\$

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.