Returns Office Details from Database

Question

I am building an Administration web site for the Office details in my database. Right now, I am just trying to display the office details.

public class OfficeRepository
{
 public static Office GetOffice(int syncID)
 {
 List<Office> results = new List<Office>();
 var sql = @"Select so.SyncID, so.title From Offices o Left Outer Join SyncOffices so On so.id = o.SyncID Where o.SyncID = @syncID";
 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())
 {
 while (reader.Read())
 {
 var office = new Office();
 office.SyncID = reader.GetInt32(0);
 office.OfficeName = reader.GetString(1);
 results.Add(office);
 }
 }
 }
 Office returnOffice = new Office();
 returnOffice.SyncID = results.FirstOrDefault().SyncID;
 returnOffice.OfficeName = results.FirstOrDefault().OfficeName;
 return returnOffice;
 }
}
public class HomeController : Controller
{
 public ActionResult Index(int id = 0)
 {
 Office currentOffice = OfficeRepository.GetOffice(id); 
 var officeMgrViewModel = new OfficeMgrViewModel();
 officeMgrViewModel.Office = currentOffice;
 return View(officeMgrViewModel);
 }
}
public class OfficeMgrViewModel
{
 public Office Office { get; set; }
}
public class Office
{
 public Int32 SyncID { get; set; }
 public string OfficeName { get; set; }
}

• syb0rg: Could you include a short description of what this code does? It will help with the reviewing process.
• ADH: I am building an Administration web site for the Office details in my database. Right now, I am just trying to display the office details.
• Jamal: You should also have these descriptions in the question itself, not just as comments here.

Answer 1

You may want your properties to be private set so that they can't be changed from the outside:

public class Office
{
 public Int32 SyncID { get; private set; }
 public string OfficeName { get; private set; }
}

To do that you can make GetOffice a member of Office instead of OfficeRepository.

Or you can define a constructor ...

// Logically immutable
public class Office
{
 public Office(Int32 SyncID, string OfficeName)
 {
 this.SyncID = SyncID;
 this.OfficeName = OfficeName;
 }
 public Int32 SyncID { get; private set; }
 public string OfficeName { get; private set; }
}

... and change your GetOffice method to invoke the constructor instead of invoking the set properties.

Similarly, OfficeMgrViewModel needn't have a public set property if it had a constructor.

See also this answer which mentions how to simplify your GetOffice method to not use a List, if you expect that your join where will never produce more than one record.

Answer 2

In addition to ChrisW's suggestion, I would change your repository method a bit:

public static Office GetOffice(int syncID)
{
 //remove this List<Office> results = new List<Office>();
 var sql = @"Select so.SyncID, so.title From Offices o Left Outer Join SyncOffices so On so.id = o.SyncID Where o.SyncID = @syncID";
 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())
 {
 while (reader.Read())
 {
 //simply return the first result here (using ChrisW's suggested constructor)
 return new Office(reader.GetInt32(0), reader.GetString(1));
 }
 }
 }
 return null; //return null here and handle within calling code
}

By returning null, you can handle a null office in your controller code (show an error and allow the user to select a different syncId, for example). In the old code, if no results were found, the FirstOrDefault().SyncId would throw an exception.

• ChrisW: +1 However throwing from inside the SQL method would be better, if the passed-in SyncID was supposed to be valid and not fail, and therefore the calling code is NOT coded to expect/handle a null Office.
• Henry Fieger: Agreed - I think it depends on a) how the front-end is set up to handle failures and b) where the syncIDs come from.