C#: is calling an event handler explicitly really "a good thing to do"?

Question

This question is related to C#, but may be applicable to other languages as well. I have a reservation against using code such as the following:

using System.Windows.Forms;

class MyForm : Form
{
    private Timer myTimer;
    private Button myButton;

    public MyForm()
    {
        // Initialize the components, etc.

        myTimer.Tick += new EventHandler( myTimer_Tick );
        myButton.Click += new EventHandler( myButton_Click );

        myTimer.Start();
    }

    private void myTimer_Tick( object sender, EventArgs eventArgs )
    {
        myTimer.Stop();
        // also, I see a lot of usage of 
        // Timer.Enabled = true/false instead of -^
        myButton_Click( this, ea /* or event EventArgs.Empty, or null */ );
        return;
    }

    private void myButton_Click( object sender, EventArgs eventArgs )
    {
        // do a lot of stuff, with lots of logic that doesn't even use the
        // state of the eventArgs
        return;
    }
}

Am I alone, in that the above style is a pet peeve of mine? Are there others who enjoy the clarity of separating event handling from the workload of functions, or even separating out complex routines into separate functions?

Is there even an accepted style? I feel like any expressiveness and flexibility that event handling in C# has can be lost with styles like this. I feel like if you have a method that means "a button has been clicked", then it should only be called when a button is clicked.

To those who write like this, I would say: if you insist on having an EventHandler method to handle your timer tick, and your button click, then call it something other than button_Click -- perhaps "handleUserEvent( object sender, EventArgs eventArgs )".

Really, though, the question is, are there any style guidelines that are widely used which either support or discourage usage such as the above?

Answer 1

The special things about events in C# (and the .Net framework in general is the delegate, which is the C/C++ equivalent of a function pointer. the method attached to the event itself is not special in any way and should be callable from anywhere.

Update: perhaps I should have been more verbose, but I thought my use of "should" instead of "can" or "may" would be enough. It is my assertion that event handlers should be called when the functionality they implement is needed, instead of having them become wrappers to methods that "do the work" the less method calls you have in the stack the better you will be, specially with the performance implications of .Net's exception handling.

Answer 2

This is definitely not a "personal preference". There is a clear, well-understood approach of how to write code that is well-structured, maintainable, reusable, and understandable. Each method in your code should encapsulate a single piece of reusable functionality. The structure of your code should be:

void ButtonClickEventHandler(...)
{
    UserData userData = //determine user data from event data
    DoUserThing(userData);
}

void DoUserThing(UserData userData)
{
    //do stuff
}

void SomeOtherMethod()
{
    UserData userData = //get userdata from some other source
    DoUserThing(userData);
}

(This is a very loose example. In a proper application everything should be separated into different classes by concern.)

Answer 3

myButton.PerformClick() is probably slightly nicer, if you don't need to pass eventargs. Sometimes you just want to simulate a click.

But yes, I would agree that it's nicer to move the real code into another function. I prefer my event handlers to be very simple - just connect the UI to the logic, which is elsewhere.

Then you can rearrange and redesign your UI without worrying so much about where the logic is.

Answer 4

This code increase the chance of problems if another coder works on the myButton_Click method.

What if I came in to adjust the implementation of the myButton.Click handler? I might assume that the sender object is a Button, and try to cast:

Button b = (Button)sender;

I have no knowledge without reading the rest of the class implementation that I'm not always receiving a Button as the sender.

So my point is: -1 for maintainability, because of breaking the assumptions of what objects will be passed as myButton_Click parameters.

Answer 5

I agree with Rex M's answer, but I'd take it one step further. If you are using the MVC pattern (or something similar), the view would delegate the button click to the controller. The controllers methods can of course be called from elsewhere in your class - say, from your timer callback.

So, back to your original code:

using System.Windows.Forms;

class MyForm : Form
{
    private Timer myTimer;
    private Button myButton;

    private MyController myController;

    public MyForm()
    {
        // ...
        // Initialize the components, etc.
        // ...

        myTimer.Tick += new EventHandler( myTimer_Tick );
        myButton.Click += new EventHandler( myButton_Click );

        myTimer.Start();
    }

    private void myTimer_Tick( object sender, EventArgs eventArgs )
    {
        myTimer.Stop();
        myController.SomeMethod()
    }

    private void myButton_Click( object sender, EventArgs eventArgs )
    {
        // All the stuff done here will likely be moved 
        // into MyController.SomeMethod()
        myController.SomeMethod();
    }
}

One advantage of using MVC is the decoupling of the controller from the view. The controller can now be used across multiple view types easily and exiting GUIs are easier to maintain as they contain very little application logic.

EDIT: Added in response to comments from the OP

The fundamental design principals of software engineering talk about coupling and cohesion. Importantly we strive to minimise coupling between components while maximising cohesion as this leads to a more modular and maintainable system. Patterns like MVC and principals like the Open/Closed Principal build on these fundamentals, providing more tangible patterns of implemenation for the developer to follow.

So, anyone who writes code as seen in the original post has not understood the fundamentals of software design and needs to develop their skills considerably. The OP should be commended for identifying this "code smell" and trying to understand why it's not quite right.

Some relevant references:

• http://en.wikipedia.org/wiki/Coupling%5F%28computer%5Fscience)
• http://en.wikipedia.org/wiki/Cohesion%5F%28computer%5Fscience)
• http://en.wikipedia.org/wiki/Loose%5Fcoupling
• http://en.wikipedia.org/wiki/Model–view–controller
• http://en.wikipedia.org/wiki/Design%5Fpatterns
• http://en.wikipedia.org/wiki/Open/closed%5Fprinciple
• http://en.wikipedia.org/wiki/Design%5FPatterns%5F%28book)

Answer 6

The short answer is that why would you simulate a button click by calling the handler directly? If you want to wire both methods up to the same event, you would just wire it up. Event handlers are multicast delegates, which means you can add more than one of them. Wiring up an event more than once is totally acceptable.

    myTimer.Tick += myTimer_Tick;
    myTimer.Tick += myButton_Click;

    myButton.Click += myButton_Click;

Whether or not this is a WTF is an engineering call that we can't make from a short code snippet. However, based on your comments, it smells like a WTF. Forms or any UI should never handle business logic. They need to be business-logic-aware to some degree (as in validation) but they don't encapsulate / enforce the logic themselves.

Going further, following some simple practices as basic refactorings and using a layered (n-tier) approach to software will take you a long way, and you will realise along the way that the code you presented smells bad.

Eventually you'll come across some high-level patterns like MVC (model-view-controller) and MVP (model-view-presenter) which go a step beyond the simple layering. If you follow them you get a good separation of concerns.

I agree with the accepted answer, but jumping right into 'Use MVC', here's some code that doesn't illustrate MVC, without explaining why is a little cargo-cult for me.