2
\$\begingroup\$

I need some second or third eyes to look over this, since right now, the necessary actions to make this work sound just bad and I suppose I am missing something due to a lack of C# experience.

I am currently working on a winforms c# application. More specifically, I am reworking a highly convoluted control flow between a custom winforms widget and the corresponding controller.

To nail down some terms, the widget is called a DrawingPanel. On the drawing panel, the user can place certain components, which can be considered orange squares for now. He can select single components by clicking on them with the left button, he can select multiple components by dragging a box around them. Once he has components selected, he can right click and select operations like cut, paste, copy and so on with the obvious semantics. For usability reasons, he can also use these operations on a single component without selecting it.

In the old state, the control flow between the DrawingPanel and the so-called EventManager mostly looks like two angry cats wrestling in mud, as it goes back and forth about 4 - 6 times in order to deduce the set of selected components. Somewhere in there, they are highlighted, but after an hour, I gave up deducing that for now, especially because there are about 2 dead highlighting functions in there. Since I have to add quite a bit of funcionality here, I guess it is up to me to redo this.

In order to redo this, I figured a good way to straighten this out would be the following:

I add one event per user command to the DrawingPanel. The Controller of the DrawingPanel subscribes to these events and modifies the model accordingly in the event handler. For example, if a set of components is selected and the user clicks on "Copy" in the context menu, a CopyRequest-Event is raised which has this set of arguments as an argument. If a user clicks on "Paste" in the component menu, a PasteRequest-Event is raised, which is parametrized by the position of the paste request.

This should encapsulate the user interface, as the interface is reduced to a black box which is parametrized by the data model and raises events from the user. The EventManager could handle all user interfaces, which provide the appropiate events. Furthermore, the control flow would be greatly simplified, as it would become unidirectional: UI goes to Controller goes to Model.

Now, onto my problem. I have some very heavy code duplication in my event handlers in the DrawingPanel. A ton of event handlers subscribe to the Click-Event of a context menu, wrap the currently selected components into an ComponentSetArgument-object and fire the request-event with this new argument (if its not null). I am failing to remove this duplication.

So far, I have tried to implement a higher order event handler generator (which would take a component set event and generate an evenhandler to subscribe at the click-event) and a mapping function on events (which would handle the click-event, apply a function and signal the outgoing event). Both of these fail because the multiway delegates are immutable. Thus, correct functionality would depend on creating the mapping or calling the wrap+call-higher order function after all subscribers are subscribed to the event. This yells "recipe for disaster" for me.

So, is there anything I can do beyond either tolerating this massive duplication or rolling my own subscription management there? Or, do I take the totally wrong way there and there is a massively better architecture here?

Edit 1:

I have been asked for sample code. I will try my best to condense the code without removing essential information. I will use the current state of code, which has event-based, improved control flow, but a heavy smell due to duplication.

These are the significant portions of the DrawingPanel.

// IComponent is something of the model. This class moves them around as black boxes
public class ComponentSetEventArguments { public ISet<IComponent> Components; } 
public delegate void ComponentSetHandler(object sender, ComponentSetEventArguments args);

public partial class DrawingPanel {
    // These events signal an action of the user to the controller.
    // In order to maintain brevity and demonstrate the problem, I 
    // include just these 2. There are about 6 ComponentSetHandlers
    // and various other handlers, for example the mentioned PasteRequest
    // with a PositionEventArgument.
    // These events closely mimic the events found in actual WinForms-classes,
    // like ToolStripMenuItem.Click or IPanel.MouseDown. 
    public event ComponentSetHandler CopyRequest;
    public event ComponentSetHandler CutRequest;

    // this contains components which were selected by the user. This happens
    // by clicking on a single component or dragging a box around these
    // components. This component and the following as well as selecing the
    // correct context menu to display are done in mouseDown/mouseUp-Handlers,
    // which are mostly big if-else-chains, so I won't show them. 
    ISet<IComponent>() selectedComponents;


    // this contains the last component hovered, so you can use the context menu on
    // a component by just right-clicking on top of it if no other components are
    // selected
    IComponent lastHoveredComponent;

    public DrawingControl() {
        ...
        // Let us look at the names first. These are toolstrips in 
        // the componentSetContextMenu. This context menu pops up if the user
        // right-clicks while more than one component is selected. I have to
        // differntiate between "1 component selected" and "more than 1 component
        // selected", because certain operations are only possible for multiple
        // components. Second, the "cutIn..." means it is the toolstrip item
        // with the text cut on it. 
        this.copyInComponentSetContextMenu.Click += copyComponentSetHandler;
        this.cutInComponentSetContextMenu.Click += cutComponentSetHandler;

        // note the difference, this is just in the component context menu,
        // not the component set context menu.
        this.cutInComponentContextMenu.Click += cutComponentHandler;

        ...
    }

    // SMELL: Duplication (I.I)
    // these two somethingComponentSetHandlers just wrap the selected components
    // into event arguments and fire the corresponding user request. 
    void copyComponentSetHandler(object sender, EventArgs e) {
        if (CopyRequest == null) return;
        // im just omitting safety copies here.
        ComponentSetArguments a = new ComponentSetArguments(selectedComponents);
        CopyRequest(this, a);
    }

