tags:

views:

86

answers:

4

I am using C# 3.0. Following the standard event pattern I have:

    public event EventHandler<EventArgs> SomeEventHappens;

    protected virtual void OnSomeEventHappens(EventArgs e)
    {
        if (SomeEventHappens != null)
        {
            SomeEventHappens(this, e);
        }
    }

    private object _someProperty;

    public object SomeProperty
    {
        get
        {
            return _someProperty;
        }
        private set
        {
            if (_someProperty == value)
            {
                return;
            }
            OnSomeEventHappens(EventArgs.Empty);
            _someProperty = value;
        }
    }

Within my same class I would like to take some actions when SomeProperty changes. The way I see it I have 3 alternatives:

1) Do stuff within my SomeProperty setter. Something rubs me the wrong way about doing this since I try to subscribe to the philosophy of everything should do one thing and do it well. Cramming stuff into a setter seems to go against that, or at least has the propensity to.

2) Do stuff in OnSomeEventHappens. Again, seems to go a little against keeping this in simple pieces. Also, if this method gets overridden, could potentially lose functionality if the implementer does not call the base method.

3) Have the class subscribe to SomeEventHappens. To me this seems to be the proper choice as far as encapsulation is concerned, and seems pretty clean. Again, possible repercussions if OnSomeEventHappens is overridden.

Maybe there is something more elegant? I cannot decide between option 2 and 3, and I am curious as to what the Best Practice is. Maybe the safest place is in the property setter after all.

Thoughts?

Update: Thanks for the great comments and answers below. I have learned that it is "OK" to have a class subscribe to its own events, although in my case I am leaning to not do due to overhead. I have put thought into the behavior of potential overriders of my virtual methods and what exactly I want to happen.

In my real-world case, I do not really want the events to be raised without the property being set. As the answers below have guided my thought process, I think I may go with option 1, due to the lower overhead, the reduced risk of improper behavior from inheritors, and it just generally makes better sense to me. Thanks again!

A: 

Will you always want to take this action, or might you want to subscribe and unsubscribe? In the latter case, option 3 is clearly a good idea.

Is the action you wish to take the kind of action that another class may wish to take? Again, that would lean towards option 3.

Is the action you wish to take inherently part of setting the property? If so, action 1 may be advisable.

Option 3 does sound like a nice "light touch" approach to me.

Jon Skeet
In my real-world situation (not the oversimplified version above) I would not want to subscribe and unsubscribe. The property is actually an enum and any one of a host of different events gets raised based on the enum value being set. Since I am raising the event in the setter, all of the event information is available to me there. Overhead-wise, it may be lighter to not subscribe to all those events and just deal with it in the setter.
jomtois
+2  A: 

If you call SomeEventHappens and OnSomeEventHappens from some common location (the property procedure or another function), then you don't have to worry about overriders neglecting to raise the event. I would prefer to override a function rather than handle events because there's less overhead.

BlueMonkMN
I chose this answer because it was the closest explanation to what I ended up doing. I learned a lot from the other answers thoug. Thanks!
jomtois
+1  A: 

In object frameworks outside of .NET, an object that subscribes to its own events is frowned upon primarily because such things lead to circular references that could keep the object alive indefinitely. This is not an issue in .NET, but it still seems "strange" to me for a object to grope itself this way.

If a class always needs to be aware of when changes happen to the property, your best bet IMO is to make the OnSomeEventHappens method virtual and override it in descendant classes that need the additional info. It's ok to put code in the event firing method. The event firing method is there precisely so that everyone who wants to fire that event has a uniform way to do it.

If you only occasionally need to be informed of when the property changes, then I suppose subscribing and unsubscribing to the event would be appropriate.

dthorpe
My initial thought was that possibly it was frowned upon for a class to self-subscribe as well. From some of the other answers and yours it does not seem to be an issue in .NET. If I do put some additional logic in my OnSomeEventHappens virtual method, I need to take the leap of faith that overriders will call the base version in their override. For my purposes I am still debating whether or not that would be an issue.
jomtois
The main difference between virtuals and events is that events are / should be contractless with little or no demands on what the event handlers should do, whereas virtuals are intrinsically contractual. It is always the case that a virtual method should be documented to describe when and whether it is supposed to be called by descendents when extending or overriding behavior. IOW, if your overriders are good programmers, this shouldn't be a major concern. If your overriders will be the general public / random idiots, then fortifications may be needed.
dthorpe
There is a 99% chance that any overrider will be my future self, whether that puts me into the category of good programmer or random idiot is up for debate! :)
jomtois
You can always guarantee safety by removing the overrider(s) ability to _not_ call the base implementation, if that's what you desire, by tossing in a `virtual On[action]` method. I find I'm quite often the random idiot even against my own code because of laziness. I like to protect myself from myself.
Marc
+1  A: 

If you own the state of your own object, catching events just sounds wrong to me. I would go with a separate virtual method. Don't interfere with your event and hope the children throw it. Maybe this would look something like:

    private object _someProperty;
    public object SomeProperty
    {
        get
        {
            return _someProperty;
        }
        private set
        {
            if (_someProperty != value)
            {
              OnSettingSomeProperty(_someProperty, value);
              OnSomeEventHappens(EventArgs.Empty);
              _someProperty = value;
            }
        }
    }

    protected virtual void OnSettingSomeProperty(object oldValue, object newValue)
    {
        // children can play here, validate and throw, etc.
    }
Marc
This goes back to leaning to Option 1. Although I don't think I would make OnSettingSomeProperty virtual, probably private so I could guarantee it would not be missed. I care less about the event being raised than the conditions that caused it to be raised in the first place.
jomtois
@jomtois, the intent of the `OnSettingSomeProperty` is to allow descendants to inject behavior. It's clear what you have but not what you _want_ in your original post. Perhaps you're not happy with anyone's answer because you haven't defined `Do stuff...` in your question.
Marc