Linq C# pulling from table row into input fields

Question

Today is the first time I've used Linq, so I wanted to get a code review on this and make sure I'm going about it correctly.

I'm filling text boxes and dropdown lists from a table row.

protected void Page_Load(object sender, EventArgs e)
        {

            using (DBContext da = new DBContext())
            {
                string dob;
                var detail = from a in da.tblPatients
                             where a.ID == 1198
                             select a;
                foreach (var d in detail)
                {

                    txtFirstName.Text = d.FirstName;
                    txtLastName.Text = d.LastName;
                    txtPreferredName.Text = d.PreferredName;
                    dob = d.DOB;
                    if(!String.IsNullOrEmpty(dob))
                    {
                        try
                        {
                            int lastindexofdob = 0;
                            string month = "";
                            string day = "";
                            string year = "";
                            lastindexofdob = dob.LastIndexOf("/");
                            month = dob.Substring(0, dob.IndexOf("/"));
                            day = dob.Substring(month.Length + 1, (dob.LastIndexOf("/") - month.Length) - 1);
                            year = dob.Substring(dob.Length - 4);

                            ddlDOBMonth.SelectedIndex = ddlDOBMonth.Items.IndexOf(ddlDOBMonth.Items.FindByValue(month));
                            ddlDOBDay.SelectedIndex = ddlDOBDay.Items.IndexOf(ddlDOBDay.Items.FindByText(day));
                            ddlDOBYear.SelectedIndex = ddlDOBYear.Items.IndexOf(ddlDOBYear.Items.FindByText(year));
                        }
                        catch { //error catching will go here}

                    }
                    txtAddress.Text = d.Address;
                }
            }  

        }

There are actually about 20 more input fields to do, but I wanted a code review at this point.

Answer 1

Your use of linq is fine.

You may consider using a more descriptive variable than a, such as patient

You should pass any query parameters likely to change as variables. Consider:

var patientID = 1198;
var detail = from patient in da.tblPatients
             where patient.ID == patientID
             select patient;

I find the form population logic and database design to be a touch more troubling. txtFirstName.Text = d.FirstName is perfectly fine. DOB, which I assume to be date of birth should be stored and used as a Date rather than a String.

If you are unable to change the data-layer, at least convert it to a Date for use using Date.Parse or similar.

Your try block is lazy coding. Check the results from your dob.IndexOf and 'dob.Substring' calls rather than relying on catching an error -- be defensive in your code. Consider the following:

DateTime dob = DateTime.MinValue;
if (DateTime.TryParse(d.DOB, dob))
{
    ddlDOBMonth.SelectedValue = dob.Month.ToString();
    ddlDOBDay.SelectedValue = dob.Day.ToString();
    ddlDOBYear.SelectedValue = dob.Year.ToString();
}

Answer 2

Naming

A linq query will return an IEnumerable<T>. Also, as you are using (hopefully) a non null unique ID, your linq query will return an IEnumerable<T> containing one or zero elements. Therefor a variable should be named using a form of plural.

As psaxton already pointed out a should be patient so detail should be patients.

The query in the Page_Load eventhandler

Is a little bit senseless at this place, as your page is now always populated by the patiens value with the ID == 1198. A better approach would be to add a method which takes either an ID or a Patient as parameter, where the later would be better.

But for the purpose of this review we should do both.

So let us refactor the Page_Load first as this is easier.

protected void Page_Load(object sender, EventArgs e)
{
    UpdateView(1198);
}  

That had been easy, hadn't it ?

But now we need the UpdateView(int) method. So let us write it.

private void UpdateView(int ID)
{
    Patient patient;
    using (DBContext context = new DBContext())
    {
        patient =  (from patient in da.tblPatients
             where patient.ID == ID
             select patient).FirstOrDefault();

    }
    UpdateView(patient);
}

We are using the FirstOrDefault() extension method to get either the first item in the IEnumerable<Patient> or the default which will be null here.

So taken into account the patient could be null we should think about what should happen in the UpdateView(Patient) method. To keep the same functionality as the original code, we just don't populate/clear any UI element if patient == null.

private void UpdateView(Patient patient)
{
    if (patient == null) { return; }

    txtFirstName.Text = patient.FirstName;
    txtLastName.Text = patient.LastName;
    txtPreferredName.Text = patient.PreferredName;

    .....
    .....
    .....

}  

Also you won't need the try..catch anymore, if you follow psaxton's advice , in the current state (as posted) it wouldn't compile at all because by initializing a comment with // all the trailing will be a comment, so the } too.

Additional, a empty catch block should be avoided as long as you are not sure that you really can swallow the exception wich also is rare.