Calling functions from Events
-
I had an old school teacher that that concept was one of the first things he taught us. 1. put it in a method that can be called as a general rule 2. name the method something useful and meaningful. 3. add comments where necessary to clear things up further. The inherited code I'm looking at right now does pretty much none of the above. well over 100,000 lines of code and I have found 5 comments, one of which i paraphrase a tad bit "the order of these items must match the order of the input or bad things will happen" That is the most informative comment in the entire dang thing. and he liked names such as
public static string Code()Programming is a race between programmers trying to build bigger and better idiot proof programs, and the universe trying to build bigger and better idiots, so far... the universe is winning. A graduation ceremony is an event where the commencement speaker tells thousands of students dressed in identical caps and gowns that 'individuality' is the key to success
gavindon wrote:
he liked names such as
public static string Code()I'm not surprised, I find I use the first three a lot too. The last one I don't use myself, but that's just one of my foibles
Graham Librarians rule, Ook!
-
I doubt you can. You can pass null for the sender, but not the eventargs, I don't think.
Christian Graus Driven to the arms of OSX by Vista. Read my blog to find out how I've worked around bugs in Microsoft tools and frameworks.
I pass
nullfor both; have for years. -
I can understand one or two lines, easily. I can't handle the mammoth 20-100 lines in an event handler. I don't know if I'm being idiosyncratic, but it drives me mad!
Craigslist Troll: litaly@comcast.net "I have a theory that the truth is never told during the nine-to-five hours. " — Hunter S. Thompson
wizardzz wrote:
I can understand one or two lines, easily. I can't handle the mammoth 20-100 lines in an event handler. I don't know if I'm being idiosyncratic, but it drives me mad!
I love these kind of questions. I suspect the kernel of this issue is something to do with the notion of separating UI from logic. It's sound thinking. There are few things worse than a mess of code implemented directly behind the controls on a form. So...Is it "good practice" to move code out of event handlers and provide simple calls from the handler to the extracted method? I say no. But stay with me, and don't throw that tomato yet... An event handler is just a method. The fact that it's not "called" by your code in the traditional sense doesn't make it any less of a method. Same rules for determining the quality of a method should apply. If the quality of the code in the event handler is good, then there's generally little to be gained by moving it, except another layer of indirection. If the quality of the code is bad then moving it out without refactoring it won't make it any better. So, to answer your first question, NO, I wouldn't simply call out from an event handler to another method. An event handler should do it's job...handle an event. Which ties in with your idea that you are ok with a few lines of code, but big mammoth event handlers are the real problem. Absolutely, but it's the bigness and the mammothness of them that's the problem. Not the Eventness of them. The key as with any problem method is to clean up the code. Decompose those 100 lines into perhaps 4 or 5 methods, or more. Whether you then orchestrate calls to those methods directly from the event handler, or from some other method that the handler calls is up to you. Here's a question. When you extract the code from an event handler in another method, where does that method go? If it's just another private method in the Form or WebForm then you haven't really achieved much. If on the other hand you devise a sort of service method separate from the UI, then extracting the code from the event handler works well. The key as I've said is that you have to tidy up the code, not simply move it out of the event handler for the sake of moving it. Incidently moving code out of the UI into a service layer also makes it easier to Test. Another good reason to do it. It's not the almost empty event handlers that we're aiming for, it's the set of well defined
-
I doubt you can. You can pass null for the sender, but not the eventargs, I don't think.
Christian Graus Driven to the arms of OSX by Vista. Read my blog to find out how I've worked around bugs in Microsoft tools and frameworks.
It's only a problem if the recipient is expecting a value and doesn't do a null check. ~99% of events that take the base EventArgs don't look at it, so it's generally safe. IIRC my code is 100% for not looking at the useless thing.
3x12=36 2x12=24 1x12=12 0x12=18
-
wizardzz wrote:
I can understand one or two lines, easily. I can't handle the mammoth 20-100 lines in an event handler. I don't know if I'm being idiosyncratic, but it drives me mad!
I love these kind of questions. I suspect the kernel of this issue is something to do with the notion of separating UI from logic. It's sound thinking. There are few things worse than a mess of code implemented directly behind the controls on a form. So...Is it "good practice" to move code out of event handlers and provide simple calls from the handler to the extracted method? I say no. But stay with me, and don't throw that tomato yet... An event handler is just a method. The fact that it's not "called" by your code in the traditional sense doesn't make it any less of a method. Same rules for determining the quality of a method should apply. If the quality of the code in the event handler is good, then there's generally little to be gained by moving it, except another layer of indirection. If the quality of the code is bad then moving it out without refactoring it won't make it any better. So, to answer your first question, NO, I wouldn't simply call out from an event handler to another method. An event handler should do it's job...handle an event. Which ties in with your idea that you are ok with a few lines of code, but big mammoth event handlers are the real problem. Absolutely, but it's the bigness and the mammothness of them that's the problem. Not the Eventness of them. The key as with any problem method is to clean up the code. Decompose those 100 lines into perhaps 4 or 5 methods, or more. Whether you then orchestrate calls to those methods directly from the event handler, or from some other method that the handler calls is up to you. Here's a question. When you extract the code from an event handler in another method, where does that method go? If it's just another private method in the Form or WebForm then you haven't really achieved much. If on the other hand you devise a sort of service method separate from the UI, then extracting the code from the event handler works well. The key as I've said is that you have to tidy up the code, not simply move it out of the event handler for the sake of moving it. Incidently moving code out of the UI into a service layer also makes it easier to Test. Another good reason to do it. It's not the almost empty event handlers that we're aiming for, it's the set of well defined
After reading through many replies to my original post, I have determined that a lot of my frustration stems from the lack of any documentation in any code, along with poorly and inconsistently named variables. This combination with event handlers containing what should be thrown into a nicely callable method, has made deciphering what any control actually does nearly impossible.
Craigslist Troll: litaly@comcast.net "I have a theory that the truth is never told during the nine-to-five hours. " — Hunter S. Thompson
-
Because you may need to trigger the action differently - e.g. from different events, or programmatically. Now, you might say "I will isolate it when I need it" - but the issue goes deeper in my experience. When you put the code immediately in the handler, you can make all kinds of assumptions: The user just clicked that button, so the form is probably on screen and active and the user didn't do anything else in the last 10 ms. These implicit assumptions pile up over time, and make maintenance hard. If you put the action into a public method right away you have to make sure the method can be called anytime by anyone (or understand and document trhe cases where this isn't). Separate the what from the when - this does more for UI/functionality independence than putting it into separate classes. Now yes, that's overhead. However, in my experience the "quick and dirty" method leads often leads to repeated operations in the long run because it's impossible to detangle the call paths of the helepr method. My personal warning sign is methods getting parameters like "doUpdate", "refresh", "updateImmediately" etc. are a warning sign for that.
FILETIME to time_t
| FoldWithUs! | sighist | WhoIncludes - Analyzing C++ include file hierarchyCompletely agree with you. I actually tend to break things up into separate into functions, except for ultra simple ui stuff. This just comes with experience and recognizing/anticipating patterns. I was just pointing out a reason (performance) not to do this, which with today's super computers on our desks, it's not really an issue anymore.
- S 50 cups of coffee and you know it's on! Code, follow, or get out of the way.
-
Because you may need to trigger the action differently - e.g. from different events, or programmatically. Now, you might say "I will isolate it when I need it" - but the issue goes deeper in my experience. When you put the code immediately in the handler, you can make all kinds of assumptions: The user just clicked that button, so the form is probably on screen and active and the user didn't do anything else in the last 10 ms. These implicit assumptions pile up over time, and make maintenance hard. If you put the action into a public method right away you have to make sure the method can be called anytime by anyone (or understand and document trhe cases where this isn't). Separate the what from the when - this does more for UI/functionality independence than putting it into separate classes. Now yes, that's overhead. However, in my experience the "quick and dirty" method leads often leads to repeated operations in the long run because it's impossible to detangle the call paths of the helepr method. My personal warning sign is methods getting parameters like "doUpdate", "refresh", "updateImmediately" etc. are a warning sign for that.
FILETIME to time_t
| FoldWithUs! | sighist | WhoIncludes - Analyzing C++ include file hierarchypeterchen wrote:
Now, you might say "I will isolate it when I need it" - but the issue goes deeper in my experience. When you put the code immediately in the handler, you can make all kinds of assumptions: The user just clicked that button, so the form is probably on screen and active and the user didn't do anything else in the last 10 ms. These implicit assumptions pile up over time, and make maintenance hard.
If you put the action into a public method right away you have to make sure the method can be called anytime by anyone (or understand and document trhe cases where this isn't). Separate the what from the when - this does more for UI/functionality independence than putting it into separate classes.I'm skeptical about this. Knowing that
DooFoo()is only called from theFooButtonClicked()event handler, what makes you think that a code monkey won't just xfer his full set of assumptions to the method that came from the event handler?3x12=36 2x12=24 1x12=12 0x12=18
-
peterchen wrote:
Now, you might say "I will isolate it when I need it" - but the issue goes deeper in my experience. When you put the code immediately in the handler, you can make all kinds of assumptions: The user just clicked that button, so the form is probably on screen and active and the user didn't do anything else in the last 10 ms. These implicit assumptions pile up over time, and make maintenance hard.
If you put the action into a public method right away you have to make sure the method can be called anytime by anyone (or understand and document trhe cases where this isn't). Separate the what from the when - this does more for UI/functionality independence than putting it into separate classes.I'm skeptical about this. Knowing that
DooFoo()is only called from theFooButtonClicked()event handler, what makes you think that a code monkey won't just xfer his full set of assumptions to the method that came from the event handler?3x12=36 2x12=24 1x12=12 0x12=18
Thanks for your feedback.
Dan Neely wrote:
what makes you think that a code monkey won't just xfer his full set of assumptions to the method that came from the event handler
Nothing. For me it's just a practice that makes me think about a clean interface, separating functionality from the event, and it documents that separation in the code. That last point makes it "not just personal" for me. Of course you can have an
EnableEditMode()method that fails spectacularly whenbutton2is disabled. And I can have anEnableEditModeButOnlyWhenButton2IsEnabledmethod. Both sucks - but in both cases, I would argue the code is better even without any comments: In the first case, the code shows clearly that when Event X fires, EditMode should be enabled. The function is well named, but has a bug. In the second case, the interface already "smells" wrong.
Now, this may be an idealized example, and I don't think it's necessary to do this separation, just good practice. Code is never perfect, we usually can't even tell if it's good enough, but we can tell better from worse.
FILETIME to time_t
| FoldWithUs! | sighist | WhoIncludes - Analyzing C++ include file hierarchy -
After reading through many replies to my original post, I have determined that a lot of my frustration stems from the lack of any documentation in any code, along with poorly and inconsistently named variables. This combination with event handlers containing what should be thrown into a nicely callable method, has made deciphering what any control actually does nearly impossible.
Craigslist Troll: litaly@comcast.net "I have a theory that the truth is never told during the nine-to-five hours. " — Hunter S. Thompson
That's what I say all the time. Take some time and document the damn method. Let me in your "beautiful" mind and enlighten me with this method's purpose so i don't have to spend hours extracting it from the bare code. I have seen this project exquisitely architected, exuding of design patterns and decoupling all over the place. Very manageable they say. Still, lacking documentation in 90% of the cases, made it so hard to understand for new people that soon on top of that beautifully crafted architecture people started to throw patches and "Band-Aid" code. So, what good is it that you decoupled stuff, when it made it so abstract, that new people have a hard time understanding it, ‘cause you did not document. Doesn't that bring us back to code management trouble? I don't know man, maybe it's just me. Cheers.
giuchici
-
Same here. If it's a couple of lines, it stays in the event handler. Otherwise, it's going into it's own method, even if, right now, it's only called by the event handler. You never know. Tomorrow, you'll find out that you now need to call it from multiple places. But, the biggest reason I break it out into its own method because of "self documenting code". Why this is such a weird concept no-a-days is beyond me. Put the code into a method and name the method something that describes what the code does! OH MY GOD! WHAT A CONCEPT!! Imagine making debugging easier! ;)
A guide to posting questions on CodeProject[^]
Dave Kreskowiak -
I had an old school teacher that that concept was one of the first things he taught us. 1. put it in a method that can be called as a general rule 2. name the method something useful and meaningful. 3. add comments where necessary to clear things up further. The inherited code I'm looking at right now does pretty much none of the above. well over 100,000 lines of code and I have found 5 comments, one of which i paraphrase a tad bit "the order of these items must match the order of the input or bad things will happen" That is the most informative comment in the entire dang thing. and he liked names such as
public static string Code()Programming is a race between programmers trying to build bigger and better idiot proof programs, and the universe trying to build bigger and better idiots, so far... the universe is winning. A graduation ceremony is an event where the commencement speaker tells thousands of students dressed in identical caps and gowns that 'individuality' is the key to success
Regarding comments: It is best to summarize right at the beginning of a method what it is supposed to do. You then write the code in a clear and self-commenting manner that makes it easy for you and other programmers to see how it is being done. So if it turns out that there are times when the result of the method is inconsistent with the summary, it is easier to debug. The other benefit of doing it this way is that the developer has given thought to the method's objective before actually writing the code that does it.
-
Same here. If it's a couple of lines, it stays in the event handler. Otherwise, it's going into it's own method, even if, right now, it's only called by the event handler. You never know. Tomorrow, you'll find out that you now need to call it from multiple places. But, the biggest reason I break it out into its own method because of "self documenting code". Why this is such a weird concept no-a-days is beyond me. Put the code into a method and name the method something that describes what the code does! OH MY GOD! WHAT A CONCEPT!! Imagine making debugging easier! ;)
A guide to posting questions on CodeProject[^]
Dave KreskowiakWell, since Event Handlers can be named as well, you could create self-documenting code without necessarily having to create a separate method. Of course, if your event handler is button18_Click that isn't going to work.
-
Not a question, more of a survey. How many people here always separate code from event handlers, especially Form events. I always do it, but it is driving me mad looking at and updating code that doesn't do it, written by a senior engineer. All my event handlers call functions, nothing more.
Craigslist Troll: litaly@comcast.net "I have a theory that the truth is never told during the nine-to-five hours. " — Hunter S. Thompson
There is an anti-pattern defined for what you described: Magic Pushbutton[^]. Another consideration is whether to handle the event asynchronously or not. In most cases I handle the event in a different thread and disable the button (or other control) until the event handling code finishes. That way the UI doesn't become unresponsive while waiting. Of course this is uneccessary for very short operations.
-
Well, since Event Handlers can be named as well, you could create self-documenting code without necessarily having to create a separate method. Of course, if your event handler is button18_Click that isn't going to work.
Part of self-documenting code is the parameters that are passed into the method. Sure, you could name a Button.Click event handler something more useful, like GetSccmFolders, but what does the
sender (Object)andSystemEventArgsor even theHandlesclause have to do with getting Sccm Folders? Nothing! So why is it there? This just leads to misleading documentation and code that has to be called in very specific ways that have nothing to do with the operation the method is really performing.A guide to posting questions on CodeProject[^]
Dave Kreskowiak -
Part of self-documenting code is the parameters that are passed into the method. Sure, you could name a Button.Click event handler something more useful, like GetSccmFolders, but what does the
sender (Object)andSystemEventArgsor even theHandlesclause have to do with getting Sccm Folders? Nothing! So why is it there? This just leads to misleading documentation and code that has to be called in very specific ways that have nothing to do with the operation the method is really performing.A guide to posting questions on CodeProject[^]
Dave Kreskowiak -
Same here. If it's a couple of lines, it stays in the event handler. Otherwise, it's going into it's own method, even if, right now, it's only called by the event handler. You never know. Tomorrow, you'll find out that you now need to call it from multiple places. But, the biggest reason I break it out into its own method because of "self documenting code". Why this is such a weird concept no-a-days is beyond me. Put the code into a method and name the method something that describes what the code does! OH MY GOD! WHAT A CONCEPT!! Imagine making debugging easier! ;)
A guide to posting questions on CodeProject[^]
Dave KreskowiakDave Kreskowiak wrote:
You never know. Tomorrow, you'll find out that you now need to call it from multiple places.
The biggest reason for separate methods, in my experience. I don't work for MS, but one thing they get right (imo) is to have multiple ways of doing almost anything. That is easiest to handle with a single well-named and well-designed method (or "umbrella" method) called by multiple event handlers. This way, I can be reasonably sure that every UI action that enters that code will behave the same way.
-
I agree with you. But what does GetSccmFolders() mean? In other words, is it a good example of self-documenting code?
No, it's not a good example of self-documenting code, but it's also not what you would call an event handler either. The point of the post wasn't so much what you called the method, but that the arguments don't match what the method does.
A guide to posting questions on CodeProject[^]
Dave Kreskowiak -
Overall it is a good idea to call functions, that way you can change the logic pretty easily and also make the code readable (assuming functions have good names). It is pretty easy these days with refactoring support to extract the code into separate methods. For instance,
void serverName_changed(object sender, EventArgs e)
{
ResetUserNameAndPassword();
}is more readable then
void serverName_changed(object sender, EventArgs e)
{
Username.Text = "";
Password.Text = ""
// Some other code usually gets very long
//}
you still end up with:
void serverName_changed(object sender, EventArgs e)
{
ResetUserNameAndPassword();
}
...
void ResetUserNameAndPassword(void)
{
Username.Text = "";
Password.Text = ""
// Some other code usually gets very long
//
}The only thing you have really done is add an additional call to the execution stream. Granted, if the event is not a critical timed event handler, it probably won't matter. But if the event is generated 100s or 1000s of times a second, the extra code could have an impact on performance.
Steve _________________ I C(++) therefore I am
-
Not a question, more of a survey. How many people here always separate code from event handlers, especially Form events. I always do it, but it is driving me mad looking at and updating code that doesn't do it, written by a senior engineer. All my event handlers call functions, nothing more.
Craigslist Troll: litaly@comcast.net "I have a theory that the truth is never told during the nine-to-five hours. " — Hunter S. Thompson
I almost always call functions from events, but if I need to act on the control I put all that logic in the event (e.g. RunButton_Click() will change the text to "Stop") Ionce had an engineer working for me who liked to raise control events to use the functionality he put in the event handlers. Lots of very interesting bugs crawled out of that practice. He never quite caught on to the problem.
-
Not a question, more of a survey. How many people here always separate code from event handlers, especially Form events. I always do it, but it is driving me mad looking at and updating code that doesn't do it, written by a senior engineer. All my event handlers call functions, nothing more.
Craigslist Troll: litaly@comcast.net "I have a theory that the truth is never told during the nine-to-five hours. " — Hunter S. Thompson
I typically guard condition, delegate to some other logic outside of an event model, and then bubble if applicable. I also try to reuse the same abstracted event as much as possible and deal w/ variability via arguments vs having an event per usage. I feel that event handlers are just listeners / hooks and not workers. Just middle men basically, that facilitate decoupled collaboration. I do make some exceptions where common usage dictates...like a repeater item data bound event handler; it doesn't always make sense to punt the binding if it's tiny. But for non-trivial scenarios I'll push the repeater's body template into a separate component with a "Bind(someModel)" method, and forward to it from the event handler on the containing page or component. As it happens, I'm currently refactoring a ASP.NET client server app towards patterns, paying back a big design debt incurred by the original developers. Refactoring bloated event handlers in this app is one of the biggest sources of code improvement. In particular I'm frequently finding baskin robbins 31 flavor events that all basically do the same thing with only slight variations. This one is chocolate, that one is double fudge, that one is choc-vanilla swirl, etc. Extracting the commonality and variabilizing the differences has significant impact on thinning down code behinds and standardizing the logic; it also has lead to the detection of latent or inconsistent bugs, and generally been a major enhancement to the code quality. It's also a lot easier to read / maintain the end product.
