Function for obtaining the 3 latest patient vital signs

Question

I'm just putting together a small project, checking to see if this would be the fastest/cleanest way to write out a function that grabs the 3 latest vital signs.

Edit: I am thinking GetRecentVitalSigns should be in the domain model.

Also, if I wanted to grab all the vital signs, I would just call the VitalSigns list, but I was thinking I could make that as private and create a GetAllVitalSigns, but that seems redundant. Any advice would be excellent (on anything related).

public class Patient
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public DateTime AdmissionDate { get; set; }
    public List<string> Allergies { get; set; }
    public List<VitalSign> VitalSigns { get; set; } 

    public List<VitalSign> GetRecentVitalSigns()
    {
        if(VitalSigns.Count > 0)
        {
            var results = VitalSigns
                .Take(3)
                .OrderBy(x => x.DateTimeChecked)
                .ToList();

            return results;
        }
        return null;
    }
}
public class VitalSign
{
    public decimal BodyTemperature { get; set; }
    public int Pulse { get; set; }
    public int BloodPressure { get; set; }
    public int RespiratoryRate { get; set; }
    public DateTime DateTimeChecked { get; set; }
}

New structure

public class Patient
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public DateTime AdmissionDate { get; set; }
    public List<string> Allergies { get; set; }
    private List<VitalSign> vitalSigns;

    private IEnumerable<VitalSign> VitalSigns
    {
        get { return vitalSigns; }
    }

    public IEnumerable<VitalSign> GetRecentVitalSigns(int vitalSignsCount = 3)
    {
        var recentVitalSigns = new IEnumerable<VitalSign>();
        if (recentVitalSigns.Any())
        {
            recentVitalSigns = vitalSigns
                .OrderBy(vs => vs.DateTimeChecked)
                .Take(vitalSignsCount);
        }
        return recentVitalSigns;
    }
}
public class VitalSign
{
    public decimal BodyTemperature { get; set; }
    public int Pulse { get; set; }
    public int BloodPressure { get; set; }
    public int RespiratoryRate { get; set; }
    public DateTime DateTimeChecked { get; set; }
}

Answer 1

I would just call the VitalSigns list, but I was thinking I could make that as private and create a GetAllVitalSigns, but that seems redundant.

That depends on what you want the users of your class to allow. As it is, any user can modify everything about the contents of your objects. Depending on how are you going to use them, that might not be a good idea.

Consider whether it would make sense to make (some of) your setters private or whether VitalSigns should be more something like:

private List<VitalSign> vitalSigns;
public IEnumerable<VitalSign> VitalSigns { get { return vitalSigns; } } 

Now, to the GetRecentVitalSigns() method:

public List<VitalSign> GetRecentVitalSigns()
{
    if(VitalSigns.Count > 0)
    {
        var results = VitalSigns
            .Take(3)
            .OrderBy(x => x.DateTimeChecked)
            .ToList();

        return results;
    }
    return null;
}

This seems wrong to me on several levels:

1. Most importantly, I think it's wrong, because you first take the first 3 items from the list and then order those 3 items. You should do it the other way around: first sort and then take the last 3 based on that.

   Depending on how many items are in the list, sorting the whole collection every time might be too slow. But it's hard to suggest the best solution without knowing how the class is going to be used.

2. You return null when there are no results. That's a bad practice, when you want to represent an empty collection, use empty collection, not null. It will make both the code that uses the result and the code in the method simpler and avoids NullReferenceException bugs.

3. You return List. I see no good reason for that here, just return IEnumerable. If the caller of this method wants a List that they can modify, they can easily do that by calling ToList() themselves.

4. When it makes sense, make even the variables in your lambdas at least a bit descriptive. For example, using vs for a VitalSign is better then just x.

5. Consider whether the number of items to return should be configurable. Are you sure 3 is always the right number? You could still make it the default. (On the other hand, don't do this if you expect that 3 will be always the right number.)

This means that I would probably write the method like this:

public IEnumerable<VitalSign> GetRecentVitalSigns(int vitalSignsCount = 3)
{
    return VitalSigns
        .OrderByDescending(vs => vs.DateTimeChecked)
        .Take(vitalSignsCount);
}

Also:

public decimal BodyTemperature { get; set; }

Yes. I think that most people would use double here (decimal is just for money, right?), but decimal is probably the right choice here. (Assuming the temperature is entered by humans. If it it were somehow read directly from the thermometer, then double would make sense.)

Answer 2

You should use OrderByDescending, and do it before Take

var results = VitalSigns
                    .OrderByDescending(x => x.DateTimeChecked)
                    .Take(3)
                    .ToList();

The VitalSigns should be IReadOnlyList<VitalSign> so the class will always know when list get changed, then we can support caching safely.

public IReadOnlyList<VitalSign> VitalSigns { get; set { ... } } 

Also make a property for recent vital signs and we can cache it and use create-on-read method.

public IReadOnlyList<VitalSign> RecentVitalSigns { ... }

Also make the VitalSign immutable, thus prevent any changes and discrepancy of the cached recent value. The final class design should look like this for me:

public class Patient
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public DateTime AdmissionDate { get; set; }
    public List<string> Allergies { get; set; }
    private IReadOnlyList<VitalSign> _vitalSigns;
    private IReadOnlyList<VitalSign> _recentVitalSigns;
    public IReadOnlyList<VitalSign> VitalSigns { 
        get { return _vitalSigns; }
        set { 
            _vitalSigns = value; 
            _recentVitalSigns = null;
        }
    }

    public IReadOnlyList<VitalSign> RecentVitalSigns {
        get { 
            if (_recentVitalSigns == null) {
                _recentVitalSigns = GetRecentVitalSigns ();
            }
            return _recentVitalSigns;
        }
    }

    private IReadOnlyList<VitalSign> GetRecentVitalSigns()
    {
        var results = VitalSigns
                    .OrderByDescending(x => x.DateTimeChecked)
                    .Take(3)
                    .ToList();

        return results;
    }
}

public class VitalSign
{
    public decimal BodyTemperature { get; private set; }
    public int Pulse { get; private set; }
    public int BloodPressure { get; private set; }
    public int RespiratoryRate { get; private set; }
    public DateTime DateTimeChecked { get; private set; }
    public VitalSign (decimal bodyTemperature, int pulse, int bloodPressure, int respiratoryRate, DateTime dateTimeChecked)
    {
        this.BodyTemperature = bodyTemperature;
        this.Pulse = pulse;
        this.BloodPressure = bloodPressure;
        this.RespiratoryRate = respiratoryRate;
        this.DateTimeChecked = dateTimeChecked;
    }
}

There are few main features in my design:

1. We have cache version of RecentVitalSigns ready, so multiple calls, e.g. for frequent UI update, would need single LINQ execution.
2. Setting VitalSigns only invalidate RecentVitalSigns cache, not recreating it, so setting VitalSigns does not hurt the performance.
3. Data is read-only so we can be sure our cache is always valid to the real data and no one could mess it up.