Lambda .Where() expression to return selected/flagged objects

Question

In our software we enable certain features based on the existence of a flag in the Windows Registry. We have a global method called FeatureIsEnabled(string propertyName) that checks the registry for a particular feature name, which is hard-coded in the software.

I came across one piece of logic that I wrote that I feel could benefit from refactoring. The boolean logic works, but I think it's difficult to understand or read because it relies very heavily on short-circuiting.

The basic logic is:

Get all menu items.
Return a subset of those menu items by using the following rule:
- Some menu items are tied to a specific enable-able feature. We check whether a feature is enabled in the registry by calling FeatureIsEnabled(string featureName).
- Any other menu items by default are enabled, and should be displayed.

Any help would be much appreciated.

private IEnumerable<MenuItemDefinition> GetMenuItems()
{
     return GetAllMenuItems().Where(mi => 
          (mi.MethodName != "NewWidget" || FeatureIsEnabled("Widgets")) &&
          (mi.MethodName != "DeleteWidget" || FeatureIsEnabled("Widgets")) &&
          (mi.MethodName != "NewDongle" || FeatureIsEnabled("Dongles")) &&
          (mi.MethodName != "DeleteDongle" || FeatureIsEnabled("Dongles")));
}

Update: Updated example code and further information

I updated the sample code to be a bit cleaner. Also, here is the sample result of a GetAllMenuItems() call, which is processed by the code block above in GetMenuItems():

new [] 
{
    new MenuItemDefinition() { MethodName = "NewWidget" },
    new MenuItemDefinition() { MethodName = "DeleteWidget" },
    new MenuItemDefinition() { MethodName = "Save" },
    new MenuItemDefinition() { MethodName = "Close" },
    new MenuItemDefinition() { MethodName = "NewDongle" },
    new MenuItemDefinition() { MethodName = "DeleteDongle" }
}

If the feature "Dongles" is enabled, then the method should return Save, Close, NewDongle, and DeleteDongle.

The goal of refactoring GetMenuItems is to not rely on inlining a huge number of confusing boolean logic, and instead coming up with a construct that is easier to follow. Currently, have to explain to the casual observer the curious combination of the !=, ||, and &&. I think a simpler method exists, I'm just not sure what.

Lars-Erik · Accepted Answer · 2012-01-25 19:27:53Z

up vote 2 down vote accepted

If you control the MenuItemDefinition class, why not just add a feature tag to it?

public class MenuItemDefinition
{
    public string MethodName { get; set; }
    public string Feature { get; set; }
}

// each instance
new MenuItemDefinition { MethodName = "NewWidget", Feature = "Widgets" }

// GetMenuItems():
return GetAllMenuItems().Where(m => FeatureIsEnabled(m.Feature));

If not, you could put them in a dictionary:

var items = new Dictionary<string, MenuItemDefinition[]> {
    { "Widgets", new[] {
        new MenuItemDefinition { MethodName = "NewWidget" },
        new MenuItemDefinition { MethodName = "DeleteWidget" }
    },
    { "Default", new[] {
        // ....
    }
}

return items.Where(pair => FeatureIsEnabled(pair.Key)).SelectMany(pair => pair.Value);

Or better yet, build the initial list based upon content in a dictionary. That way you don't run the risk of having typos in the switch or the initial setup:

var dict = new Dictionary<string, string[]> 
{
    { "Widgets", new[] { "NewWidget", "DeleteWidget" },
    { "Default", new[] { "..." }
    ...
}

var menuItems = dict
    .Where(p => FeatureIsEnabled(p.Key))
    .SelectMany(p => p.Value)
    .Select(mn => new MenuItemDefinition { MethodName = mn });

// GetMenuItems()
return menuItems; // :)

edited Jan 25 '12 at 19:27

answered Jan 25 '12 at 9:09

Lars-Erik
1,12429

Hi @Lars-Erik, thanks for the answer. I've provided additional information in the example that answers your question, I hope that is helpful. – Kevin McCormick Jan 25 '12 at 17:00

I got it, entirely new answer. ;) – Lars-Erik Jan 25 '12 at 18:55

Thanks for the updated response. I'm not able to make changes to the MenuItemDefinition class, but if I were able to, this would be the best solution. For reference, I've posted my solution separately as well. – Kevin McCormick Jan 25 '12 at 19:08

Oh no, not the switch! It's evil. ;) At least pull it out and hide it in a method as I first described. Anyway, I added a third proposal, which I hope appeals. – Lars-Erik Jan 25 '12 at 19:28

Ah yes, switch is very evil indeed! The GetMenuItems() method is in fact the method that hides the implementation so we are already in a hidden place for such evil things. That being said, I like the dictionary approach even more! – Kevin McCormick Jan 25 '12 at 20:17

Kevin McCormick · Answer 2 · 2012-01-25 19:13:39Z

For reference, here's what I went with:

private IEnumerable<MenuItemDefinition> GetMenuItems()
{
    return GetAllMenuItems().Where(mi => 
            {
                switch(mi.MethodName)
                {
                    case "NewWidget":
                    case "DeleteWidget":
                        return FeatureIsEnabled("Widgets");
                    case "NewDongle":
                    case "DeleteDongle":
                        return FeatureIsEnabled("Dongles");
                    default:
                        return true;
                }
            });
}

The list of features tends to be pretty small, so the switch/case is an easy-to-understand solution.

asked	1 year ago
viewed	4694 times
active	1 year ago

Lambda .Where() expression to return selected/flagged objects

2 Answers

Your Answer

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

Lambda .Where() expression to return selected/flagged objects

2 Answers

Your Answer

Sign up or login

Post as a guest

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

Related