Refactor and reduce cyclomatic complexity with LINQ

Question

I have a method that I feel like could be refactored more efficiently with LINQ.

The purpose of the function is to use some logic to determine which phone number to return. The logic is: Any returned number must be sms_capable. If a number was last used for an rx, use it, otherwise return the first valid number by type in this order: Other, Home, Office

        string GetDefaultSMSPhoneNumber(IEnumerable<PhoneNumbers> patientNumbers)
    {
        const int PHONE_TYPE_HOME = 1;
        const int PHONE_TYPE_OFFICE = 3;
        const int PHONE_TYPE_OTHER = 9;

        var phoneNumberByType = patientNumbers.Where(p => p.sms_capable == 1).GroupBy(p => p.phone_type_id);

        // Select the phone number last used in creating a prescription
        if (patientNumbers.Where(p => p.sms_capable == 1 && p.last_used_for_rx == 1).Count() > 0)
        {
            return patientNumbers.Where(p => p.sms_capable == 1 && p.last_used_for_rx == 1).FirstOrDefault().phone_number;
        }

        // If no number has been used, select a configured SMS number in the following order (Other, Home, Office) 
        if (patientNumbers.Where(p => p.sms_capable == 1 && p.phone_type_id == PHONE_TYPE_OTHER).Count() > 0)
        {
            return patientNumbers.Where(p => p.sms_capable == 1 && p.phone_type_id == PHONE_TYPE_OTHER).FirstOrDefault().phone_number;
        }

        // If no number has been used, select a configured SMS number in the following order (Other, Home, Office) 
        if (patientNumbers.Where(p => p.sms_capable == 1 && p.phone_type_id == PHONE_TYPE_HOME).Count() > 0)
        {
            return patientNumbers.Where(p => p.sms_capable == 1 && p.phone_type_id == PHONE_TYPE_HOME).FirstOrDefault().phone_number;
        }

        // If no number has been used, select a configured SMS number in the following order (Other, Home, Office) 
        if (patientNumbers.Where(p => p.sms_capable == 1 && p.phone_type_id == PHONE_TYPE_OFFICE).Count() > 0)
        {
            return patientNumbers.Where(p => p.sms_capable == 1 && p.phone_type_id == PHONE_TYPE_OFFICE).FirstOrDefault().phone_number;
        }

        return string.Empty;
    }

