How to unify/simplify this code - event handling/delegating?

Question

I have Process objects that are monitored from two different views. A Windows.Forms.ListView (actually a derived class) and a Graph Viewer (based on Microsoft Research's Automatic Graphing Layout). Each has a context menu which can have similar events activated. While the list view can have multiple selections, I don't allow it on the graph view.

This is what I currently have:

    private void ctxgrphAddBreakpoint_Click(object sender, EventArgs e)
    {
        Process p = GetProcess(viewer);
        if (p != null)
        {
            p.AddBreakpoint();
            BeginRefresh(false, false);
        }
    }

    private void ctxgrphRemoveBreakpoint_Click(object sender, EventArgs e)
    {
        Process p = GetProcess(viewer);
        if (p != null)
        {
            p.RemoveBreakpoint();
            BeginRefresh(false, false);
        }
    }

    private void ctxlistAddBreakpoint_Click(object sender, EventArgs e)
    {
        foreach (Process p in lvwProcessList.SelectedProcesses())
        {
            p.AddBreakpoint();
        }
        BeginRefresh(false, false);
    }

    private void ctxlistRemoveBreakpoint_Click(object sender, EventArgs e)
    {
        foreach (Process p in lvwProcessList.SelectedProcesses())
        {
            p.RemoveBreakpoint();
        }
        BeginRefresh(false, false);
    }

I'd like to unify the two context menus into one and the event handling into one like this:

    private void ctxlistAction_Click(object sender, EventArgs e)
    {
        // I can unify the Viewer and ListView and implement some common interface,
        // so I'm pretty sure I can handle this part
        foreach (Process p in UIView.SelectedProcesses())
        {
            p.Action(); // What is the best way to handle this?
        }
        BeginRefresh(false, false);
    }

How do I get there?

Answer 1

Locate the event assignment ( ... += new System.EventHandler(ctxlistAction_Click); ) in the *.Designer.cs file, and make them point to the same function.

Answer 2

Visitor pattern?

Answer 3

Can't you just cast the sender to your interface?

 private void ctxlistAction_Click(object sender, EventArgs e)
 {
      UIView view = sender as UIView;

      if (view != null)
      {

         // I can unify the Viewer and ListView and implement some common interface,
         // so I'm pretty sure I can handle this part
         foreach (Process p in view.SelectedProcesses())
         {
             p.Action(); // What is the best way to handle this?
         }
         BeginRefresh(false, false);
      }
  }

Answer 4

First, try refactoring out the common functionality and using delegates (please excuse my poor choice of names like "DoListAction"):

readonly Action<Process> removeBreakpoint = p => p.RemoveBreakpoint();
readonly Action<Process> addBreakpoint = p => p.AddBreakpoint(); 

void DoSingleAction(Action<Process> action)
{
     var p = GetProcess(viewer);
     if(p != null)
     {
         action(p); //invokes the action
         BeginRefresh(null,null);
     }
}

void DoListAction(Action<Process> action)
{
    lvwProcessList.ForEach(action);
    BeginRefresh(false, false);
}



private void ctxgrphAddBreakpoint_Click(object sender, EventArgs e)
{
    DoSingleAction(addBreakpoint);
}

private void ctxgrphRemoveBreakpoint_Click(object sender, EventArgs e)
{
    DoSingleAction(removeBreakpoint);
}

private void ctxlistAddBreakpoint_Click(object sender, EventArgs e)
{
    DoListAction(addBreakpoint);
}

private void ctxlistRemoveBreakpoint_Click(object sender, EventArgs e)
{
    DoListAction(removeBreakpoint);
}

then you can unify DoProcessAction and DoListAction:

void DoAction(object sender, Action<Process>)
{
     if(sender is ListView)
     {
         lvwProcessList.ForEach(action);
         BeginRefresh(false, false);
     }
     else if (sender is GraphView)
     {
         var p = GetProcess(viewer);
         if(p != null)
         {
             action(p); //invokes the action
             BeginRefresh(null,null);
         }
     }
     else {throw new Exception("sender is not ListView or GraphView");}
}

//and update all the handlers to use this, and delete DoSingleAction and DoListAction:
private void ctxgrphAddBreakpoint_Click(object sender, EventArgs e)
{
    DoAction(sender, addBreakpoint);
}
//etc.

No matter what, in each event handler, I think you're gonna have to specify what action to take. I'm not sure if inheritance or extension methods will really be your friend in this case, but here's how it would probably look using extension methods:

//these two are in a separate static class
public static InvokeAction(this ListView listView, Action<Process> action)
{ ... }
public static InvokeAction(this GraphView listView, Action<Process> action)
{ ... }

private void handler(object sender, Action<Process> action)
{ 
    var lv = sender as ListView;
    var gv = sender as GraphVeiw;    
    lv.InvokeAction(action); //(need to check for null first)
    gv.InvokeAction(action);
    if(lv == null && gv == null) {throw new Exception("sender is not ListView or GraphView");}
}

//then update all the handlers:
private void ctxgrphAddBreakpoint_Click(object sender, EventArgs e)
{
    handler(sender, addBreakpoint);
}

C#2 / .NET 2.0

To do this in C#2 (VS2005), the first way using delegates will still work, you just can't have the lambdas. (2.0 has the Action<T> delegate). Just change the lambdas to functions. Everything else will work correctly.

void removeBreakpoint(Process p) { p.RemoveBreakpoint(); }
void addBreakpoint(Process p) { p.AddBreakpoint(); }

//you could also do this, but it's overkill:
readonly Action<Process> removeBreakpoint = delegate(Process p) { p.RemoveBreakpoint(); };
readonly Action<Process> addBreakpoint = delegate(Process p) { p.AddBreakpoint(); };