5
\$\begingroup\$

In our software we enable certain features based on the existence of a flag. The Flags class is a static class used to determine the existence of such a flag.

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:

  1. Get all menu items.
  2. Return a subset of those menu items by using the following rule:
    • Some menu items are tied to a specific flag that is expected to be hard-coded in some way. The menu item name may not match the associated flag.
    • Any other menu items should be displayed.

Any help would be much appreciated.

private IEnumerable<MenuItemDefinition> GetMenuItems()
{
     return GetAllMenuItems().Where(mi => 
          (mi.MethodName != "NewWidget" || Flags.HasFlag("Feature1")) &&
          (mi.MethodName != "NewDongle" || Flags.HasFlag("Feature2")));
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

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; // :)
\$\endgroup\$
5
  • \$\begingroup\$ Hi @Lars-Erik, thanks for the answer. I've provided additional information in the example that answers your question, I hope that is helpful. \$\endgroup\$ Jan 25, 2012 at 17:00
  • \$\begingroup\$ I got it, entirely new answer. ;) \$\endgroup\$
    – Lars-Erik
    Jan 25, 2012 at 18:55
  • \$\begingroup\$ 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. \$\endgroup\$ Jan 25, 2012 at 19:08
  • \$\begingroup\$ 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. \$\endgroup\$
    – Lars-Erik
    Jan 25, 2012 at 19:28
  • \$\begingroup\$ 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! \$\endgroup\$ Jan 25, 2012 at 20:17

Your Answer

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

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