I know the first thing I can do is filter the list to only sms_capable numbers. I feel like I should be able to use .GroupBy to group the numbers by there type, but after they're grouped I'm not sure how to return the first non empty value? I feel like I'm looking for a way to coalesce in linq?

    string GetDefaultSMSPhoneNumber(IEnumerable<PhoneNumbers> patientNumbers)
    {
        const int PHONE_TYPE_HOME = 1;
        const int PHONE_TYPE_OFFICE = 3;
        const int PHONE_TYPE_OTHER = 9;

        var phoneNumberByType = patientNumbers.Where(p => p.sms_capable == 1).GroupBy(p => p.phone_type_id);
        var phoneNumber = patientNumbers.FirstOrDefault(p => p.sms_capable == 1 && p.last_used_for_rx == 1)?.phone_number;

        // Doesn't work
        if (string.IsNullOrEmpty(phoneNumber))
        {
            var number =  phoneNumberByType.FirstOrDefault(p =>  p.Key == PHONE_TYPE_OTHER && p.Where(x => !string.IsNullOrEmpty(x.phone_number)) ||
                                                                (p.Key == PHONE_TYPE_HOME && p.Where(x => !string.IsNullOrEmpty(x.phone_number)) ||
                                                                (p.Key == PHONE_TYPE_OFFICE && p.Where(x => !string.IsNullOrEmpty(x.phone_number))));
        }

I'm trying to find a concise and efficient way of filtering to the needed data. Preferably with method syntax over query syntax.

You can order by and take first... Order by p.last_used_for_rx == 1 then -1 else typeid — Selvin, Feb 18 '23 at 21:59
Damn, you wana "other" first which is last id ... But you got the point - You would need to give weight to second predicate.. — Selvin, Feb 18 '23 at 22:04
FWIW, the cyclomatic-complexity FxCop/static-analysis rule is a blunt-instrument - it's safe to suppress its warnings when the code _actually_ is straightforward and easy to read. Your code looks fine to me: attempts to "simplify" it might add inadvertent complexity. — Dai, Feb 18 '23 at 22:19

Guru Stron · Answer 1 · 2023-02-18T22:36:03.660

If you need matching against predicates in specific order you can create a collection of Func<PhoneNumbers, bool> and iterate it (also if PhoneNumbers is a class or record then you don't need Count, if it is not, better use Any instead of count):

string GetDefaultSMSPhoneNumber(IEnumerable<PhoneNumbers> patientNumbers)
{
    const int PHONE_TYPE_HOME = 1;
    const int PHONE_TYPE_OFFICE = 3;
    const int PHONE_TYPE_OTHER = 9;
    var predicates = new List<Func<PhoneNumbers, bool>>()
    {
        p => p.sms_capable == 1 && p.last_used_for_rx == 1,
        p => p.sms_capable == 1 && p.phone_type_id == PHONE_TYPE_OTHER,
        p => p.sms_capable == 1 && p.phone_type_id == PHONE_TYPE_HOME,
        p => p.sms_capable == 1 && p.phone_type_id == PHONE_TYPE_OFFICE
    }; // Can be moved to static field

    // prevent potential multiple materialization of the source
    var enumerated = patientNumbers as ICollection<PhoneNumbers> ?? patientNumbers.ToArray();
    foreach (var predicate in predicates)
    {
        var firstOrDefault = enumerated.FirstOrDefault(predicate);
        if (firstOrDefault is not null)
        {
            return firstOrDefault.phone_number;
        }
    }

    return string.Empty;
}

Also in this particular case you can "prefilter" the enumerated with .Where(p => p.sms_capable == 1) to improve performance a bit:

// ...
var enumerated = patientNumbers
    .Where(p => p.sms_capable == 1)
    .ToArray();
var predicates = new List<Func<PhoneNumbers, bool>>()
{
    p => p.last_used_for_rx == 1,
    p => p.phone_type_id == PHONE_TYPE_OTHER,
    p => p.phone_type_id == PHONE_TYPE_HOME,
    p => p.phone_type_id == PHONE_TYPE_OFFICE
};
// ...

This is a nice solution! I wish to inquire on your comment. How is multiple iteration possible when not manifesting the collection first via .ToArray()? — jrandomuser, Feb 18 '23 at 22:16
@jrandomuser `IEnumerable` can be anything including not yet materialized query (to the database for example), so every materialization operation will perform the IEnumerable recomputation (i.e. in case of db access - a new call to db to fetch the same data) — Guru Stron, Feb 18 '23 at 22:21
@jrandomuser check out [this](https://stackoverflow.com/a/61639290/2501279) and [this](https://stackoverflow.com/a/75400131/2501279) answers for details. — Guru Stron, Feb 18 '23 at 22:24
Is the logic preserved if I replace the foreach loop with something like this: return predicates.Select(p => enumerated.FirstOrDefault(p))?.FirstOrDefault()?.phone_number ?? String.Empty; — jrandomuser, Feb 18 '23 at 22:29
@jrandomuser almost, you need to add filter for nulls after select. — Guru Stron, Feb 18 '23 at 22:30
@jrandomuser `predicates.Select(p => enumerated.FirstOrDefault(p)).Where(p => p is not null)...` — Guru Stron, Feb 18 '23 at 22:34

score 0 · Answer 2 · answered Feb 18 '23 at 22:17

This isnt using linq, but you can refactor this by putting some of the complexity into their own methods

private IEnumerable<IGrouping<int, PhoneNumbers>> GetSmsCapablePhoneNumbersByType(IEnumerable<PhoneNumbers> patientNumbers)
{
    return patientNumbers.Where(p => p.sms_capable == 1).GroupBy(p => p.phone_type_id);
}

private PhoneNumbers GetLastUsedSmsNumber(IEnumerable<PhoneNumbers> patientNumbers)
{
    return patientNumbers.FirstOrDefault(p => p.sms_capable == 1 && p.last_used_for_rx == 1);
}

private PhoneNumbers GetFirstSmsNumberByType(IEnumerable<PhoneNumbers> patientNumbers, int phoneTypeId)
{
    return patientNumbers.FirstOrDefault(p => p.sms_capable == 1 && p.phone_type_id == phoneTypeId);
}

public string GetDefaultSMSPhoneNumber(IEnumerable<PhoneNumbers> patientNumbers)
{
    var phoneNumberByType = GetSmsCapablePhoneNumbersByType(patientNumbers);

    var lastUsedSmsNumber = GetLastUsedSmsNumber(patientNumbers);
    if (lastUsedSmsNumber != null)
    {
        return lastUsedSmsNumber.phone_number;
    }

    var defaultSmsNumber = GetFirstSmsNumberByType(patientNumbers,  PHONE_TYPE_OTHER)
                           ?? GetFirstSmsNumberByType(patientNumbers, PHONE_TYPE_HOME)
                           ?? GetFirstSmsNumberByType(patientNumbers, PHONE_TYPE_OFFICE);
    if (defaultSmsNumber != null)
    {
        return defaultSmsNumber.phone_number;
    }

    return string.Empty;
}

If you do it correctly, your method names should describe exactly whats happening, so when somone else reads your code they should be able to follow whats happening by reading the method names (This also means there is less need for comments)

jrandomuser · Accepted Answer · 2023-03-13T13:53:41.153

I felt like there were a lot of good answers to this question. I think though that this is perhaps the most efficient method as far as readability and number of iterations.

If I'm not mistaken this will materialize / iterate once check if there was a last used number. If so return. So minimum number of materialization / iteration is 1.

The .GroupBy on configuredNumbers should cause a materialization / iteration. After that, the OrderBy, SelectMany, and FirstOrDefault are all off the already materialized collection. There may be another iteration for the Sort / Transformation but I believe this is against the model materialized in memory for the GroupBy.

While I like the other solutions presented here, the separate predicates methods seems to me to add additional materializations / iterations of the data.

One complication to this is that the Phone Type Values can't be ordered as their values don't represent the desired priority. This can be handled with either a switch or a dictionary map I've done here with phoneTypeSortOrder. The Phone Types were moved from const's to an Enum but the values were preserved. By incorporating this translation into our GroupBy we are able to use OrderBy on our new key.

        string GetDefaultSMSPhoneNumber(IEnumerable<PhoneNumbers> patientNumbers)
        {
            var phoneTypeSortOrder = new Dictionary<int, int> { { (int)PhoneType.Other, 1 }, { (int)PhoneType.Home, 2 }, { (int)PhoneType.Office, 3 } };
            var smsNumbers = patientNumbers.Where(p => p.sms_capable == 1);              
            var lastUsedNumber = smsNumbers.FirstOrDefault(p => p.last_used_for_rx == 1);
            if (lastUsedNumber != null) 
                return lastUsedNumber.phone_number; 
            // If no number has been used, select a configured SMS number in the following order (Other, Home, Office)             
            var configuredNumbers = smsNumbers.Where(x => phoneTypeSortOrder.ContainsKey(x.phone_type_id))
                .GroupBy(p => phoneTypeSortOrder[p.phone_type_id])
                .OrderBy(g => g.Key)                
                .SelectMany(g => g)
                .FirstOrDefault();
            return configuredNumbers?.phone_number ?? string.Empty; 
        }

Refactor and reduce cyclomatic complexity with LINQ

3 Answers3