Usings in C#

When starting a new job in a position of leadership, you often will inherit less-than-optimal code. Perhaps some of this code is “legacy” or had been written unchecked by previous developers, and had made its way to production. Depending on the competence of your predecessor, some of this code may be less than your standard.

In my experience, I discovered a lot of this sort of code. The code-base I inherited came from a small team of inexperienced developers with no formal engineering education, but had successfully gotten the startup off the ground and running. The code was not optimal nor written well, but it got the job done. But now I was tasked with growing the building a proper engineering team, complete with version control with a scalable management style (I adopted agile).

One offense I discovered early on was in a collection of poorly written webservices. This was written in Microsoft webserves 2,0 (SOAP). It had been deprecated since (2006?), and was written in .vb scripts (not even compiled). The code was full of SQL injection vulnerabilities along with a lot of copy-pasted code, with poor naming conventions. Much of this had to change, organically.

One easy but large, low-hanging fruit type error was the offense of leaving connections to the database open. This would routinely fill the app pool and degrade the user experience, as well as even kill the IIS server and kill the hosting server! It was unacceptable to have a user, on an average internet connection, have their site take more than 10 seconds to load.

Imagine opening up the code and seeing a function as this:

  [WebMethod]
    public DataTable GetData(int Id, string entityName)
    {

        SqlConnection connection = ConnectDB.connectToDB(Id.ToString());
        
        string sql = @"SELECT * FROM MyTable where name =" + entityName + ";

        SqlCommand cmd = new SqlCommand(sql, connection);

        try
        {
            connection.Open();
            SqlDataAdapter adapter = new SqlDataAdapter(cmd);
            DataTable table = new DataTable("Data");
            adapter.Fill(table);
            return table;
        }
        catch
        {
            return new DataTable("NoData");
        }
        finally
        {
            if (connection != null)
            {
                connection.Close();
            }
        }
    }

There is a lot to unpack here. A lot.

First and foremost, we cannot move forward without recognizing the blatant SQL injection vulnerability. The not-so-obfuscated variable name “entityName” is concatenated onto the string that is the sql command. This should instead be written with a parameter and added later to the sql command.

Secondly, the use of the try/catch/finally is not how I would recommend. I try and avoid try/catches as I find them to be somewhat poor performance-wise.

Lastly, the lack of using IDisposible where available will definitely impact user experience; not only will it leave open stale connections and occupy the IIS app pool, but it could potentially “take down” a server once IIS begins refusing connections to the database. One would have to tweak IIS to continually refresh the app pool, which is not recommended. It is also not scalable — are you really going to refresh the app pool on potentially every page reload? I would hope not!

This pattern was ubiquitous throughout the entire set of services I inherited. The services consisted of approximately 20 separate .asmx services with some of those being thousands of lines of code. On top of that, code in general was not very well organized and had a lot of copy/pasted code (i.e. was not written in an object oriented way — in fact, not one model or class existed upon inheritance).

As part of an Epic, I set out to refactor these main issues out of this set of services:

1. Use IDisposable where applicable – This meant placing using blocks on SqlConnection, SqlCommand, SqlDataAdapter, DataTable, and readers and writers, to name the most common.
2. Get rid of SQL injection vulnerabilities – This means to ensure we add the variables we want to use as parameters to the SQL Command.
3. Do not concatenate immutable objects – Strings are immutable! It irks me when they are being continually concatenated as it forces me to sit there and imagine each and every character being re-written to memory on every concatenation. Ugh! Use a StringBuilder, please!
4. Remove try/catch/finally – the usings will get rid of the finally by closing the connection automatically, and instead of falling into a “catch”, a proper exception should be logged (and thrown if in debug mode).
5. Reorganize copy/pasted code – This means creating static classes where applicable, or creating objects where necessary.
6. Break apart huge files, and organize according to function – I like to have all CRUD operations for a particular entity together in the file, in the order READ, CREATE, UPDATE, DELETE, and then ordered alphabetically according to the entity upon which it operates. If a file is excessively long, like over 1000 lines, when it needs to be broken apart. For example, a service can (and should!) share the same namespace, but its larger functions can be broken apart into different files. The more files the better, in my opinion, as they are smaller, more modular, and more easy to work with.

Thus, I re-wrote the set of services within a two-week sprint, along with all appropriate unit tests to ensure I did not break core functionality. Additionally, I created a manual for the team to follow with respect to coding guidelines, so such a mess would be mitigated in the future.

Publishing of these services showed a fantastic uptick in performance. With the old configuration, our infrastructure set-up failed at around 600 simultaneous users, with very poor 1990-ish performance until then. The new updates allowed us over 1000 users with no degradation in performance (with default IIS settings) — we hit the simultaneous user-ceiling of our test! Huzzah! It was a breath of fresh air since the band-aide rolled out years prior was to configure IIS to refresh after every 200 connections.

A re-written function above would look like this:

[WebMethod]
    public DataTable GetData(int Id, string name)
    {
        var sql = @"SELECT * FROM MyTable where name = @entityName";
        using (var connection = ConnectDB.connectToDB(Id.ToString()))
        {
            using(var cmd = new SqlCommand(sql, connection))
            {
                cmd.Parameters.AddWithValue("@entityName", name);
            
                try
                {
                    connection.Open();
            
                    using(var adapter = new SqlDataAdapter(cmd))
                    {
                        using(var table = new DataTable("Data"))
                        {
                        adapter.Fill(table);
                        return table;
                        }
                    }
                }
                catch (Exception ex)
                {
                    //or log this somewhere
                    Console.WriteLine(ex.Message);
                }
            } 
        }
    }