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

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?

Solution

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: