Performing and timing loops with SQL queries

Question

I am a casual C# programmer, not formally trained in OOP at all; mostly I focus on Transact-SQL in SQL Server, and so when I write C# apps I get grief about the constructs and methods I use. Here is a simple app I wrote for this blog post - it was not meant to be a production app, and performance is certainly not a priority, but I would like to become better at writing optimal code naturally.

The purpose of the code is quite simple - for 100,000 cycles I need to generate a random name and number from a SQL Server stored procedure, then write those values to a table . After that, read 100 random rows from the table, 1,000 times, and not really care what is done as you iterate through the 100 rows.

The point of the exercise was to profile the effects of encryption on the application. So what the reader loop does is not really material and does not need to be optimized, it is just there to take up time and be the same in both cases. Also it should be noted that the multiple connections are intentional - it is supposed to simulate - without getting into multi-threading or multiple instances of the app - all of the overhead you would see in a highly concurrent app (even though, deep down, this still executes serially).

Items that were pointed out to me (and that I'd like to better understand):

• for is better than while
• Variable declaration at the top is bad
• Code is bloated
• Legibility is bad

I am also sure there is a better way to time the performance of the code, other than dumping the current clock time to the console.

using System;
using System.Collections.Generic;
using System.Text;
using System.Configuration;
using System.Data;
using System.Data.SqlClient;

namespace AEDemo
{
  class Program
  {
    static void Main(string[] args)
    {
      using (SqlConnection con1 = new SqlConnection())
      {
        Console.WriteLine(DateTime.UtcNow.ToString("hh:mm:ss.fffffff"));
        string name;
        string EmptyString = "";
        string conString = ConfigurationManager.ConnectionStrings[args[0]].ToString();
        int salary;
        int i = 1;
        while (i <= 100000)
        {
          con1.ConnectionString = conString;
          using (SqlCommand cmd1 = new SqlCommand("dbo.GenerateNameAndSalary", con1))
          {
            cmd1.CommandType = CommandType.StoredProcedure;
            SqlParameter n = new SqlParameter("@Name", SqlDbType.NVarChar, 32) 
                             { Direction = ParameterDirection.Output };
            SqlParameter s = new SqlParameter("@Salary", SqlDbType.Int) 
                             { Direction = ParameterDirection.Output };
            cmd1.Parameters.Add(n);
            cmd1.Parameters.Add(s);
            con1.Open();
            cmd1.ExecuteNonQuery();
            name = n.Value.ToString();
            salary = Convert.ToInt32(s.Value);
            con1.Close();
          }

          using (SqlConnection con2 = new SqlConnection())
          {
            con2.ConnectionString = conString;
            using (SqlCommand cmd2 = new SqlCommand("dbo.AddPerson", con2))
            {
              cmd2.CommandType = CommandType.StoredProcedure;
              SqlParameter n = new SqlParameter("@LastName", SqlDbType.NVarChar, 32);
              SqlParameter s = new SqlParameter("@Salary", SqlDbType.Int);
              n.Value = name;
              s.Value = salary;
              cmd2.Parameters.Add(n);
              cmd2.Parameters.Add(s);
              con2.Open();
              cmd2.ExecuteNonQuery();
              con2.Close();
            }
          }
          i++;
        }
        Console.WriteLine(DateTime.UtcNow.ToString("hh:mm:ss.fffffff"));
        i = 1;
        while (i <= 1000)
        {
          using (SqlConnection con3 = new SqlConnection())
          {
            con3.ConnectionString = conString;
            using (SqlCommand cmd3 = new SqlCommand("dbo.RetrievePeople", con3))
            {
              cmd3.CommandType = CommandType.StoredProcedure;
              con3.Open();
              SqlDataReader rdr = cmd3.ExecuteReader();
              while (rdr.Read())
              {
                EmptyString += rdr[0].ToString();
              }
              con3.Close();
            }
          }
          i++;
        }
        Console.WriteLine(DateTime.UtcNow.ToString("hh:mm:ss.fffffff"));
      }
    }
  }
}

Answer 1

In the case of both if your while loops, you could easily make them for loops instead, which tends to be the best practice.

    int i = 1;
    while (i <= 100000)
    {
        // main while code
        i++
    }

Should be rewritten as:

for (int i = 1; i <= 100000; i++)
{
    // main while code
}

Likewise:

    i = 1;
    while (i <= 1000)
    {
        // main while code
        i++;
    }

Would become:

for (int i = 1; i <= 1000; i++)
{
    // main while code
}

This allows you to recycle the variable i later, and also prevents from accidentally forgetting to reset i back to 1 between loops. Generally, the variable i stands for iterator, and is typically reserved exclusively for loops.

This has no real performance impact, but it has a severe readability and maintainability impact. Using the for loop, it is immediately clear what is happening to the iterator. Using the while loop, you have to search through the body of the while to find where you manipulate the iterator.

---

As far as:

variable declaration at the top is bad

Generally it is frowned upon. You should declare variables as close to their usage as possible, and within the tightest block that they are to be used in.

For example, the:

   int salary;
   string name;

Should be declared within your first loop (which should now be a for loop):

for (int i = 1; i <= 100000; i++)
{
    int salary;
    string name;
}

This is so that variables cannot unexpectedly be used in places where they should not, and so that they can be recycled when they fall out-of-scope immediately. It also means that you cannot accidentally use the value of them outside of the block they apply to.

---

Try to choose more effective variable names. Never abbreviate except for certain well-accepted abbreviates (i for iterator, e for eventArgs, etc.):

  using (SqlConnection con1 = new SqlConnection())
  {

Would be more accepted as:

  using (SqlConnection connection1 = new SqlConnection())
  {

This is mostly a best-practice.

---

You are likely going to see a huge performance drag using the EmptyString variable as you are: string types are immutable, which means each time you do EmptyString += rdr[0].ToString(); it creates a new instance of a string with the combination, then assigns it to EmptyString, then schedules the original string for garbage collection.

Instead, use a StringBuilder:

StringBuilder emptyStringBuilder = new StringBuilder();

for (int i = 1; i <= 1000; i++)
{
    // ...
    while (rdr.Read())
    {
        emptyStringBuilder.Append(rdr[0].ToString());
    }
    // ...
}

This saves a lot of performance. See MSDN for more information.

---

Another variable naming nit-pick: you should always camelCase local and private variable names: i.e. emptyString instead of EmptyString.

This is another best-practice.

---

After all our rewrites:

using System;
using System.Collections.Generic;
using System.Text;
using System.Configuration;
using System.Data;
using System.Data.SqlClient;

namespace AEDemo
{
    class Program
    {
        static void Main(string[] args)
        {
            using (SqlConnection connection1 = new SqlConnection())
            {
                Console.WriteLine(DateTime.UtcNow.ToString("hh:mm:ss.fffffff"));
                string connectionString = ConfigurationManager.ConnectionStrings[args[0]].ToString();

                for (int i = 1; i <= 100000; i++)
                {
                    connection1.ConnectionString = connectionString;

                    string name;
                    int salary;

                    using (SqlCommand command1 = new SqlCommand("dbo.GenerateNameAndSalary", connection1))
                    {
                        command1.CommandType = CommandType.StoredProcedure;
                        SqlParameter nameParameter = new SqlParameter("@Name", SqlDbType.NVarChar, 32) { Direction = ParameterDirection.Output };
                        SqlParameter salaryParameter = new SqlParameter("@Salary", SqlDbType.Int) { Direction = ParameterDirection.Output };

                        command1.Parameters.Add(nameParameter);
                        command1.Parameters.Add(salaryParameter);

                        connection1.Open();
                        command1.ExecuteNonQuery();
                        name = nameParameter.Value.ToString();
                        salary = Convert.ToInt32(salaryParameter.Value);
                        connection1.Close();
                    }

                    using (SqlConnection connection2 = new SqlConnection())
                    {
                        connection2.ConnectionString = connectionString;
                        using (SqlCommand command2 = new SqlCommand("dbo.AddPerson", connection2))
                        {
                            command2.CommandType = CommandType.StoredProcedure;
                            SqlParameter nameParameter = new SqlParameter("@LastName", SqlDbType.NVarChar, 32);
                            SqlParameter salaryParameter = new SqlParameter("@Salary", SqlDbType.Int);
                            nameParameter.Value = name;
                            salaryParameter.Value = salary;

                            command2.Parameters.Add(nameParameter);
                            command2.Parameters.Add(salaryParameter);

                            connection2.Open();
                            command2.ExecuteNonQuery();
                            connection2.Close();
                        }
                    }
                }
                Console.WriteLine(DateTime.UtcNow.ToString("hh:mm:ss.fffffff"));

                // We'll declare the StringBuilder outside the `for` to get maximum performance impact, and demonstrate it's effectiveness
                StringBuilder emptyStringBuilder = new StringBuilder();

                for (int i = 1; i <= 1000; i++)
                {
                    using (SqlConnection connection3 = new SqlConnection())
                    {
                        connection3.ConnectionString = connectionString;
                        using (SqlCommand command3 = new SqlCommand("dbo.RetrievePeople", connection3))
                        {
                            command3.CommandType = CommandType.StoredProcedure;
                            command3.Open();

                            SqlDataReader reader = command3.ExecuteReader();
                            while (reader.Read())
                            {
                                emptyStringBuilder.Append(reader[0].ToString());
                            }
                            connection3.Close();
                        }
                    }
                }
                Console.WriteLine(DateTime.UtcNow.ToString("hh:mm:ss.fffffff"));
            }
        }
    }
}

Disclaimer

I wrote this outside the IDE, there may be minor changes required to make it work.

---

Also I am sure there is a better way to time the performance of the code, other than dumping the current clock time to the console.

Yes, you should look into the Stopwatch class for timing. It's much more accurate than using DateTime strings printed to the Console. It also allows you to easily reset it and such, so you could build a very effective diagnostics procedure that would measure the specific performance impacts that you are trying to.

---

I also recommend you take Malachi's advice on the connection string.

---

And finally, after discussion in the comments, I would recommend you build a class to represent the return of dbo.RetrievePeople. Then, you should create an instance of that class instead of the emptyStringBuilder.Append, and add each instance to a List<T>. This will allow you to performance test with more relation to what would likely be the actual use-case.

---

Glad to see you over here, you're a legend on the DBA site! :)

Answer 2

Why isn't using (SqlConnection con1 = new SqlConnection()) inside your while loop, right before using (SqlCommand cmd1 = new SqlCommand("dbo.GenerateNameAndSalary", con1))?

Right now con1 is instantiated on line 14 and not disposed until line 82, yet it isn't used after line 39. You actually open con2 inside the con1 using block, as well as con3 which is inside while (i <= 1000).

---

This:

SqlParameter n = new SqlParameter("@LastName", SqlDbType.NVarChar, 32);
SqlParameter s = new SqlParameter("@Salary", SqlDbType.Int);
n.Value = name;
s.Value = salary;
cmd2.Parameters.Add(n);
cmd2.Parameters.Add(s);

... can be rewritten as:

cmd2.Parameters.Add("@LastName", SqlDbType.NVarChar, 32).Value = name;
cmd2.Parameters.Add("@Salary", SqlDbType.Int).Value = salary;

---

Why do this:

using (SqlConnection con2 = new SqlConnection())
{
    con2.ConnectionString = conString;

... when SqlConnection has a constructor that accepts the connection string?

---

No need to do con1.Close();, con2.Close(); etc. since the connection is inside a using block.

---

You need to wrap SqlDataReader inside a using block as well.

Answer 3

You should move your connection string outside of the using statement

  using (SqlConnection con1 = new SqlConnection())
  {
    Console.WriteLine(DateTime.UtcNow.ToString("hh:mm:ss.fffffff"));
    string name;
    string EmptyString = "";
    string conString = ConfigurationManager.ConnectionStrings[args[0]].ToString();
    int salary;
    int i = 1;
    while (i <= 100000)
    {
      con1.ConnectionString = conString;

Like this

  string conString = ConfigurationManager.ConnectionStrings[args[0]].ToString();

  using (SqlConnection con1 = new SqlConnection(conString))
  {
    Console.WriteLine(DateTime.UtcNow.ToString("hh:mm:ss.fffffff"));
    string name;
    string EmptyString = "";
    int salary;
    int i = 1;
    while (i <= 100000)
    {

by doing this you eliminate the need to set the connection string in the SqlConnection object later in the code.

You can also leave out the calls to close the connections, this happens automatically when scope leaves the using statements.

---

I would think that the connection to the Database shouldn't have anything to do with the rest of the code. You could actually use the same connection for each and every operation in this application, and it should give you a better idea of how each operation is being handled.

another thing that I would suggest is not calling Console.WriteLine until the end of the method. By that I mean that you should hold the times in variables, declared before all the fun starts, assign the variables where you currently have Console.WriteLine and then when everything is said and done you could then write them out to the Console.

This would take the heavy process of Console.WriteLine out of the equation for figuring the timing of these processes

---

There is also a way that you can use a using statement surrounding the SqlDataReader as well.

You have to create the reader and then put it into a using statement like this

SqlDataReader rdr = cmd3.ExecuteReader();
using (rdr)
{
    while (rdr.Read())
    {
        EmptyString += rdr[0].ToString();
    }
}

it's a little different than a command or a connection but a data reader still implements the IDisposable interface and should be disposed up properly should anything happen while it is being used.

---

As far as the loops go, a for loop is going to look cleaner and be a little easier to maintain should you have to change something in the code later.

• declare incrementation variable in for loop declaration
• handle incrementation in the for loop declaration
• handle how many loops in the for loop declaration

with a while loop you can only do one of these inside the declaration, the rest have to be done elsewhere in the code and could lead to unexpected incrementation or something else changing the incrementation variable.

with a for loop the incrementation variable is scoped to the loop and destroyed when the scope leaves the for loop. In your while loop the incrementation variable is still usable after the while loop is exited, and you don't need to access that value so why do that?

Answer 4

for loops

Rather than using a while loop, and having an iterator variable i, You can use a for loop. A simple for loop is structured something like this:

for(/* Declare `i` */; /* Check the value of `i` against a condition */; /* Change the value of `i` */)
{
    ...
}

For example, this while loop, near the beginning of your code:

int i = 1;
while (i <= 100000)
{
    ...
}

Would become the following for loop

for(int i = 1; i <= 100000; ++i)
{

}

By using a for loop, you can eliminate the need to declare i, and increment it on a separate line.

---

Separation of repeated code

I've noticed that you're repeated this line of code three times:

Console.WriteLine(DateTime.UtcNow.ToString("hh:mm:ss.fffffff"));

While in a small program like this, it isn't such a huge deal, I'd recommend encapsulating this in a helper method, like this:

private void CurrentUtcDate()
{
    Console.WriteLine(DateTime.UtcNow.ToString("hh:mm:ss.fffffff"));
}

You can then add this method to your Program class, and call this.CurrentUtcDate(), or CurrentUtcDate() to achieve the same effect.

---

Nitpicks

I don't see many things about this code that I need to nitpick, but here's a few things that could be changed:

• Add some blank lines in your Main method. This will help improve readability.
• Drop some inline comments // in there as well, explaining what certain blocks of code do, and how they do that.
• Remove the useless using statements at the top of your code, like System.Collections.Generic, and System.Text.