2
\$\begingroup\$

This code correctly returns a View_appointment. Is there a better way to accomplish this with using left joins and unions? I'm trying to learn Linq a little bit better.

Components:

  • PostgreSQL 9.3 (backend database)
  • Telerik DataAccess (the entity framework)

Overview:

I am writing an appointment book for my office. Each "appointment" is unique by its date (tappointment.date) and a cell number. The date and cell number correspond 1 to 1 with the cells of a WPF DataGrid. Each appointment cell can be reused depending on the activity of its current occupant so has a history file attached. That is, if the current person assigned to that cell "cancels" their appointment, the cell can be assigned to someone else.

Patients can be assigned to the cell from one of two tables:

  • New_patients
  • Established_patients

New_patients do not have many of the fields which Established_patients have (e.g., a chart, or insurance information). Established_patients in turn reference a third file, Patients, that contains the full description of the patient.

A separate sequence provides a unique ticket number for each patient making an appointment. That is the ticket number is unique in New_patients and Established_patients and a given ticket number will only be in either New_patients or established_patients --not both.

Therefore, how best can I return the View_appointment record when the ticketno could be in either new_patient or established_patients? And if in established_patients, how then to join it with the Patient table? Can this be done with left joins and unions?

  public View_appointment get_appointment(DateTime tappointment, int cellnum)
    {
        using (var ctx = new Nova.Data.Data())
        {
            // get the appointment cell and its most recent history useage.
            var x = (from a in ctx.Appointments
                     where a.Tappointment.Date == tappointment.Date && a.Cellnum == cellnum
                     join h in ctx.Appointment_histories on a.Recid equals h.Appointment_recid
                     orderby h.Tposted
                     select new
                     {
                         APPOINTMENT = a,
                         HISTORY = h
                     }).ToList().LastOrDefault();

            if (x == null) return null;

            // retrieve the patient name associated with this ticketno
            var y = ( from e in ctx.Established_patients
                      where e.Ticketno == x.HISTORY.Ticketno
                      select e).SingleOrDefault();

            var y1 = ( from f in ctx.New_patients
                       where f.Ticketno == x.HISTORY.Ticketno
                       select f).SingleOrDefault();

            // expand records into one view
            View_appointment v = null;

            if ( y != null )
            {           // Established patients are those with a chart
                var p = (from a in ctx.Patients
                         where a.Recid == y.Patient_recid
                         select new 
                         {  
                             a.Recid,
                             a.Lastname,
                             a.Firstname,
                             a.Birthdate,
                             a.Mi,
                             a.Sex,
                             a.Phone
                         }).Single();

                v = new View_appointment {
                    Appointment_history_recid = x.HISTORY.Recid,
                    Appointment_recid = x.APPOINTMENT.Recid,
                    Birthdate = p.Birthdate,
                    Cancelled = x.HISTORY.Cancelled,
                    Cellnum = x.APPOINTMENT.Cellnum,
                    Contact_number = p.Phone,
                    Deleted = x.HISTORY.Deleted,
                    Firstname = p.Firstname,
                    Lastname = p.Lastname,
                    Mi = p.Mi,
                    Name_record = x.HISTORY.Name_record,
                    Noshow = x.HISTORY.Noshow,
                    Orderno = x.HISTORY.Orderno,
                    Patient_recid = p.Recid,
                    Rescheduled = x.HISTORY.Rescheduled,
                    Sex = p.Sex,
                    Tappointment = x.APPOINTMENT.Tappointment,
                    Ticketno = x.HISTORY.Ticketno,
                    Tposted = x.HISTORY.Tposted,
                    Why = x.HISTORY.Why
                };
            }
            else if (y1 != null)
            {       // new patients have never been seen before
                v = new View_appointment
                {
                    Appointment_history_recid = x.HISTORY.Recid,
                    Appointment_recid = x.APPOINTMENT.Recid,
                    Birthdate = y1.Birthdate,
                    Cancelled = x.HISTORY.Cancelled,
                    Cellnum = x.APPOINTMENT.Cellnum,
                    Contact_number = y1.Contact_number,
                    Deleted = x.HISTORY.Deleted,
                    Firstname = y1.Firstname,
                    Lastname = y1.Lastname,
                    Mi = y1.Mi,
                    Name_record = x.HISTORY.Name_record,
                    Noshow = x.HISTORY.Noshow,
                    Orderno = x.HISTORY.Orderno,
                    Patient_recid = null,
                    Rescheduled = x.HISTORY.Rescheduled,
                    Sex = y1.Sex,
                    Tappointment = x.APPOINTMENT.Tappointment,
                    Ticketno = x.HISTORY.Ticketno,
                    Tposted = x.HISTORY.Tposted,
                    Why = x.HISTORY.Why
                };
            }

            return v;
        }

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

I don't know much LINQ or C#, so I will only review the form, as I find a glaring issue in your code.

Naming Things

You wrote your whole script using mostly 1-letter identifiers, like x, a, h, y and even y1. When I was trying to read your code I found myself constantly going up and down the page to see what those identifiers refer to, since you can't tell by their names. This makes it much more difficult to focus on what the code actually does.

This part is particularly hard to follow and illustrates the point:

v = new View_appointment {
    Appointment_history_recid = x.HISTORY.Recid,
    Appointment_recid = x.APPOINTMENT.Recid,
    Birthdate = p.Birthdate,
    Cancelled = x.HISTORY.Cancelled,
    Cellnum = x.APPOINTMENT.Cellnum,
    Contact_number = p.Phone,
    Deleted = x.HISTORY.Deleted,
    Firstname = p.Firstname,
    Lastname = p.Lastname,
    Mi = p.Mi,
    Name_record = x.HISTORY.Name_record,
    Noshow = x.HISTORY.Noshow,
    Orderno = x.HISTORY.Orderno,
    Patient_recid = p.Recid,
    Rescheduled = x.HISTORY.Rescheduled,
    Sex = p.Sex,
    Tappointment = x.APPOINTMENT.Tappointment,
    Ticketno = x.HISTORY.Ticketno,
    Tposted = x.HISTORY.Tposted,
    Why = x.HISTORY.Why
};

It's easy enough to Refactor->Rename using almost any IDE, so this would be a way to make your code much more legible. It's OK to have short names and/or abbreviate, but not to the point where it makes the code hard to maintain. One advantage that can help you is that medical terminology has thorough lists of approved abbreviations; here is one from Wikipedia.

Consider refactoring:

a -> appt
v -> viewAppt
x -> apptHistory
p -> pt
y -> estPt
y1 -> newPt
etc.

And now that section reads much more easily, for example. Do this throughout your code and you will thank yourself in a few months when you look at your code again!

viewAppt = new View_appointment {
    Appointment_history_recid = apptHistory.HISTORY.Recid,
    Appointment_recid = apptHistory.APPOINTMENT.Recid,
    Birthdate = pt.Birthdate,
    Cancelled = apptHistory.HISTORY.Cancelled,
    Cellnum = apptHistory.APPOINTMENT.Cellnum,
    Contact_number = pt.Phone,
    Deleted = apptHistory.HISTORY.Deleted,
    Firstname = pt.Firstname,
    Lastname = pt.Lastname,
    Mi = pt.Mi,
    Name_record = apptHistory.HISTORY.Name_record,
    Noshow = apptHistory.HISTORY.Noshow,
    Orderno = apptHistory.HISTORY.Orderno,
    Patient_recid = pt.Recid,
    Rescheduled = apptHistory.HISTORY.Rescheduled,
    Sex = pt.Sex,
    Tappointment = apptHistory.APPOINTMENT.Tappointment,
    Ticketno = apptHistory.HISTORY.Ticketno,
    Tposted = apptHistory.HISTORY.Tposted,
    Why = apptHistory.HISTORY.Why
};
\$\endgroup\$
  • \$\begingroup\$ Good idea. Thanks :) How about the left-join and union? \$\endgroup\$ – Alan Wayne Jan 28 '16 at 2:40
  • \$\begingroup\$ @AlanWayne I don't know much about LINQ and C#, maybe another reviewer will address that topic. \$\endgroup\$ – Phrancis Jan 28 '16 at 5:27
0
\$\begingroup\$

Using CTE's directly in a postgreSQL stored procedure, the above code can be easily and clearly rewritten. (And as this will be on the server, I suspect much faster!).

CREATE OR REPLACE FUNCTION "GetAppointmentsForDay"(tschedule timestamp)
  RETURNS SETOF view_appointment AS
$BODY$

with apt as (   -- get the appointments for the day
    select a.recid as appointment_recid, a.tappointment, a.cellnum 
    from appointments a
    where a.tappointment::date = tschedule::date
)
, history as (  -- get the history of each appointment cell
    select apt.*, h.recid as history_recid, h.tposted, h.ticketno, h.orderno, h.noshow 
           ,h.cancelled, h.rescheduled, h.deleted, h.name_record, h.why 
    from apt
    join appointment_history h on (h.appointment_recid = apt.appointment_recid)
)
, estpt as (    -- get the appointments for established patients 
    select 0 as recid, h.*, p.recid as patient_recid, p.lastname, p.firstname, p.mi, p.birthdate, p.sex, p.phone 
    from history h 
    join established_patients e on (h.ticketno = e.ticketno)
    join patients p on (p.recid = e.patient_recid) 
)
, newpt as (    -- get the appointments for the new patients
    select 0 as recid, h.*, null::integer, n.lastname, n.firstname, n.mi, n.birthdate, n.sex, n.contact_number
    from history h
    join new_patients n on (n.ticketno = h.ticketno)
)
select * from estpt
union
select * from newpt
order by cellnum

$BODY$
  LANGUAGE sql VOLATILE
  COST 100
  ROWS 1000;
ALTER FUNCTION "GetAppointmentsForDay"(timestamp)
  OWNER TO postgres;


select * from "GetAppointmentsForDay"('2013-11-12 15:15:00');
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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