Filtering a list by comparing enums against a user choice

Question

I have a list of objects where one of the properties on the object class is an enum.

The program below simply loops through each item in the list, checks an int value, then checks the enum value for that item, then adds the item to another list when the condition is met.

I am using if-else statements, I attempted to use a switch statement but it doesn't seem possible to place 2 constants on a single case.

I'd like to know if there's a better / more elegant way to do this or is what I've done sufficient in this scenario?

Program Code

// User inputs number from 1 - 5        
int SelectedOppType = Int32.Parse(Console.ReadLine());

foreach (Opportunities Opp in OpportunitiesList)
{
    if (SelectedOppType == 1 && Opp.OpportunityStatusID == OpportunityStatus.Active) // Opp.OpportunityStatusID == OpportunityStatus.Draft
    {
        FilterdOppsList.Add(Opp);
    }
    else if (SelectedOppType == 2 && Opp.OpportunityStatusID == OpportunityStatus.Draft)
    {
        FilterdOppsList.Add(Opp);
    }
    else if (SelectedOppType == 3 && Opp.OpportunityStatusID == OpportunityStatus.Closed)
    {
        FilterdOppsList.Add(Opp);
    }
    else if (SelectedOppType == 4 && (Opp.OpportunityStatusID == OpportunityStatus.Active || Opp.OpportunityStatusID == OpportunityStatus.Draft))
    {
        FilterdOppsList.Add(Opp);
    }
    else if (SelectedOppType == 5)
    {
        FilterdOppsList.Add(Opp);
    }

}

foreach (var OppItem in FilterdOppsList)
{
    Console.WriteLine(OppItem.OppText);
}

Console.ReadLine();

Answer 1

Shortening ifs

You add everything to the same list so you can concatenate all conditions and use only one if with a helper variable:

foreach (Opportunities Opp in OpportunitiesList)
{
    var canAdd =
        (SelectedOppType == 1 && Opp.OpportunityStatusID == OpportunityStatus.Active) ||
        (SelectedOppType == 2 && Opp.OpportunityStatusID == OpportunityStatus.Draft) ||
        (SelectedOppType == 3 && Opp.OpportunityStatusID == OpportunityStatus.Closed) ||
        (SelectedOppType == 4 && (Opp.OpportunityStatusID == OpportunityStatus.Active || Opp.OpportunityStatusID == OpportunityStatus.Draft)) ||
        (SelectedOppType == 5);

    if (canAdd)
    {
        FilterdOppsList.Add(Opp);
    }
}

---

Other improvements

Magic numbers

You should create an enum for the SelectedOppType and cast the int given by the user to it.

Example:

enum OppType
{
    OppType1 = 1,
    OppType2 = 2,
    OppType3 = 3,
    OppType4 = 4,
    OppType5 = 5,
}

var selectedOppType = (OppType)Int32.Parse(Console.ReadLine());

I know the names OppTypeX are ugly but I don't know what the numbers mean, this is why we dislike them. They have no meaning.

Next, the logic can be extracted as an easier to maintain dictionary. Now that all values are enums we can easily build such a dictionary that I find by far is better then a switch nested in a loop. Besides, you can reuse the dictionary to display the options in other places (if you make it a filed or a property of some class).

var allowedOppTypeStatuses = new Dictionary<OppType, IEnumerable<OpportunityStatus>>
{
    [OppType.OppType1] = new[] { OpportunityStatus.Active },
    [OppType.OppType2] = new[] { OpportunityStatus.Draft },
    [OppType.OppType3] = new[] { OpportunityStatus.Closed },
    [OppType.OppType4] = new[] { OpportunityStatus.Active, OpportunityStatus.Draft },
    [OppType.OppType5] = Enumerable.Empty<OpportunityStatus>(),
};

foreach (Opportunities opp in OpportunitiesList)
{
    var canAdd = 
        allowedOppTypeStatuses[selectedOppType].Any(x => x == opp.OpportunityStatusID) || 
        !allowedOppTypeStatuses[selectedOppType].Any();

    if (canAdd)
    {
        FilterdOppsList.Add(opp);
    }
}

Minor issues

• We use camelCase for local variables so SelectedOppType should be selectedOppType
• Opportunities - this doesn't look like a good name for a type, there seems to be more wrong with your code (unless it's an enum with Flags attribute).

Answer 2

When SelectedOppType == 5, all values in OpportunitiesList will be added to FilterdOppsList, but only after evaluating many conditions unnecessarily. It would be better to handle this as a special case, and add all values of OpportunitiesList in FilterdOppsList before ever entering the foreach loop.

Next, in the foreach loop, since selectedOppType doesn't change throughout the loop, regardless of the value of opp, it would make more sense to use conditionals on the value of opp.OpportunityStatusID instead of selectedOppType. You can rearrange the conditions to switch statement.

Putting it all together:

if(selectedOppType == 5)
{
    FilterdOppsList.AddRange(OpportunitiesList);
}
else
{
    foreach (Opportunities opp in OpportunitiesList)
    {
        bool shouldAdd = false;
        switch (opp.OpportunityStatusID)
        {
            case OpportunityStatus.Active:
                 shouldAdd = selectedOppType == 1 || selectedOppType == 4;
                 break;
            case OpportunityStatus.Draft:
                 shouldAdd = selectedOppType == 2 || selectedOppType == 4;
                 break;
            case OpportunityStatus.Closed:
                 shouldAdd = selectedOppType == 3;
                 break;
        }
        if (shouldAdd)
        {
            FilterdOppsList.Add(opp);
        }
    }
}