How to make this code more DRY?

Question

Let's say we have these checkboxes:

• FooCheckBox
• BarCheckBox
• BazCheckBox

And these methods:

• Foo
• Bar
• Baz

I want to call each method only if the correponding checkbox is checked. The code might look like this:

void DoWork()
{
    if (FooCheckBox.Checked)
    {
        Foo();
        Console.WriteLine("Foo was called");
    }

    if (BarCheckBox.Checked)
    {
        Bar();
        Console.WriteLine("Bar was called");
    }

    if (BazCheckBox.Checked)
    {
        Baz();
        Console.WriteLine("Baz was called");
    }
}

Now consider that instead of 3 checkboxes and 3 methods you have a lot more. How would you rewrite the code above to make it more DRY?

Answer 1

I would say for the case you presented, leave it as is; you don't want to over-abstract without having a good reason, as it can make a code base less maintainable. Of course context matters though, and it's ultimately a judgement call.

That said, here's how I would approach this. Create a collection where each item contains both the control and the action delegate. Then loop through and perform the logic on each item.

var items = new KeyValuePair<CheckBox, Action>[] {
    new KeyValuePair<CheckBox,Action>(FooCheckBox, Foo),
    new KeyValuePair<CheckBox,Action>(BarCheckBox, Bar),
    new KeyValuePair<CheckBox,Action>(BazCheckBox, Baz)
};

foreach (var item in items)
{
    if (item.Key.Checked) 
    {
        item.Value.Invoke();
        Console.WriteLine("Invoked " + item.Value.Method.Name);
    }
}

Or (possibly?) better using Linq:

items.Where(item => item.Key.Checked).ToList().ForEach(item => new {
    item.Value.Invoke();
    Console.WriteLine("Invoked " + item.Value.Method.Name);
});

Answer 2

You could use a Dictionary to keep up with what actions refer to which checkboxes. Then you could do the following:

foreach(KeyValuePair<CheckBox, Action> kvp in Dict)
{
    if(kvp.Key.Checked)
        kvp.Value.Invoke();
}

Answer 3

for simplicity I would go with

void DoWork()
{
    DoIfChecked(FooCheckBox, Foo, "Foo as Called");
    DoIfChecked(BarCheckBox, Bar, "Bar as Called");
    DoIfChecked(BazCheckBox, Baz, "Baz as Called");
}
void DoIfChecked(CheckBox checkBox, Action action, string message)
{
    if (checkBox.IsChecked)
    {
        action();
        Console.WriteLine(message);
    }
}

but you could do something with the message part if it is that simple, and I might throw in some error checking depending on local context.

Answer 4

It could be done in the following way:

void DoWork()
{
    Func<Action, string, Tuple<Action, string>> toT = 
        (a, s) => new Tuple<Action, string>(a, s);

    var map = new Dictionary<CheckBox, Tuple<Action, string>>
    {
        {FooCheckBox, toT(Foo, "Foo")},
        {BarCheckBox, toT(Bar, "Bar")},
        {BazCheckBox, toT(Baz, "Baz")},
    };

    foreach (var x in map.Keys)
        if (x.Checked)
        {
            map[x].Item1();
            Console.WriteLine(map[x].Item2 + " was called");
        }
}

But I think that sometimes being not very DRY is just ok.