7
\$\begingroup\$

Basically my code waits for database table a to have an XML string inserted into it. The SQL query parses the XML string and stores it into separate columns (this has to occur this way because the insert is actually happening from elsewhere, I just rewrote if for testing). The service broker listens to database a and when it sees a new record inserted, it creates a new XML string to send out. I am not adept at threading and this is my very first time ever using the service broker in SQL.

Are there any tips/suggestions that could be offered up? Are there any places which code is going to, for lack of better terms, explode and fail? I just want to make sure this is at least an acceptable way of completing the operations.

This code works perfectly, but does it make sense and can it be optimized?

Interface

enter image description here

Code

#region Declarations
private bool isPermitted()
{
    try
    {
        SqlClientPermission clientPermission = new SqlClientPermission(PermissionState.Unrestricted);
        clientPermission.Demand();
        return true;
    }
    catch
    {
        return false;
        lbxSystemAvailabilityReply.Items.Add("Failed permissions test.");
    }
}
string db = Connect.GetDBString("DatabaseName");
static int RecordsProcessed = 0;
static decimal MaxRecordsRetained = 10;
const string SYSTEM_AVAILABILITY_QUERY = "SELECT [Columns] FROM [dbo].[System];";
const string SYSTEM_AVAILABILITY_REPLY_QUERY = "SELECT [Columns] FROM [dbo].[SystemReply];";
const string SYSTEM_AVAILABILITY_AWAIT = "SELECT [Columns] FROM [dbo].[System] WHERE [Columns] = 0;";
#endregion

public SystemAvailabilityLog()
{
    InitializeComponent();
}
private void dbTransactionLog_Load(object sender, System.EventArgs e)
{
    SqlDependency.Stop(db);
    SqlDependency.Start(db);

    Thread sysAvail = new Thread(SystemAvailabilityThread);
    Thread sysAvailRep = new Thread(SystemAvailabilityReplyThread);
 
    sysAvail.Start();
    sysAvailRep.Start();
    txtLastRestart.Text = DateTime.Now.ToString();
}
private void dbTransactionLog_FormClosing(object sender, FormClosingEventArgs e)
{
    SqlDependency.Stop(db);
}

#region Methods and Functions
private SqlCommand CreateCommandWithDependency(string queryText, SqlConnection con, OnChangeEventHandler e)
{
    using (SqlCommand cmd = con.CreateCommand())
    {
        cmd.CommandText = queryText;
        cmd.CommandType = CommandType.Text;
        cmd.Notification = null;

        SqlDependency sqlDep = new SqlDependency(cmd);
        sqlDep.OnChange += new OnChangeEventHandler(e);

        return cmd;
    }
}
private void UpdateRecordCount()
{
    RecordsProcessed++;
    if (txtProcessed.InvokeRequired)
    {
        txtProcessed.Invoke(
          new ThreadStart(delegate
          {
              txtProcessed.Text = RecordsProcessed.ToString();
          }));
    }
    else
    {
        txtProcessed.Text = RecordsProcessed.ToString();
    }
}
private void SendSystemAvailabilityPing()
{
    using (SqlConnection con = new SqlConnection(db))
    {
        SystemAvailability oPing = new SystemAvailability();
        oPing.PingDate = DateTime.Today.Date.ToString("MMddyyyy");
        oPing.PingTime = DateTime.Now.TimeOfDay.ToString().Replace(":", "").Remove(6);
        string xmlSend = oPing.GetXMLString;
        using (SqlDataReader dr = Connect.ExecuteReader("[db_InsertSystemAvailability]", con,
                                                           new SqlParameter("@XmlSend", xmlSend))) { }
    }
}
private void SystemAvailabilityReply()
{
    try
    {
        if (isPermitted())
        {
            using (SqlConnection con = new SqlConnection(db))
            {
                using (SqlCommand cmd = CreateCommandWithDependency(SYSTEM_AVAILABILITY_AWAIT, con, SystemAvailabilityReply_OnChange))
                {
                    con.Open();
                    using (SqlDataReader dr = cmd.ExecuteReader())
                    {
                        if (dr.HasRows)
                        {
                            while (dr.Read())
                            {
                                if (lbxSystemAvailabilityReply.Items.Count >= MaxRecordsRetained)
                                {
                                    lbxSystemAvailabilityReply.Items.RemoveAt(0);
                                }

                                SystemAvailabilityReply sarPing = new SystemAvailabilityReply();
                                sarPing.PingID = dr["PingID"].ToString();

                                using (SqlConnection conInsert = new SqlConnection(db))
                                {
                                    using (SqlDataReader drInsert = Connect.ExecuteReader("[db_InsertSystemAvailabilityReply]", conInsert,
                                                                                             new SqlParameter("@Param1", sarPing.PingID)))
                                    { }
                                }
                                string transactionDetail = string.Format("PingID: {0} PingDate: {1} PingTime: {2}",
                                                                          sarPing.PingID, sarPing.PingDate, sarPing.PingTime);
                                lbxSystemAvailabilityReply.Items.Add(transactionDetail);
                                UpdateRecordCount();
                            }
                        }
                    }
                }
            }
        }
    }
    catch (Exception ex)
    {

    }
}
private void SystemAvailability()
{
    lbxSystemAvailability.Items.Clear();
    if (isPermitted())
    {
        string pingDate = DateTime.Today.ToShortDateString();
        string pingTime = DateTime.Now.TimeOfDay.ToString();

        string pingMessage = string.Format("Ping Received: {0} @ {1}", pingDate, pingTime);
        lbxSystemAvailability.Items.Add(pingMessage);
    }
}
private void SystemAvailabilityThread()
{
    using (SqlConnection con = new SqlConnection(db))
    {
        con.Open();
        using (SqlCommand cmd = CreateCommandWithDependency(SYSTEM_AVAILABILITY_QUERY, con, SystemAvailability_OnChange))
        {
            using (SqlDataReader dr = cmd.ExecuteReader()) { }
        }
    }
}
private void SystemAvailabilityReplyThread()
{
    using (SqlConnection con = new SqlConnection(db))
    {
        con.Open();
        using (SqlCommand cmd = CreateCommandWithDependency(SYSTEM_AVAILABILITY_REPLY_QUERY, con, SystemAvailabilityReply_OnChange))
        {
            using (SqlDataReader dr = cmd.ExecuteReader()) { }
        }
    }
}
#endregion

