Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

@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 variablesusing 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)))
 { }

@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)))
 { }

@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)))
 { }
Source Link
rolfl
  • 97.5k
  • 17
  • 218
  • 418

@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)))
 { }