21
\$\begingroup\$

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"));
      }
    }
  }
}
\$\endgroup\$
  • \$\begingroup\$ Out of curiosity, how long does this take to execute, on average? \$\endgroup\$ – 410_Gone Aug 12 '15 at 14:20
  • 2
    \$\begingroup\$ @EBrown Over 5 runs these were the averages: cdn.sqlperformance.com/wp-content/uploads/2015/08/AE-Perf.png \$\endgroup\$ – Aaron Bertrand Aug 12 '15 at 14:22
  • \$\begingroup\$ I guess this title is ok, then? \$\endgroup\$ – Aaron Bertrand Aug 12 '15 at 15:51
  • \$\begingroup\$ lol @AaronBertrand you should discuss that with him in chat. but that title explains very briefly what the code is about. \$\endgroup\$ – Malachi Aug 12 '15 at 16:01
  • \$\begingroup\$ @Malachi So did my original title, I thought. \$\endgroup\$ – Aaron Bertrand Aug 12 '15 at 16:53
30
\$\begingroup\$

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! :)

\$\endgroup\$
  • \$\begingroup\$ Thanks, the performance of the iterator is not what I am looking to optimize, at all. In fact I wrote that intentionally to be slow. \$\endgroup\$ – Aaron Bertrand Aug 12 '15 at 14:00
  • 1
    \$\begingroup\$ The iterator's are not slow, the adding a string to a string part is. Depending on what you are trying to measure (performance wise) it would be best to use a StringBuilder there so you don't get noise from the creation/recycling of excess resources. \$\endgroup\$ – 410_Gone Aug 12 '15 at 14:01
  • \$\begingroup\$ Okay, but I'm trying to make the app do something, even if it's wasteful, like real applications do. \$\endgroup\$ – Aaron Bertrand Aug 12 '15 at 14:04
  • 1
    \$\begingroup\$ @AaronBertrand The question is, are you trying to test how it would perform on the application, or on the SQL server? If you want to stress-test the SQL server, then you would be better off using the StringBuilder as that bit will be significantly faster, and come back to the SQL server for more work almost immediately. (I assume that is your goal?) \$\endgroup\$ – 410_Gone Aug 12 '15 at 14:06
  • 5
    \$\begingroup\$ @AaronBertrand the string concatenation has quadratic performance cost. This is not representative of real applications. I understand your need to cause some processing, but it should be linear in the number of rows. Not quadratic. \$\endgroup\$ – usr Aug 12 '15 at 14:28
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I have no idea how I missed the SqlDataReader not having a using block. Good answer! \$\endgroup\$ – 410_Gone Aug 12 '15 at 14:24
8
\$\begingroup\$

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?

\$\endgroup\$
  • 4
    \$\begingroup\$ Can you explain why that is better? Remember, I want to connect to the database 100,000 times, not once, so that handshake overhead is part of the profile. \$\endgroup\$ – Aaron Bertrand Aug 12 '15 at 13:34
  • 2
    \$\begingroup\$ @AaronBertrand Moving the conString to the constructor of the SqlConnection will not connect to the database. It merely saves you wasting resources on resetting that variable to the exact same thing 100,000 times. \$\endgroup\$ – 410_Gone Aug 12 '15 at 13:37
  • \$\begingroup\$ For the connection strings actually there are different connections used, I just simplified slightly. And for the console.writeline stuff, you can assume that a real application that is interacting with data will actually do stuff. Since the cost of writing a datetime to the console should be the same in all cases, does it really matter if that additional bit of time is always included in all tests or never included in any tests? I'm trying to measure relative performance, not absolute. \$\endgroup\$ – Aaron Bertrand Aug 12 '15 at 13:49
  • \$\begingroup\$ you should post the code that is closest to the actual code as possible, otherwise you will get reviews on stuff that doesn't fit your actual code. \$\endgroup\$ – Malachi Aug 12 '15 at 13:51
  • 1
    \$\begingroup\$ Settle down, I just consolidated two separate conString variables. \$\endgroup\$ – Aaron Bertrand Aug 12 '15 at 14:15
8
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ IIRC if I removed System.Configuration I was not able to pull ConfigurationManager.ConnectionStrings from App.Config. Also is the while -> for a worthwhile change (given that most of my SQL Server-based readers have much more familiarity with while than for), or more of a micro-optimization? \$\endgroup\$ – Aaron Bertrand Aug 12 '15 at 13:44
  • \$\begingroup\$ CurrentUtcDate() isn't very descriptive, and has unexpected side effects. I would go with something like PrintCurrentUtcDate() or PrintDate(DateTime date) and call PrintDate(DateTime.UtcNow). \$\endgroup\$ – Dorus Aug 13 '15 at 12:36

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.