6
\$\begingroup\$

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; }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ could Allergies be an enum instead of string? I can see there being hundreds of elements, but IMHO type safety, intelisense, and avoiding string gotchas (CaSe, typos) wins out. Finally, the enum necessarily defines all possible allergies and does so in one place in code. \$\endgroup\$
    – radarbob
    Oct 20, 2013 at 18:39

2 Answers 2

6
\$\begingroup\$

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

\$\endgroup\$
3
  • \$\begingroup\$ those are really practices. Number 1 was me just not testing properly, the rest are spot on. But going back to number one, you said sorting the list each time might be slow, if I had thousands of records what would be the best approach to getting the latest 3? \$\endgroup\$
    – datatest
    Oct 19, 2013 at 13:19
  • \$\begingroup\$ also, I have edited my code based on your suggestions. One more question, should I even have GetRecentVitalSigns in my Patient model, or should it be moved to the repository? \$\endgroup\$
    – datatest
    Oct 19, 2013 at 13:32
  • 1
    \$\begingroup\$ @datatest If you have just thousands and you don't call this method that often, that might be okay. You should try it and see if it's a problem in your real application. But one possible solution would be to use SortedSet, where you can be sure that it's always sorted, so you don't have to sort it again. \$\endgroup\$
    – svick
    Oct 19, 2013 at 14:39
3
\$\begingroup\$

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.
\$\endgroup\$
3
  • \$\begingroup\$ I like the idea of using a cache version of RecentVitalSigns, will definitely be adding that \$\endgroup\$
    – datatest
    Oct 19, 2013 at 14:29
  • \$\begingroup\$ The one caveat for IReadOnlyList is that it's new in .Net 4.5. \$\endgroup\$
    – svick
    Oct 19, 2013 at 14:40
  • \$\begingroup\$ @svick Yes, and you should upgrade to 4.5 now if you have not done that yet. :P \$\endgroup\$
    – tia
    Oct 20, 2013 at 4:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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