Function for obtaining the 3 latest patient vital signs

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 Allergies { get; set; }
    public List VitalSigns { get; set; } 

    public List 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 Allergies { get; set; }
private List vitalSigns;

private IEnumerable VitalSigns
{
get { return vitalSigns; }
}

public IEnumerable GetRecentVitalSigns(int vitalSignsCount = 3)
{
var recentVitalSigns = new IEnumerable();
if (recentVitalSigns.Any())
{
recentVitalSigns = vitalSigns
.OrderBy(vs => vs.DateTimeChecked)
.Take(vitalSignsCount);
}
return recentVitalSigns;
}
}
public class VitalSign
{
public decimal

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 vitalSigns;
public IEnumerable VitalSigns { get { return vitalSigns; } }

Now, to the GetRecentVitalSigns() method:

public List 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:

-
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.

-
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.

-
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.

-
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.

-
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 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.)

StackExchange Code Review Q#32916, answer score: 6