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; }
}

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.