Does my code close connection to SQL Server

Question

So method GetAllData is one of my universal methods for connecting to the database and here, since I have to add mupltiple records in once to the database I need to have one connetion with multiple commands to run. So my question is does this way of connection closes the database correctly, or if you have any improvments to my code please share with me.

       var conn = new SqlConnection(ConnString);
        conn.Open();
        var data = new Dictionary<string, List<object>>();
        foreach (var h in hours)
        {
            data += SqlUniversal.GetAllData(query,//idk how I will collect the data yet... i know += wouldnt work for dictionary
                new[] {
                    //some parameters
                },
                conn);
        }
//Here is the method, above is how I call it.
        public static Dictionary<string, List<object>> GetAllData(string command, SqlParameter[] pars, SqlConnection conn)
        {
        if (conn == null)
        {
            conn = new SqlConnection(ConnString);
            conn.Open();
        }
        var res = new Dictionary<string, List<object>>();
        using (conn)
        {
            using (var cmd = new SqlCommand(command, conn))
            {
                cmd.Parameters.AddRange(pars);
                using (var reader = cmd.ExecuteReader())
                {
                    while (reader.Read())
                        for (var i = 0; i < reader.VisibleFieldCount; i++)
                        {
                            if (reader.GetValue(i) == DBNull.Value)
                                continue;

                            var name = reader.GetName(i);

                            if (res.ContainsKey(name))
                                res[name].Add(reader.GetValue(i));

                            else
                                res.Add(name, new List<object> {reader.GetValue(i)});
                        }

                    return res;
                }
            }
        }
    }

In short no.. it could remain open for a while until the garbage collector finishes it off — BugFinder, Apr 24 '18 at 14:35
Yes, it does close it (eventually), but you should consider letting the method own the connection, you create it if it is `null` in the beginning, so why open it in the calling method? After this is called, it is closed the first time anyway. Since you don't re-open the connection, I can see this being a problem. Just let the method own it and create it. — Ron Beyer, Apr 24 '18 at 14:37
But I would eventually need to open and close it 200 times @RonBeyer — Daniel Suchan, Apr 24 '18 at 14:39
Lots of confusing information here for you from the commenters so far. Your connection won't be closed immediately. It will end up in the connection pool and eventually it will get closed (through `Using`), or reused if you call the same connection. In fact, I would say the current answer is patently incorrect. — Jacob H, Apr 24 '18 at 14:39
@DanielSuchan That is why there is a connection pool, it isn't creating 200 connections, it will reuse them. — Ron Beyer, Apr 24 '18 at 14:40
The *using statement* ensures that Dispose is called even if an exception occurs while you are calling methods on the object : https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/using-statement — Mate, Apr 24 '18 at 14:42
The answer is everyone is right about some portion of the code, the first code doesnt have a using, has an open and no close this will stay around like a bad smell. Your method, however does have a using and so should close it. So if you called this pair in quick succession you would see a rise in open connections — BugFinder, Apr 24 '18 at 14:43
Possible duplicate of [SqlConnection.Close() inside using statement](https://stackoverflow.com/questions/18588049/sqlconnection-close-inside-using-statement) — Ricardo Pontual, Apr 24 '18 at 14:47
Why are you creating a connection in your code, then passing that connection to a method and then creating a new connection? Why not stop passing the connection around and just create it in your method? The way you are dealing with connections is really brittle. — Sean Lange, Apr 24 '18 at 15:36
I would do the `return` after your `using` blocks are closed. I do see a problem in that a SQL Connection in any state besides _open_ can be passed in and that connection would not be _opened_ — Mad Myche, Apr 24 '18 at 16:23

score 3 · Answer 1 · answered Apr 24 '18 at 14:46

Probably the better way to do this is to let the overseeing method manage the connection:

public void SomeDataMethod()
{
    using (var conn = new SqlConnection(ConnString))
    {
        conn.Open();
        var data = new Dictionary<string, List<object>>();
        foreach (var h in hours)
        {
            data += SqlUniversal.GetAllData(query,
                new[] {
                    //some parameters
                },
                conn);
        }
    }
}

And remove the using (conn) from the GetData method:

public static Dictionary<string, List<object>> GetAllData(string command, SqlParameter[] pars, SqlConnection conn)
{
    var res = new Dictionary<string, List<object>>();
    using (var cmd = new SqlCommand(command, conn))
    {
        cmd.Parameters.AddRange(pars);
        using (var reader = cmd.ExecuteReader())
        {
          //...
          return res;
        }
    }
}

Now SomeDataMethod controls the connection lifetime and GetData doesn't worry about connection management.

This is good, but in some cases in my code I'm executing just one query so in main method I don't want to control the connetion, but I'll think about this — Daniel Suchan, Apr 24 '18 at 14:50
@DanielSuchan Really it is "dealers choice", but only one method should control/create the connection, not both. If you want to move the connection handling into the `GetData` method, by all means do it, but don't pass in a connection as a parameter. — Ron Beyer, Apr 24 '18 at 14:52
But since I now have idea how conn pool works I'll probably use your solution. Thanks. — Daniel Suchan, Apr 24 '18 at 14:53

Does my code close connection to SQL Server

1 Answers1