2
\$\begingroup\$

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; }
}
\$\endgroup\$
3
  • \$\begingroup\$ Could you include a short description of what this code does? It will help with the reviewing process. \$\endgroup\$
    – syb0rg
    Feb 18, 2014 at 20:59
  • \$\begingroup\$ 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. \$\endgroup\$
    – ADH
    Feb 18, 2014 at 21:01
  • 1
    \$\begingroup\$ You should also have these descriptions in the question itself, not just as comments here. \$\endgroup\$
    – Jamal
    Feb 18, 2014 at 21:02

2 Answers 2

4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ +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. \$\endgroup\$
    – ChrisW
    Feb 20, 2014 at 7:17
  • \$\begingroup\$ Agreed - I think it depends on a) how the front-end is set up to handle failures and b) where the syncIDs come from. \$\endgroup\$ Feb 20, 2014 at 21:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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