|
The idea of the extension methods is fine, but they're implemented with several mistakes and are too simple. Also, the sample uses an outdated convention which is rarely used.
|
|
|
|
|
have to downvote - see my other comments...
|
|
|
|
|
There is also a possibility for null exception there due to a well known race condition when dealing with events where a context switch could occur (threading is hard)
So you should probably rewrite this:
public static void Raise(this EventHandler eventHandler, object sender, EventArgs e)
{
if (eventHandler != null)
{
eventHandler(sender, e);
}
}
To this
public static void Raise(this EventHandler eventHandler, object sender, EventArgs e)
{
var handler = eventHandler;
if (handler != null)
{
handler(sender, e);
}
}
Sacha Barber
- Microsoft Visual C# MVP 2008-2012
- Codeproject MVP 2008-2012
Open Source ProjectsCinch SL/WPF MVVM
Your best friend is you.
I'm my best friend too. We share the same views, and hardly ever argue
My Blog : sachabarber.net
|
|
|
|
|
It's an argument, it can't change randomly during the method call. Inlining should be disabled though.
“Today is the first day of the rest of your life.”
|
|
|
|
|
|
I just love detailed exhaustive answers which completely prove the point.
“Today is the first day of the rest of your life.”
|
|
|
|
|
lol - you don't know about the handler pinning problem? you know the argument is a reference anyway, so even your "argument" argument is wrong. I think Sasha showed the point - this is a very well known issue in .NET... so sorry for not writing to much to answer your comment - but see my other comments here...
|
|
|
|
|
When you pass a delegate as an argument, it's already a copy of a reference to an immutable object (a "pinned handler" if you like), so how can it become null after the null check? Copying the reference to the delegate into a separate local variable would achieve nothing. If you think you're smarter than Jon Skeet who approves this[^], provide the code.
“Today is the first day of the rest of your life.”
|
|
|
|
|
Ah now I see what you missed: We are talking about multithreaded scenario! And btw. what Jon Skeet is writing there (don't know him, just saw his posts on the linked site, add extra handler to avoid null check, classes should be sealed etc.) I think I can honestly say I'm a smarter programmer (but thats not my point, just giving you an answer).
Provide code for what? Sasha showed the point, you commented he is wrong (you know that Sasha is one of the heroes here?), I told you you are wrong, you linked to "not so good" postings about the topic...
Btw as I mentioned this is a VERY WELL known topic in .NET and therefore this pattern (to create a local copy) emerged. You didn't know it, that's ok, and if you still write your code like Jon Skeet, I have no problem with that too (And if you don't do any multithreading and only simple apps it's ok too).
But you are right it shouldn't need to be done like this, I'd call it a "design flaw" in .NET.
|
|
|
|
|
Jon Skeet is StackOverflow's local god (he also wrote C# in Depth, is C# MVP etc.). He may have questionable preferences, but it doesn't make him wrong. It's not like I agree with everything even Eric Lippert says either, but I'm not pretending I'm smarter.
My points:
1. Delegates are immutable.
2. An argument passed by value is a local variable.
3. When you write changed += handler , you get a new delegate assigned to changed .
4. When you write if (changed != null) changed(...) and changed is an event field, it can become null between accesses, because the value of the field can be changed by another thread to another delegate or null .
5. When you write if (changed != null) changed(...) and changed is a local variable (or an argument passed by value), it cannot be changed from the outside between the accesses, because only one method has access to it and delegates are immutable.
The "VERY WELL KNOWN TOPIC" you refer to is #4. We're dealing with #5. Look at the code, it's not that hard.
“Today is the first day of the rest of your life.”
|
|
|
|
|
Hi Athari,
Now I'm confused... We are talking about events here, don't we? So yes, I was refering to topic 4, and never argued about topic 5 (if passed by value). This is the problem Sacha was refering to (his code is clear?)
Sorry about my (maybe to fast) opinion on Jon (as I mentioned, didn't know him and just read what he wrote on the linked site, sounded quite strange...)
You seem to be a good .NET programmer too, but I'm wondering why you insist that capturing an Event variable in a multithreaded scenario isn't needed. Still don't get it. Maybe I'm not that smart as I thought?
Btw. I'm programming for many years too, so value type and reference type diff is clear to me (or did you mean something else?
If I read my posts again, they sound quite rude, must say sorry for that (English is not my native language, as you may already know )
Kind Regards
Johannes
|
|
|
|
|
Yes , I agree to your Point, It should be coded as you did , to Avoid Race Condition. Thank you
Bheeeshm
|
|
|
|
|
I don't like your solution because there is a convention in .NET to use a SINGLE method to raise an event (On<eventname>), it should be declared virtual so derived classes can completly disable the event. Don't raise the same event from variouse places.
Your code is not wrong and workes for shure - but if you use it in your projects you break the "convention" everywhere. I use a snippet for this case - creates the event, a strongly typed EventArgs class an the On...Method to raise the event. (thread safe) -> convention fullfilled and even faster than your approach (in terms of typing work).
|
|
|
|
|
Agreed.
The only instant messaging I do involves my middle finger.
English doesn't borrow from other languages.
English follows other languages down dark alleys, knocks them over and goes through their pockets for loose grammar.
|
|
|
|
|
Hi johannesnestler.
I agree with your point at some extent, e.g. Declaring it Virtual. Rest I do not see any problem?
I have also accepted the comments from 'Sacha' , and Update the Code to avoid Race Condition.
Here what i like is , many time we need to raise events , we check the handlers for null and then Invoke the event... It can be done now with the 'Raise' extension method.
Please Comment to help me improve further! Thanks
Bheeeshm
|
|
|
|
|
Hi Bheeshm,
So I try to elaborate what I mean.
* If you raise an Event from variouse places this can quickly lead to a maintanance nighmare - therefore we have a "convention" not to raise the same event from variouse places, but from a single "Point" - this is the On[EventName] method you can see everywhere in the framework. The other "twist" with this pattern is that you can completly disable the event in derived classes.
So the pattern is:
EventHandler<SomeEventArgs> SomeEvent;
protected virtual void OnSomeEvent(SomeEventArgs sea)
{
}
Have also a look at the naming of the 3 parts: Event, EventArgs, name of the method raising the event. This is another convention.
So in the variouse parts of your codes in your class which needs to raise the Event you call the On method to raise the Event for you (giving it the needed (different) EventArgs as argument.
Another important thing:
* If this is a "Model" class (MVVM) you should implement INotifyPropertyChanged interface http://msdn.microsoft.com/en-us/library/system.componentmodel.inotifypropertychanged(v=vs.110).aspx[^]
This is another "convention" you are breaking (but this time your code won't work in some circumstances, cause some framework features are depending on it)
So your solution:
* Raises the same event (through your generic "raiser" from variouse places, no chance to disable it for derived types)
* Breaks the convention and therefore may be harder for others to understand
* Doesn't consider multithreaded scenarios (Sashas comment) and can lead to VERY nasty bugs.
* Your sample doesn't give the changed values to the events (shouldn't always use the generic variant with some specific EventArgs types?)
* Your method needs the parameter for the object raising the event (On... method pattern doesn't)
* Your Model classes should really implement INotifyPropertyChanged/-Changing pattern!
The only good thing I can see about your solution is: It saves some typing, and as I mentioned I can be much faster creating the whole pattern with a code snippet. (I can give it to you if you want - just have to change the german commenting... (Even the comments for this pattern follow always the same convention in the framework!)
But don't be sad about this topic, you tried to find a way for yourself, put the efford in to write this tip, and now you may learn that there is a convention and may decide to follow it.
Kind Regards Johannes
|
|
|
|
|
Hi Johannes,
Thanks for Comments. Now I agree with the points you mentioned, and I have also gone through INotifyPropertyChanged . and I liked it very much.
And Now i have mixed everything and wants you to comment again
... I will Implement INotifyPropertyChanged into Model
1. in this Model ... Should I Mark NotifyPropertyChanged method as Virtual ? I think it will be Good ? So i can Disable the Event Invocation in Child for some Property if i need to.
2. TO Avoid Race condition/Multithreading issue, should i still pin handler variable locally? I think yes!
3. I am still digesting all your comments and will be back if needed Thanks Again !!!
Bheeeshm
|
|
|
|
|
Hi Bheeshm,
Nice to hear you like NotifyPropertyChanged interface/pattern. maybe you want to google for some implementation patterns and read further about MVVM http://msdn.microsoft.com/en-us/library/gg405484(v=pandp.40).aspx[^]
But to quickly answer your questions.
1. yes
2. YES always, for all event raising helper mehthods. And don't do anything else there than raise the event
3. You are welcome. I know it's hard to hear, if you come up with a solution for your own, that there is already an accepted pattern. But this way, you will get the best insight to problems in the long run. When I was a young programmer, I found most of the common solution-patterns for myself (messing arround, and loosing a lot of time, just to find out others had a better/ready solution), but I think this leaded to much more insight into some topics, than just take the "recipe" and don't think about it.
So good luck with your projects in between, and
Kind Regards Johannes
|
|
|
|
|
> So the pattern is
You forgot the event keyword, like in the article. Without this keyword, the encapsulation is broken.
> This is another "convention" you are breaking (but this time your code won't work in some circumstances, cause some framework features are depending on it)
WPF supports both conventions (INotifyPropertyChanged and separate events) and several others (dependency properties, static events etc.), and in fact uses the older separate events convention too. There's nothing wrong with it, it's just not the most convenient. I guess WinForms supports both approaches too.
> Your sample doesn't give the changed values to the events
It's optional. In case of the "property changed" event, it's redundant information, because you can just get the value from the property.
“Today is the first day of the rest of your life.”
|
|
|
|
|
@ event:
A, thank you (shouldn't write the code directly in the comments)
@WPF/Forms - you are right, both approaches are supported (but think about designer-aware code etc.)
@Right again, But keep an eye on multithreading if you don't use it with WPF (dispatched) and just let the handlers read the property value.
Thank you for the corrections.
|
|
|
|
|