current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

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")));
}
share|improve this question

1 Answer 1

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; // :)
share|improve this answer
    
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

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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