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

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

Community · Accepted Answer · 2017-04-13 12:40:41Z

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.

Henry Fieger · Accepted Answer · 2014-02-19 22:20:16Z

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.

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

Stack Exchange Network

Returns Office Details from Database

2 Answers 2

You must log in to answer this question.

Linked

Hot Network Questions

Returns Office Details from Database

2 Answers 2

You must log in to answer this question.

Linked

Related

Hot Network Questions