Is it bad practice to write inline event handlers

Question

Is it bad practice to write inline event handlers ?

For me, I prefer use it when I want to use a local variable in the event handler like the following:

I prefer this:

// This is just a sample
private void Foo()
{
    Timer timer = new Timer() { Interval = 1000 };
    int counter = 0; // counter has just this mission
    timer.Tick += (s, e) => myTextBox.Text = (counter++).ToString();
    timer.Start();
}

Instead of this:

int counter = 0; // No need for this out of Boo & the event handler

private void Boo()
{
    Timer timer = new Timer() { Interval = 1000 };

    timer.Tick += timer_Tick;
    timer.Start();
}

void timer_Tick(object sender, EventArgs e)
{
    myTextBox.Text = (counter++).ToString();
}

I think it depends on your team. If everybody is up on those features, then it's fine. I personally like to make the lambdas more obvious with on a separate line, etc.., I like to code so that 1 line of code does 1 thing. — kenny, Oct 31 '10 at 15:51
+1 @kenny I agree with you, this make the code more legible. — Omar, Mar 05 '12 at 17:53
Agree with kenny as well. Once you've accepted Jon's answer and determined it's fine from a technical standpoint (performant, scalable, etc.), good/bad practice is all about predicting who will be reading or modifying this code later and how they will perceive it, including yourself after you've forgotten what it does. — xr280xr, Aug 22 '19 at 18:09

score 86 · Accepted Answer · edited Jun 01 '23 at 13:38

86

It's absolutely fine - although there are two caveats:

If you're modifying a local variable from within a closure, you should make sure you understand what you're doing.
You won't be able to unsubscribe from the event

Typically I only inline really simple event handlers - for anything more involved, I use lambda expressions (or anonymous methods) to subscribe with a call to a method with a more appropriate method:

// We don't care about the arguments here; SaveDocument shouldn't need parameters
saveButton.Click += delegate { SaveDocument(); };

edited Jun 01 '23 at 13:38

karlingen

13,800
5
43
74

answered Oct 31 '10 at 15:51

Jon Skeet

1,421,763
867
9,128
9,194

3

This answer can send people down a bad path. It depends on the application as to how important it is that the event handler can't be unsubscribed. In memory constrained situations (ex: on mobile), it is important to clean up event handlers to ensure that ViewControllers and Activities can be properly garbage collected. Failing to do so can cause notable memory allocation increases, or unintended behaviors when an event fires for a screen that is in the navigation back-stack and not currently shown. It is important to know based on your use case whether it's ok to not unwire events. – SmartyP Mar 05 '18 at 16:05
4

@SmartyP: Users should know whether or not they need to unsubscribe events - and that's not really in the scope of this question, IMO. I've made it clear that they can't do so, which means if they *need* to, they'll know not to use anonymous functions. – Jon Skeet Mar 05 '18 at 18:02
Keep a reference to the delegate to be able to remove it later. https://stackoverflow.com/a/2915741/351245 – Ian Oct 27 '19 at 03:28

score 3 · Answer 2 · answered Nov 01 '10 at 10:59

In most cases I would rather have the separate methods like “timer_Tick()”, however I should rather it be called OnTimerTick() as:

When I read the class, it is clearer wheat is going on. The “On” tells me its can event handler.
It is easier to set a break point in the method in the “inline” case.
The event is fired a long time after “Foo” contractor has returned, and I don’t think it as running in t the scope of the contractor.

However if the event will only be fired before the method it is declared in-line returns and the object the event is set on has a scope what that is limited to the declaring method, then I think the “in line” version is better. Hence I like using “in line” for the compare delegate being passed to a “sort” method.

score -1 · Answer 3 · answered Oct 31 '10 at 21:12

-1

You put the two samples together. It is clear that the second option (which you don't prefer) is the most readable.

Code readability and maintainability are very important. Keep things simple, as easy as possible to understand. Lambda expressions are generally considered harder to understand by majority of people. Even if they are a second nature for you for others might not.

answered Oct 31 '10 at 21:12

Liviu Mandras

6,540
2
41
65

3

Personally I find the Lambda Syntax quicker to mentally parse and prefer the first sample. But then this is 4 years later. – Holf Jan 13 '15 at 17:53

Is it bad practice to write inline event handlers

3 Answers3

Linked