#region Object Handles
private void btnSystemAvailabilityForm_Click(object sender, EventArgs e)
{
    Thread sysPing = new Thread(SendSystemAvailabilityPing);
    sysPing.Start();
}
private void nudRecentTransactionCount_ValueChanged(object sender, EventArgs e)
{
    MaxRecordsRetained = nudRecentTransactionCount.Value;
    while (lbxSystemAvailabilityReply.Items.Count > MaxRecordsRetained)
    {
        lbxSystemAvailabilityReply.Items.RemoveAt(0);
    }
}
private void SystemAvailability_OnChange(object sender, SqlNotificationEventArgs e)
{
    if (this.InvokeRequired)
    {
        lbxSystemAvailabilityReply.BeginInvoke(new MethodInvoker(SystemAvailabilityReply));
    }
    else
    {
        SystemAvailabilityReply();
    }

    Thread sysAvail = new Thread(SystemAvailabilityThread);
    sysAvail.Start();
}
private void SystemAvailabilityReply_OnChange(object sender, SqlNotificationEventArgs e)
{
    if (this.InvokeRequired)
    {
        lbxSystemAvailability.BeginInvoke(new MethodInvoker(SystemAvailability));
    }
    else
    {
        SystemAvailability();
    }

    Thread sysAvailRep = new Thread(SystemAvailabilityReplyThread);
    sysAvailRep.Start();
}
#endregion
\$\endgroup\$
1

2 Answers 2

5
\$\begingroup\$

I see a couple of things in this piece of your code that I would like to address

