views:

152

answers:

2

This thread is a continuation of http://stackoverflow.com/questions/2220422/is-there-a-reason-to-check-for-null-inside-multiple-using-clausule-in-c

I've noticed that resharper lets me define using without opening any opening/closing bracket like in the method below (but then i can't use defined vars later on if the brackets ain't there, other then the one that is used exactly beneath defined using):

public static string sqlGetDatabaseRows() {
        string varRows = "";
        const string preparedCommand = @"
                    SELECT SUM(row_count) AS 'Rows'
                    FROM sys.dm_db_partition_stats
                    WHERE index_id IN (0,1)
                    AND OBJECTPROPERTY([object_id], 'IsMsShipped') = 0;";
        using (var varConnection = Locale.sqlConnectOneTime(Locale.sqlDataConnectionDetailsDZP))
        using (var sqlQuery = new SqlCommand(preparedCommand, varConnection))
        using (var sqlQueryResult = sqlQuery.ExecuteReader())
            if (sqlQueryResult != null) {
                while (sqlQueryResult.Read()) {
                    varRows = sqlQueryResult["Rows"].ToString();
                }
                sqlQueryResult.Close();
            }
        return varRows;
    }

Is it good? Or should i use it like this?

public static string sqlGetDatabaseRows() {
        string varRows = "";
        const string preparedCommand = @"
                    SELECT SUM(row_count) AS 'Rows'
                    FROM sys.dm_db_partition_stats
                    WHERE index_id IN (0,1)
                    AND OBJECTPROPERTY([object_id], 'IsMsShipped') = 0;";
        using (var varConnection = Locale.sqlConnectOneTime(Locale.sqlDataConnectionDetailsDZP)) {
            using (var sqlQuery = new SqlCommand(preparedCommand, varConnection))
            using (var sqlQueryResult = sqlQuery.ExecuteReader())
                if (sqlQueryResult != null) {
                    while (sqlQueryResult.Read()) {
                        varRows = sqlQueryResult["Rows"].ToString();
                    }
                    sqlQueryResult.Close();
                }
            varConnection.Close();
        }
        return varRows;
    }

sqlConnectOneTime looks like this:

    public static SqlConnection sqlConnectOneTime(string varSqlConnectionDetails) {
        SqlConnection sqlConnection = new SqlConnection(varSqlConnectionDetails);
        sqlConnect(sqlConnection);
        if (sqlConnection.State == ConnectionState.Open) {
            return sqlConnection;
        }
        return null;
    }

My questions are:

  1. Should I be closing varConnection by using varConnection.Close() and sqlQueryResult.Close(); in the end? (but this enforces use of brackets) or will the connection close itself when using is done.

  2. Should i be checking for NULL for varConnection since it's possible it will return null (on the other hand resharper doesn't complain here).

  3. Is there some better approach i could use for defining sqlConnectOneTime ? Like when the connection fails to open it should return ConnectionState.Closed instead of null?

Also just on a note i open up new connection every time i execute new query / update / insert since i am using threading and it was the smartest idea i could come up at that very moment. Feel free to suggest better one :-)

I am asking about all this since i want to better understand whole process and stop making silly mistakes so be gentle with me.

MadBoy

Edit: modified question if varConnection.Close() and sqlQueryResult.Close() are nessecary if using is used.

+1  A: 

It's safe to use it like in the first example. using closes the reader, command and connection objects and even checks for null values (so you don't get a NullReferenceException if varConnection is null)

Pent Ploompuu
What does it do if any of those variables are null? If varConnection is null it will return and not execute what's inside the first,2nd using.
MadBoy
What I meant by the null check is that before calling Dispose, it checks if the variable is null. So you still have to check if `varConnection` is null before passing it to some other method. However I would advise against returning null values from the `sqlConnectOneTime` method and instead throw an exception.
Pent Ploompuu
+1  A: 
  1. Close() is called by the Dispose() method, so as long as you are using "using" properly you don't need to call Close() explicitly.
  2. If it's possible for a NULL to be returned, you should check for it. I would advise that if you control the code that gets the sql connection, you strongly consider throwing an exception rather than returning NULL. Otherwise other developers may run into the same sorts of problems. Failure to open a SQL connection that is required seems to me to be a valid exceptional case. If necessary, you can always include a TryConnectOneTime for when a developer wants to avoid exception handling.

Also, another style note - I would advise properly bracing your using statements in case a stray extra line is added and an unexpected error occurs. Style-wise, I usually don't tend to indent using statements when I have multiple statements together, but that's all down to personal preference.

Ryan Brunner
The thing is i am the only one creating this application so no one else will call it :-) Also throwing exception isn't good idea as it will confuse users. I guess the best option would be to stay quiet and so that if the query isn't executed user gets no info so he/she simply retries. Eventually I guess i could add some MessageBox saying that the connection cannot be established if he/she want to try again? Do you think it would be best approach ?
MadBoy
I certainly don't think you should expose the exception directly to the users. However, this shouldn't be the concern of the connection call. It's signature is fairly clear - it is responsible for creating a connection. If it can't perform this task, it's reasonable for it to throw an exception (and for the calling code to catch this and deal with it appropriately). By returning null, you are returning an unexpected value, and worse yet, providing no information about why this connection failed. By throwing an exception, you resolve both of these issues.
Ryan Brunner