tags:

views:

188

answers:

5

I'm throwing this out there just as a question of curiosity...

Assuming you're only expecting/wanting one method to be provided, would this be frowned upon or bad practice?

public class Something  {
    public Action OnRemove = () => { };
    public Action<object, EventArgs> OnFinishedLoading = (sender, e) => { };
}

// then used like...
something.OnRemove = () => { /*do something...*/ };
something.OnFinishedLoading = (sender, e) => { /*do something else...*/ };

I realize this is like cheating at events, but I is there anything that is bad about this approach? Would this cause any potential issues with your application in the long run?

I realize if you're wanting more than one method to run, then an event would be better, this is mainly a question of if you're only wanting/expecting one method.

A: 

With the event, you can't do this:

something.OnRemove = null;

The events gives you a wrapper, kind of the wrapper with properties and attributes, so you can ensure encapsulation over the Delegate Method.

Jhonny D. Cano -Leftware-
Except if I remember correctly an event is null until a handler is assigned, which could result in the same situation.
Hugoware
+6  A: 

Well, the most obvious thing "wrong" with it is that it's exposing public fields. I'd at least use a property - at which point an event would probably be simpler anyway. It gives more flexibility with less code than a property (because you couldn't use an auto-implemented property with a specific default value).

The other alternative would be to take it in the constructor and then keep it as a private read-only field. That's probably what I'd really be inclined to do... is there really any need to expose the action as a property/field at all?

Jon Skeet
I used fields to simply shorten the length of the code in the sample above.
Hugoware
When you're asking a question about good/bad practice, it's probably best not to knowingly inject a bad practice for the sake of brevity ;)
Jon Skeet
+1  A: 

There is nothing wrong with this approach and I use it very often. It makes simple handlers have simple code (which is a good thing).

The only downside to this approach is that a handler created in this way cannot be easily removed. Normally you cane say

something.OnRemove -= SomeEvent;

But this is not the case with all lambda expressions. Textually equal lambda expressions will almost certainly have separate implementations and hence will not match up for the purpose of adding and removing event handlers.

For instance, the following code will fail to remove the handler.

something.OnRemove += () { MessageBox.Show("Removed!"); }
something.OnRemove -= () { MessageBox.Show("Removed!"); }

But as long as you only want to add, this is not a problem.

JaredPar
+1  A: 

It potentially exposes more than you want to. Normally, an event does not permit a client to examine the contents of the MulticastDelegate that backs the event.

Jeffrey Hantin
+1  A: 

I think this approach is fine, it's just a shorthand way of declaring a delegate (look at the definition of the Action family).

Richard