    // SMELL: Duplication (I.II)
    void cutComponentSetHandler(object sender, EventArgs e) {
        if (CutRequest == null) return;
        ComponentSetArgument a = new ComponentSetARguments(selectedComponents);
        CutRequest(this, a);
    }

    // This handler is mostly included as a demonstration why I think this is
    // a decent approach. It encapsulates the difference of handling a single
    // component in a convenient way in the UI itself. Further objects do not
    // need to consider this minor detail. 
    void cutComponentHandler(object sender, EventArgs e) {
        if (CopyRequest == null) return;
        ISet<IComponent> components;
        // if the user selected a single component, cut that component.
        // if the user selected no components, cut the component he pointed at.
        if (selectedComponents.Count == 0) {
            components = new HashSet<IComponent>();
            components.Add(lastHoveredComponent);
        } else {
            components = selectedComponents;
        }
        CutRequest(this, components);
    }
}

I am pleasently surprised that it looks like the EventManager is not relevant for this problem. For completeness, the EventManager has a method that subscribes to the CutRequest event. When the CutRequest is fired, the EventManager copies the components from the event argument into a separate collection and removes the cut components from the model itself.

Given all that, the smell I am annoyed by becomes apparent, I have marked them as smell I.I and I.II. These methods differ in exactly one thing. The event to signal, CutRequest and CopyRequest and this continues through a number of more methods.

My current fixes generally fail because of the immutability of multiway delegates in C#. I cannot create a closure which contains for example CutRequest, because the subscription of new Handlers in CutRequest create a new Multiway delegate and replace the old value of CutRequest with this new delegate. If I need to elaborate more on this, let me know.

\$\endgroup\$
  • \$\begingroup\$ Make sure you have tests in place (unit, integration, functional, etc) to ensure you don't break existing functionality \$\endgroup\$ – dario_ramos Jan 30 '12 at 20:56
  • 1
    \$\begingroup\$ Can you break the problem down to some sample code? This is so hard to answer. \$\endgroup\$ – Gert Arnold Jan 30 '12 at 21:09
  • \$\begingroup\$ Anway, in order to not delay further progress on my part, I just implemented my own mutable multiway delegate. It's precisely the same as a normal delegate, just that it is mutable. That means, I can pass this delegate around into classes during setup time and add handlers later on and things still work. Furthermore, this allowed me to implement a functional map on events, so I have such a channel which maps all EventArgs e on the currently selected components and signals the new event. This reduces the duplication on about 6 lines in the constructor. Thank you all for participating. \$\endgroup\$ – Tetha Jan 31 '12 at 10:49
  • \$\begingroup\$ @Tetha You should post the above, preferably with some code samples, as the answer. \$\endgroup\$ – Joshua Drake Mar 30 '12 at 15:41
1
\$\begingroup\$

First, I would probably switch to EventHandler<ComponentSetArguments> and ditch ComponentSetHandler, since generics render it obsolete.

Next, it may be useful to have a helper extension method to raise events that do your null check (for several reasons).

public static void SafeInvoke<T> (this EventHandler<T> evt, object sender, T args)
{
   EventHandler<T> theEvent = evt;

   if (theEvent != null)
   {
      theEvent (sender, args);
   }
}

Finally, I notice your events in the example code use the same type of handler, so you could create a common method to raise your events that uses default arguments.

private void RaiseEvent (EventHandler<ComponentSetArguments> evt)
{
    ISet<IComponent> components;
    // if the user selected a single component, cut that component.
    // if the user selected no components, cut the component he pointed at.
    if (selectedComponents.Count == 0)
    {
        components = new HashSet<IComponent>();
        components.Add(lastHoveredComponent);
    } else
    {
        components = selectedComponents;
    }

   evt.SafeInvoke (this, new ComponentSetArguments (components));
}

Then, you just call it as follows:

RaiseEvent (CopyEvent);
RaiseEvent (CutEvent);

For those that are just using selectedComponents, it's just a single line as well:

CopyEvent.SafeInvoke (this, new ComponentSetArguments (selectedComponents));
\$\endgroup\$
0
\$\begingroup\$

I implemented my own MVC (Model-View-Controller) framework. In order to reduce the number of view events the controller had to subscribe to, I made my views send messages. I.e. I created a Message class containing different useful pieces of information. One of them is a message type that I realized as an enum. Another vital piece of information is the type of the entity which is concerned (like customer, order, invoice). The view exposes only one event to the controller, a ViewEvent event which sends a Message.

public delegate void ViewEventHandler(MvcMessage message);

public interface IView // Simplified
{
    event ViewEventHandler ViewEvent;
}

public class MvcMessage // Simplified
{
    public object Source { get; private set; }
    public MessageType MessageType { get; private set; }
    public Type EntityType { get; private set; }
    public EventArgs SourceEventArgs { get; internal set; }
}

public enum MessageType // Simplified
{
    None,

    // Requests
    RequestingNew,
    RequestingInsert,
    RequestingOpen,
    RequestingClose,
    RequestingDelete,
    RequestingDraw,

    // View Events
    ViewFilterChanged,
    ViewSelectionChanged,
    ViewClosing
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy