Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
        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();
            }
        }
    }
share|improve this question
6  
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 at 19:20
add comment

3 Answers

up vote 7 down vote accepted

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.

share|improve this answer
    
Thank you for the response. Everything makes sense. I want to point out that I am only expecting 1 row. –  Allan Horwitz Feb 18 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 at 20:32
    
Also, if you make the string into a single-line, you can drop the @ before the ". –  svick Feb 18 at 20:34
    
@svick thanks, edited ;) –  Mat's Mug Feb 18 at 21:26
5  
+1 if for no other reason than using the word "Disemvoweling". –  Comintern Feb 19 at 1:39
add comment

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.

share|improve this answer
add comment

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?

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.