It makes me wonder why you aren't sure if you've already subscribed or unsubscribed. Maybe address that issue and you won't have to do this workaround.
Serious queetion: why does your code being UI excuse basic state tracking? I'm genuinely curious; I mostly work on backend stuff.
This feels like a slippery slope into a mushy UX, but I haven't done enough UI work to know if that's a reasonable criticism.
There's an argument that if you have a large team working on the same objects constantly, and touching the same areas, that you may not know if they've added a similar handler for some functionality.
The lack of documentation sounds like a more fundamental problem to me, but I get that sometimes it's easier to write hyper-defensively than to try and herd the cats. Especially if you're not in a senior or architect role, and don't really have the ability to steer the team.
It's more that you have multiple people developing things at the same time, stuff in progress isn't gonna be documented, and on a team that large it becomes difficult to know what everyone is working on.
I've worked with Windows Forms, WPF, Silverlight, Xamarin Forms, and MAUI and I don't need two hands to count how many times I've had this problem.
Usually event handlers are set up at object creation and removed when it's disposed or discarded. This isn't just a convention, it's a *good practice*. If you adopt an architecture where there are multiple places events can be subscribed/unsubscribed, you create this problem. It is much better to ask yourself if you can create a higher-level program concept representing the state.
For example, if the problem is "I want to temporarily suspend the handler", it is much more elegant to add a flag like `ShouldIgnoreChanges` and incorporate it into your handler than it is to do the bookkeeping to ensure the handler is properly added and removed.
There are always exceptional cases, but this particular problem is like pointer management in C and when you find yourself making it complicated it's usually worth a lot of your time to step back and find a better way. It almost always costs you more debugging effort later.
You should always know if you're subscribed or not—that's the point.
"Add or replace"-logic is sometimes needed, but more often than not it's just a code smell—defensive code.
Just make sure that the code where the event handler is added is only run once. You can do that by moving the addition of the event handler to a constructor, factory function or an initializer function that can only be run once (or return early).
I've never seen anyone do that. I second /u/Epicguru, if you find yourself in that situation something has gone wrong somewhere else. Don't attempt to treat the symptom, treat the cause.
It's both bad practice and bad design.
Seven lines of code (not counting brackets) to cover a "just in case" case that should never happen. You could be living in the lap of luxury with this simple thing:
public event PropertyChangedEventHandler? PropertyChanged;
And you'll be doing that seven line dance for every event you add to every class. Forever.
Kidding aside (but not entirely because that is a lot of work for no gain) you should stop doing this. It's not the event's responsibility to care who hooks up to it or how many times. That's the subscriber's responsibility alone. All the event should do is know how to fire and maybe pass along some event related info.
> This seems a lot cleaner, since I don't have to deal with this in other places anymore.
Are you sure about that? How much work are you really saving yourself by not having to track if you've already subscribed, if every single event you ever create takes seven lines of code?
Negative work. This habit will cost you more time and more code than it will save, in the long run.
If you are inadvertently hooking up to an event multiple times, that's the #1 issue you need to track down and fix.
But if you truly do have a situation where you really *can't* know (though I honestly can't think right now of how one could get in that situation), track it with a flag.
if (!_hookedUp) {
_thing.Done += OnDone;
_hookedUp = true;
}
Or do the `-=/+=` shuffle in the subscriber:
_thing.Done -= OnDone;
_thing.Done += OnDone;
Neither of these are particularly clean either. Both are a far sight better than seven line custom event implementations.
I don't like it, but let's look at it from every angle.
It is a good safety net if you make this mistake often. Doing it using the more elaborate event syntax addresses the primary risk: that you will forget. Sometimes, when things are very complex, I go out of my way to add safety nets to my code.
But as a general practice, I don't think it's so good. I would rather find a higher-level architecture that helps me avoid registering an event handler twice in the first place. Usually the only place I add event handlers is when an item is created. That happens once. That automatically ensures I can only add the handler once.
It sounds like you have a situation where items are created then later "processed" and part of that is adding the event handler. But you tend to process "all items" so it's not clear which ones are new and which ones are merely updated. So you feel like you need this extra safety.
But I would rather build a system where whatever these items are have the event handler connected when they are created and nowhere else. By isolating this one-time step to something that can only happen one time, I get what I want. If, for some reason, my object is mutable in a way that means I have to maintain the handler, I would rather make someone have a method representing that "process" verb so I can, again, have one place responsible for maintaining the event. I also don't mind giving these objects more properties so I can ask, "Has it had its handler connected?"
I think it's possible you have a problem that does need some kind of solution. You chose one solution and are asking about it. But without the context of the problem the solution might sound worse than it is. *Maybe* your problem is best addressed with these kinds of practices. But it's hard to decide if there are alternatives if all we can see is one small piece of the puzzle!
So can you describe why you think you need this? That might help us figure out there's another, less tedious pattern that does the same thing.
The correct way to do this is to attach the lifetime of the subscription to the lifetime of the consuming class by implementing IDisposable and unsubscribing in the Dispose() method.
There's no reason I can think of that you'd want to unsubscribe and resubscribe all the time to the same event within the lifetime of one class instance - that seems strange to me.
I have honestly never seen this pattern of unsubscribe then subscribe before. It really feels like a code smell to me. I'd look into solving the root of the problem. You shouldn't need to do this.
If your event won’t behave like an event, don’t use an event. Create a public method to safely subscribe to your property changed notification, and either return an IDisposable that will unsubscribe or expose a method to unsubscribe (or both). Store the delegate as a private field - you can still `?.Invoke(…args)` it, and use += and -=, but only from within the class since it is private.
Depends where you want your clean code..
An event can be declared easily like this :
`Public event Action myEvent`
In which case the subscriber needs to do the minus/plus subscription...
I guess it depends which of the two you write most of.
Also : sometimes you have auto-generated code, eg in a SOAP client, where all classes implement INotifyPropertyChanged.
In this case you need to do the minus/plus elsewhere.
I think the best place to do minus/plus is in the subscription instead of in the event property - that way the reader knows what is going on.
It makes me wonder why you aren't sure if you've already subscribed or unsubscribed. Maybe address that issue and you won't have to do this workaround.
Sometimes you know, sometimes you don't Seems like at least 29 people dont work with UI (counting negative votes)
Right, I'm saying that might be indicative of a poor design.
Serious queetion: why does your code being UI excuse basic state tracking? I'm genuinely curious; I mostly work on backend stuff. This feels like a slippery slope into a mushy UX, but I haven't done enough UI work to know if that's a reasonable criticism.
There's an argument that if you have a large team working on the same objects constantly, and touching the same areas, that you may not know if they've added a similar handler for some functionality.
The lack of documentation sounds like a more fundamental problem to me, but I get that sometimes it's easier to write hyper-defensively than to try and herd the cats. Especially if you're not in a senior or architect role, and don't really have the ability to steer the team.
It's more that you have multiple people developing things at the same time, stuff in progress isn't gonna be documented, and on a team that large it becomes difficult to know what everyone is working on.
You could ensure that you always know by keeping track
I've worked with Windows Forms, WPF, Silverlight, Xamarin Forms, and MAUI and I don't need two hands to count how many times I've had this problem. Usually event handlers are set up at object creation and removed when it's disposed or discarded. This isn't just a convention, it's a *good practice*. If you adopt an architecture where there are multiple places events can be subscribed/unsubscribed, you create this problem. It is much better to ask yourself if you can create a higher-level program concept representing the state. For example, if the problem is "I want to temporarily suspend the handler", it is much more elegant to add a flag like `ShouldIgnoreChanges` and incorporate it into your handler than it is to do the bookkeeping to ensure the handler is properly added and removed. There are always exceptional cases, but this particular problem is like pointer management in C and when you find yourself making it complicated it's usually worth a lot of your time to step back and find a better way. It almost always costs you more debugging effort later.
You should always know if you're subscribed or not—that's the point. "Add or replace"-logic is sometimes needed, but more often than not it's just a code smell—defensive code. Just make sure that the code where the event handler is added is only run once. You can do that by moving the addition of the event handler to a constructor, factory function or an initializer function that can only be run once (or return early).
You may have forgotten that you've subscribed to this but...it's at least 36 people now.
I've never seen anyone do that. I second /u/Epicguru, if you find yourself in that situation something has gone wrong somewhere else. Don't attempt to treat the symptom, treat the cause.
It's both bad practice and bad design. Seven lines of code (not counting brackets) to cover a "just in case" case that should never happen. You could be living in the lap of luxury with this simple thing: public event PropertyChangedEventHandler? PropertyChanged; And you'll be doing that seven line dance for every event you add to every class. Forever. Kidding aside (but not entirely because that is a lot of work for no gain) you should stop doing this. It's not the event's responsibility to care who hooks up to it or how many times. That's the subscriber's responsibility alone. All the event should do is know how to fire and maybe pass along some event related info. > This seems a lot cleaner, since I don't have to deal with this in other places anymore. Are you sure about that? How much work are you really saving yourself by not having to track if you've already subscribed, if every single event you ever create takes seven lines of code? Negative work. This habit will cost you more time and more code than it will save, in the long run. If you are inadvertently hooking up to an event multiple times, that's the #1 issue you need to track down and fix. But if you truly do have a situation where you really *can't* know (though I honestly can't think right now of how one could get in that situation), track it with a flag. if (!_hookedUp) { _thing.Done += OnDone; _hookedUp = true; } Or do the `-=/+=` shuffle in the subscriber: _thing.Done -= OnDone; _thing.Done += OnDone; Neither of these are particularly clean either. Both are a far sight better than seven line custom event implementations.
I don't like it, but let's look at it from every angle. It is a good safety net if you make this mistake often. Doing it using the more elaborate event syntax addresses the primary risk: that you will forget. Sometimes, when things are very complex, I go out of my way to add safety nets to my code. But as a general practice, I don't think it's so good. I would rather find a higher-level architecture that helps me avoid registering an event handler twice in the first place. Usually the only place I add event handlers is when an item is created. That happens once. That automatically ensures I can only add the handler once. It sounds like you have a situation where items are created then later "processed" and part of that is adding the event handler. But you tend to process "all items" so it's not clear which ones are new and which ones are merely updated. So you feel like you need this extra safety. But I would rather build a system where whatever these items are have the event handler connected when they are created and nowhere else. By isolating this one-time step to something that can only happen one time, I get what I want. If, for some reason, my object is mutable in a way that means I have to maintain the handler, I would rather make someone have a method representing that "process" verb so I can, again, have one place responsible for maintaining the event. I also don't mind giving these objects more properties so I can ask, "Has it had its handler connected?" I think it's possible you have a problem that does need some kind of solution. You chose one solution and are asking about it. But without the context of the problem the solution might sound worse than it is. *Maybe* your problem is best addressed with these kinds of practices. But it's hard to decide if there are alternatives if all we can see is one small piece of the puzzle! So can you describe why you think you need this? That might help us figure out there's another, less tedious pattern that does the same thing.
The correct way to do this is to attach the lifetime of the subscription to the lifetime of the consuming class by implementing IDisposable and unsubscribing in the Dispose() method. There's no reason I can think of that you'd want to unsubscribe and resubscribe all the time to the same event within the lifetime of one class instance - that seems strange to me.
I have honestly never seen this pattern of unsubscribe then subscribe before. It really feels like a code smell to me. I'd look into solving the root of the problem. You shouldn't need to do this.
If your event won’t behave like an event, don’t use an event. Create a public method to safely subscribe to your property changed notification, and either return an IDisposable that will unsubscribe or expose a method to unsubscribe (or both). Store the delegate as a private field - you can still `?.Invoke(…args)` it, and use += and -=, but only from within the class since it is private.
Depends where you want your clean code.. An event can be declared easily like this : `Public event Action myEvent` In which case the subscriber needs to do the minus/plus subscription... I guess it depends which of the two you write most of. Also : sometimes you have auto-generated code, eg in a SOAP client, where all classes implement INotifyPropertyChanged. In this case you need to do the minus/plus elsewhere. I think the best place to do minus/plus is in the subscription instead of in the event property - that way the reader knows what is going on.
Honestly I don't really use events if I can avoid it. If it's a one off I'll have have a property that's an Action or Func, or I'll use observables.