Handling Enum flags for a copy operation

Question

I've just set up a bit of code to try and make things neater, but all I've really done, is add a whole bunch of nested if statements:

public void Copy(Copy copyType)
{    
    FrmProjectChooser frm = null;
    if (copyType.HasFlag(Copy.Single))
    {
        if (copyType.HasFlag(Copy.Estimations) && copyType.HasFlag(Copy.SubItems))
        {
            // Single item, with sub items and estimations
            List<WB> items = new List<WB> { (WB)treeWBS.GetDataRecordByNode(treeWBS.FocusedNode) };
            items.AddRange(_WBSManager.GetAllChildren(items[0].idWBS));
            List<CTREstimation> estimations = new List<CTREstimation>();
            foreach (WB wbs in items)
                estimations.AddRange(CTREstimationsManager.Instance.GetEstimationsForWBS(wbs.idWBS));
            frm = new FrmProjectChooser(items, estimations);
        }
        else if (copyType.HasFlag(Copy.Estimations))
        {
            // Single item, with estimations.
            WB wbsItem = treeWBS.GetDataRecordByNode(treeWBS.FocusedNode) as WB;
            if (wbsItem != null)
                frm = new FrmProjectChooser(new List<WB> { wbsItem },
                    CTREstimationsManager.Instance.GetEstimationsForWBS(wbsItem.idWBS));
        }
        else if (copyType.HasFlag(Copy.SubItems))
        {
            // Single item, with sub items
            List<WB> items = new List<WB> { (WB)treeWBS.GetDataRecordByNode(treeWBS.FocusedNode) };
            items.AddRange(_WBSManager.GetAllChildren(items[0].idWBS));
            frm = new FrmProjectChooser(items);
        }
        else
        {
            // Single item
            WB wbsItem = treeWBS.GetDataRecordByNode(treeWBS.FocusedNode) as WB;
            if (wbsItem != null)
                frm = new FrmProjectChooser(new List<WB> {wbsItem});
        }
    }
    else if (copyType.HasFlag(Copy.All))
    {
        if (copyType.HasFlag(Copy.Estimations))
        {
            // All items, with estimations.
            List<WB> wbsItems = _WBSManager.GetWBSForProject(CurrentProject.idProjects);
            List<CTREstimation> estimations = new List<CTREstimation>();
            foreach (WB wbs in wbsItems)
                estimations.AddRange(CTREstimationsManager.Instance.GetEstimationsForWBS(wbs.idWBS));
            frm = new FrmProjectChooser(wbsItems, estimations);
        }
        else
        {
            // All items
            frm = new FrmProjectChooser(_WBSManager.GetWBSForProject(CurrentProject.idProjects));
        }
    }

    if (frm != null)
        frm.ShowDialog();
}

[Flags]
private enum Copy
{
    None = 0,
    Single = 1, // Straight copy of single item
    SubItems = 1 << 1, // Include any children in the copy
    Estimations = 1 << 2, // Include estimations in the copy
    All = 1 << 3 // Copy all items
}

The code will pull "Items" from a TreeList. "Sub Items" are of the same type as "Items", but Estimations are a different type. I don't think it's very neat. I've tried googling similar titles to this question but I can't really find anything that helps.

Is there anything I can do to make this neater?

Answer 1

This answer is based on what @Matt said. I thought the idea was very good and it could be pushed a step further.

Using @Matt's idea, your code wouldn't contain nested ifs, but you still chain ifs. Of course you could use a switch, but you still have the same problem. Lots of code in the same method, and we don't like crowded methods.

The solution? (Well.. my solution)

A dictionary!

Now, I don't know what your doing inside your ifs, which is sad, because I can't help you much further. But it would look like this :

//Right now, I assume your method is in an object, so I put the dictionary static, and it will be populated in the static ctor of the object.
private static readonly Dictionary<Copy,Action> _mapCopy = new Dictionary<Copy,Action>();

static YourObjectWhatever()
{
    _mapCopy.Add(Copy.All  | Copy.Estimations, CopyAllEstimations);
    _mapCopy.Add(Copy.Single | Copy.Estimations, CopySingleEstimations);
    //etc...
}

private static void CopyAllEstimations()
{
    //Do stuff..
}

private static void CopySingleEstimations()
{
    //Do stuff..
}

public void Copy(Copy copyType)
{
    Action action = null;
    if(!_mapCopy.TryGetValue(copyType, out action))
        throw new ArgumentException("The copy type doesn't have an associated action");

    action();
}

Now, your logic is separated in multiple methods, which is better but not perfect. If you want to push this even farther (for testability for example), you'd need to implement an interface, let's say ICopyAction (I can't tell more because there's a little lack of context). So now we'd have :

public interface ICopyAction
{
    void Copy(/*I'm guessing you'd have parameters*/);
}

public class CopyAllEstimations : ICopyAction
{
    public void Copy()
    {
        //do stuff
    }
}
//etc...

And the usage :

//Same assumption
private static readonly Dictionary<Copy,ICopyAction> _mapCopy = new Dictionary<Copy,ICopyAction>();

static YourObjectWhatever()
{
    _mapCopy.Add(Copy.All  | Copy.Estimations, new CopyAllEstimations());
    //etc...
}

public void Copy(Copy copyType)
{
    ICopyAction action = null;
    if(!_mapCopy.TryGetValue(copyType, out action))
        throw new ArgumentException("The copy type doesn't have an associated action");

    action.Copy();
}

This method works as well if you want to have parameters to your method, but I didn't this explanation because I don't know if you need it..

Answer 2

For an aesthetic point of view, you could define some constants:

AllEstimations = Copy.All | Copy.Estimations
SingleEstimations = Copy.Single | Copy.Estimations
etc. 

You could then check the value against these constants and create a single level if statement:

if (copyType == SingleEstimationsSubItems)
{}
elseif (copyType == SingleEstimations)
{}
elseif (....

I'm not claiming this will improve performance, but it will make the code more readable.