Return office details from multiple tables

Question

The function itself is just returning as void. But, the point of the posted code is not about what the function is returning. It is the code related to using T-SQL and C# to return data from a SQL database that I would like reviewed. Especially the way the using statements are structured.

    public void GetOffice(int syncID)
    {
        string strQry = @"
Select so.SyncID, 
   so.title
From Offices o
Left Outer Join SyncOffices so On so.id = o.SyncID
Where o.SyncID = @syncID
"; 

        using (SqlConnection conn = new SqlConnection(Settings.ConnectionString))
        {
            using (SqlCommand objCommand = new SqlCommand(strQry, conn))
            {
                objCommand.CommandType = CommandType.Text;
                objCommand.Parameters.AddWithValue("@syncID", syncID);
                conn.Open();
                SqlDataReader rdr = objCommand.ExecuteReader();

                if (rdr.Read())
                {
                    this.OfficeName= rdr.GetString(1);
                }

                rdr.Close();
            }
        }
    }

Answer 1

First of all, kudos for using a Parameter and not concatenating the value into your T-SQL string.

You're not disposing all IDisposable objects. SqlDataReader should be disposed as well. Now this makes it quite a bunch of nested using scopes, which you could rework like this:

    using (var connection = new SqlConnection(Settings.ConnectionString))
    using (var command = new SqlCommand(sql, connection))
    {
        command.CommandType = CommandType.Text;
        command.Parameters.AddWithValue("@syncID", syncId);
        connection.Open();

        using (var reader = command.ExecuteReader())
        {
            if (reader.Read())
            {
                this.OfficeName = reader.GetString(1);
            }
        }
    }

Note:

• Usage of var for implicit typing makes the code easier to read (IMO), if you're using C# 3.0+
• Disemvoweling is bad. There's no reason to call a variable rdr over reader. Use meaningful names, always.
• Hungarian notation is evil. There's no reason to prefix a string with str.
• Stick to camelCasing for locals - that includes parameters, so syncID becomes syncId.

The code assumes the query only returns 1 row, but the query isn't written to explicitly select a single row. This could lead to unexpected results.

Given some IList<string> results = new List<string>();:

        using (var reader = command.ExecuteReader())
        {
            while (reader.Read())
            {
                results.Add(reader.GetString(1));
            }
        }

You could then do this.OfficeName = results.Single(); (which would blow up if no rows were returned). One thing that strikes me, is that you're selecting 2 fields, but only using 1, which makes this reader.GetString(1) statement look surprising. If you don't need to select the SyncID field, remove it from your query and do reader.GetString(0) instead.

Finally, the T-SQL itself:

Select so.SyncID, so.title
From Offices o
Left Outer Join SyncOffices so On so.id = o.SyncID
Where o.SyncID = @syncID

Could look like this:

SELECT so.Title
FROM Offices o
LEFT JOIN SyncOffices so ON so.id = o.SyncID
WHERE o.SyncID = @syncID

Or, in a string:

var sql = "SELECT so.Title FROM Offices o LEFT JOIN SyncOffices so ON so.Id = o.SyncId WHERE o.SyncId = @syncId";

The line breaks make it look weird, and since it's not too long of a query, I think it would make the code better to have it on a single line.

Answer 2

I notice that GetOffice returns void and that the result of the query is written into this.OfficeName.

I usually have one (separate) file which contains/encapsulates all my SQL statements. So I'd have something like,

public void GetOffice(int syncID)
{
    this.OfficeName = Sql.GetOfficeName(syncID);
}

... where GetOfficeName is a static method of my Sql class.

Having all my SQL in one (separate file) makes it easier to do a code review of all the ways in which I use SQL; for example to ensure that I'm using the same 'best practices' (e.g. using transactions, disposing objects) in all SQL-related methods.