# Return Office Details From Multiple Tables

        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;
conn.Open();

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

rdr.Close();
}
}
}

-
Welcome to CR! Please include a bit of context along with your code, tell us what the code is doing, what you like about it, what you would like to improve, points of concerns, points of interest... anything! –  Mat's Mug Feb 18 '14 at 19:20

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;
connection.Open();

{
{
}
}
}


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())
{
{
}
}


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.

-
Thank you for the response. Everything makes sense. I want to point out that I am only expecting 1 row. –  Allan Horwitz Feb 18 '14 at 19:51
If you're going to use FirstOrDefault() then I don't see any reason to retrieve the extra rows. (It would be different if you used SingleOrDefault() or Single(), which I usually prefer.) –  svick Feb 18 '14 at 20:32
Also, if you make the string into a single-line, you can drop the @ before the ". –  svick Feb 18 '14 at 20:34
+1 if for no other reason than using the word "Disemvoweling". –  Comintern Feb 19 '14 at 1:39

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.

-

How would you write a unit test for this class, and this method? Could you substitute a mock SqlConnection if you had to? Could you easily create the class with a fake OfficeName for test purposes? How does it behave if the connection fails to connect? How do you test the error handling does what you need it to do?

-
If you post your question on StackOverflow you might get an answer. –  Allan Horwitz Jul 1 '14 at 23:32