private void SystemAvailabilityReply()
{
    try
    {
        if (isPermitted())
        {
            using (SqlConnection con = new SqlConnection(db))
            {
                using (SqlCommand cmd = CreateCommandWithDependency(SYSTEM_AVAILABILITY_AWAIT, con, SystemAvailabilityReply_OnChange))
                {
                    con.Open();
                    using (SqlDataReader dr = cmd.ExecuteReader())
                    {
                        if (dr.HasRows)
                        {
                            while (dr.Read())
                            {
                                if (lbxSystemAvailabilityReply.Items.Count >= MaxRecordsRetained)
                                {
                                    lbxSystemAvailabilityReply.Items.RemoveAt(0);
                                }

                                SystemAvailabilityReply sarPing = new SystemAvailabilityReply();
                                sarPing.PingID = dr["PingID"].ToString();

                                using (SqlConnection conInsert = new SqlConnection(db))
                                {
                                    using (SqlDataReader drInsert = Connect.ExecuteReader("[db_InsertSystemAvailabilityReply]", conInsert,
                                                                                            new SqlParameter("@Param1", sarPing.PingID)))
                                    { }
                                }
                                string transactionDetail = string.Format("PingID: {0} PingDate: {1} PingTime: {2}",
                                                                        sarPing.PingID, sarPing.PingDate, sarPing.PingTime);
                                lbxSystemAvailabilityReply.Items.Add(transactionDetail);
                                UpdateRecordCount();
                            }
                        }
                    }
                }
            }
        }
    }
    catch (Exception ex)
    {

    }
}
  1. There is a lot of nesting here, let's see what we can do about that.

    using (SqlConnection con = new SqlConnection(db))
    {
        using (SqlCommand cmd = CreateCommandWithDependency(SYSTEM_AVAILABILITY_AWAIT, con, SystemAvailabilityReply_OnChange))
        {
    

    Will become this

    using (SqlConnection con = new SqlConnection(db))
    using (SqlCommand cmd = CreateCommandWithDependency(SYSTEM_AVAILABILITY_AWAIT, con, SystemAvailabilityReply_OnChange))
    {
    
  2. I am pretty sure that while (dr.Read()) won't read rows that don't exist in other words dr.Read() should be false if there are no rows to read, and that makes the check for rows redundant. So this:

    if (dr.HasRows)
    {
        while (dr.Read())
        {
    

    Becomes this

    while (dr.Read())
    {
    
  3. Then there are a couple more Using statements nested deep in there that we can semi-combine without fancy Curly Braces

    using (SqlConnection conInsert = new SqlConnection(db))
    using (SqlDataReader drInsert = Connect.ExecuteReader("[db_InsertSystemAvailabilityReply]", conInsert,
        new SqlParameter("@Param1", sarPing.PingID)))
    { }
    
  4. Empty Catch -> catch everything and hide it. This smells. I would get rid of it, but I don't know what you are doing with it either.


Here is what I came up with (Without the Try/Catch)

private void SystemAvailabilityReply()
{
    if (isPermitted())
    {
        using (SqlConnection con = new SqlConnection(db))
        using (SqlCommand cmd = CreateCommandWithDependency(SYSTEM_AVAILABILITY_AWAIT, con, SystemAvailabilityReply_OnChange))
        {
            con.Open();
            using (SqlDataReader dr = cmd.ExecuteReader())
            {
                while (dr.Read())
                {
                    if (lbxSystemAvailabilityReply.Items.Count >= MaxRecordsRetained)
                    {
                        lbxSystemAvailabilityReply.Items.RemoveAt(0);
                    }

                    SystemAvailabilityReply sarPing = new SystemAvailabilityReply();
                    sarPing.PingID = dr["PingID"].ToString();

                    using (SqlConnection conInsert = new SqlConnection(db))
                    using (SqlDataReader drInsert = Connect.ExecuteReader("[db_InsertSystemAvailabilityReply]", conInsert,
                                                                            new SqlParameter("@Param1", sarPing.PingID)))
                    { }

                    string transactionDetail = string.Format("PingID: {0} PingDate: {1} PingTime: {2}",
                                                            sarPing.PingID, sarPing.PingDate, sarPing.PingTime);
                    lbxSystemAvailabilityReply.Items.Add(transactionDetail);
                    UpdateRecordCount();
                }
            }
        }
    }
}

This Method also looks a little clunky

private void SystemAvailabilityThread()
{
    using (SqlConnection con = new SqlConnection(db))
    {
        con.Open();
        using (SqlCommand cmd = CreateCommandWithDependency(SYSTEM_AVAILABILITY_QUERY, con, SystemAvailability_OnChange))
        {
            using (SqlDataReader dr = cmd.ExecuteReader()) { }
        }
    }
}   

I would move the con.Open(); closer to where it is actually going to be used, which is in the Execute command and then make the using statements a little cleaner

private void SystemAvailabilityThread()
{
    using (SqlConnection con = new SqlConnection(db))
    using (SqlCommand cmd = CreateCommandWithDependency(SYSTEM_AVAILABILITY_QUERY, con, SystemAvailability_OnChange))
    {
        con.Open();
        using (SqlDataReader dr = cmd.ExecuteReader()) { }
    }
}

I lost my place, I think you get the point of what I am doing with your using statements though. I will try to take another look later as well.

\$\endgroup\$
7
  • 1
    \$\begingroup\$ I appreciate your suggestions, first thing though, the catch is not going to be empty, I just haven't written the procedure! Don't worry, it won't emit odor when complete! :-) \$\endgroup\$
    – Volearix
    Commented Sep 18, 2014 at 18:06
  • \$\begingroup\$ Second, the {} is required for good practice here. Just following rule books and using what is requested! \$\endgroup\$
    – Volearix
    Commented Sep 18, 2014 at 18:07
  • \$\begingroup\$ I didn't make a big fuss in the answer about it because I figured it was something that was in the works. \$\endgroup\$
    – Malachi
    Commented Sep 18, 2014 at 18:08
  • \$\begingroup\$ what rule books are you following for good practice? \$\endgroup\$
    – Malachi
    Commented Sep 18, 2014 at 18:08
  • \$\begingroup\$ Thirdly, the if (dr.HasRows) is also a part of good practice. Not sure of the logistics behind it, if it is written as wrong somewhere I'd be more than happy to alter it though. \$\endgroup\$
    – Volearix
    Commented Sep 18, 2014 at 18:08
4
\$\begingroup\$

@Malachi has pointed out a number of things I agree with. The level of nesting in that one procedure is called 'arrow-coding'.

The chained using blocks is common practice: using statement with multiple variables

There are two other things though that you can also do:

  1. reverse the logic of the isPermitted() call to be:

    if (!isPermitted())
    {
        return;
    }
    
  2. For nesting, and performance reasons, create just one insert connection for all the inserts you do. Bring the insert connection 'up' the stack to outside the loops:

    using (SqlConnection con = new SqlConnection(db))
    using (SqlConnection conInsert = new SqlConnection(db))
    using (SqlCommand cmd = CreateCommandWithDependency(SYSTEM_AVAILABILITY_QUERY, con, SystemAvailability_OnChange))
    {
        ....
    }
    

    This has the added benefit that the code only connects once for each insert, instead of once per insert. The connection is typically a slow process, so reducing the number of connect/disconnect instances is a good thing.

    The possible down-side here is if you typically insert nothing, but you will need to make the decision of whether the insert-nothing case outweighs the inserts-many-times.

One small style consistency problem you have is the empty-brace statement you have for the using on the executes. Sometimes you have:

 using (SqlDataReader dr = cmd.ExecuteReader()) { }

other times you have

 using (SqlDataReader drInsert = Connect.ExecuteReader("[db_InsertSystemAvailabilityReply]", conInsert,
         new SqlParameter("@Param1", sarPing.PingID)))
 { }
\$\endgroup\$
6
  • \$\begingroup\$ Why is !isPermitted better? I have updated my code to reflect the using (SqlConnection... portion and the consistency issue should be resolved later after the Connect. method includes Connect.NonQueryWithDependency method. \$\endgroup\$
    – Volearix
    Commented Sep 19, 2014 at 12:58
  • \$\begingroup\$ The !isPermitted() is better because it removes a lot of code from an otherwise deeply nested block. \$\endgroup\$
    – rolfl
    Commented Sep 19, 2014 at 13:00
  • \$\begingroup\$ I'm just asking to understand, but this code is not executed either way, correct? Is this from a readability standpoint or is there an actual performance/other benefit? \$\endgroup\$
    – Volearix
    Commented Sep 19, 2014 at 13:09
  • \$\begingroup\$ The only benefit is readability. There's no performance benefit. See this post: programmers.stackexchange.com/questions/18454/… \$\endgroup\$
    – rolfl
    Commented Sep 19, 2014 at 13:15
  • 2
    \$\begingroup\$ Guess you're right. Went ahead and modified it, makes less braces, not easy to miss. I'm still in an unfortunate mindset of 'less code is better'... I always try to condense into the least amount of lines as possible. Been trying to remedy this habit as I know readability tends to suffer. \$\endgroup\$
    – Volearix
    Commented Sep 19, 2014 at 13:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.