CODEWITHSUNDEEP
c#
Sergio Tapia
Sergio Tapia

Reputation: 41158

Is it OK to program a method body inside of an event?

I recently saw some code from a college teacher where he had something like this:

public void Button1_Click(blabla)
{
   //His code was here.
}

Isn't it considered better programming practice to call a method to do the dirty work, that way if the method changes you only have to change the method itself and not the event? (Less chance of breaking something)

public void Button1_Click(blabla)
{
   DoSomething();
}

public void DoSomething()
{
   //The actual code here.
}

Upvotes: 3

Views: 175

Answers (9)

Dan Bryant
Dan Bryant

Reputation: 27505

What is the content of the method? If it's very clearly related to the UI (such as updating Visible/Enabled status) and is not called from anywhere else, then you might as well leave it in the handler. If, however, the method is doing things to your data/logic layer beyond delegating to a function call in those layers, then it's probably worth breaking out some of the logic and possibly moving it to a different layer. The only time I branch off strictly UI-related code to its own method is when I have to update the same or similar state from multiple event handlers.

Upvotes: 1

Stephen Cleary
Stephen Cleary

Reputation: 456657

Most modern design approaches (e.g., MVC, MVVM) only have minimal code in event handlers, passing off all program logic to a separate (unit testable) class.

However, for classroom instruction (or demo code), I see nothing wrong with just using the event handler directly.

Upvotes: 0

Jon Skeet
Jon Skeet

Reputation: 1500903

It makes a certain amount of sense to separate them like this, and it's really useful if you want to call DoSomething from elsewhere (where it wouldn't make sense to supply a sender and event args).

However, there's a nicer alternative here - at least if you're subscribing programmatically. Use an anonymous method or lambda expression instead:

button.Click += delegate { DoSomething(); }

Then it's blatantly obvious at the point of subscription that you're not using the other parameters, and you don't waste an extra method.

The disadvantage of this is that it doesn't work with Visual Studio's designer-generated event subscription. Personally I'd like Visual Studio to support subscribing to an event using a method which didn't have all the delegate's parameters - it could generate the anonymous method (or lambda expression) automatically. That would give the best of both worlds, IMO.

As other answers have said, I think it's reasonable to keep the method body in the event handler method itself if you don't need to call it from anywhere else.

Upvotes: 4

Kris van der Mast
Kris van der Mast

Reputation: 16613

If it's reusable code then place it outside. Also some coding guidelines, check your company's policy on that, force you to go for the second style.

Grz, Kris.

Upvotes: 0

Justin Niessner
Justin Niessner

Reputation: 245429

No, not necessarily.

Unless you're going to use DoSomething() elsewhere in the code as well...there's really no point in breaking it out to a seperate method.

Button1_Click is already a method in and of itself. It just happens to be a generated header that matches the click event delegate. You could make that name whatever you like and things would go on working just the way you want.

Upvotes: 1

ChrisF
ChrisF

Reputation: 137148

You only need to extract the DoSomething if

a) it's functionality is needed somewhere else, or possibly

b) it's quite a long piece of code.

If the event is the only place it's used then there's no real need to extract it.

Upvotes: 8

dave mankoff
dave mankoff

Reputation: 17779

If you don't intend on reusing the code, then it doesn't make a difference - its just overhead. If you intend on calling "DoSomething()" from other places in your code, then by all means, abstract it out.

Upvotes: 0

Joel Coehoorn
Joel Coehoorn

Reputation: 415881

Yes, the extra method is "better", but you have to weigh that against the YAGNI (You Ain't Gonna Need It) counter-principle.

There are two reaons the extra method is considered better:

  1. It makes it easier to refactor that behavior to be used in multiple places
  2. It makes it easier to separate out the behavior for use in a completely different application

Both of those come down to the idea of encapsulation and separating the behavior from the event. No one who comes to this code later is every likely to refactor to remove the extra method, but someone who comes to the code without the extra method just might refactor to add it. But again - YAGNI.

Upvotes: 0

anon
anon

Reputation:

I personally think it depends on the length of the code and if the code is logically located there.

So if it is only 1 or 2 lines then i do it inside, but if its more lines of code, i do it in a separate method.

Upvotes: 0

Related Questions