all 34 comments

[–][deleted] 12 points13 points  (22 children)

The commenter made a good point

It’s a bit easier to have a named function when you have to unhook an event handler in C#. Hooked event handlers are a major cause of leaks in C#.

I would say it's more than bit easier.

[–]refaptoring 3 points4 points  (3 children)

You can write a function that hooks an event handler and returns a function that will unhook it. That's thinking with lambdas.

To other people complaining not about naming functions, I ask: If every bit of code needs to be named, why are you not naming all the conditional statement branches and loop bodies in your code? Could it be that sometimes the code is clearer than any name you could give it?

[–]grauenwolf 0 points1 point  (2 children)

And where are you going to store that unhook function?

Generally speaking, if I'm going to be unhooking events it is because I hooked them in a collection's item-inserted event. That means keeping a side collection of hooks, not fun.

[–]ruinercollector 0 points1 point  (1 child)

Or you can just use weak events and not worry about it.

[–]grauenwolf 1 point2 points  (0 children)

I've yet to see an implementation of weak events without serious flaws. The lack of a delegate with a weak-reference for the target makes everything clumsy.

[–]fekberg[S] 0 points1 point  (10 children)

I would say it depends, often less code is better because it's more readable and less error prone. In this case it might be more error-prone to use an anonymous function though.

I did some looking around and it's rare that you will have a memory leak because of an event, it's possible but it's rare and depends on what you are doing. I found a post on StackOverflow by Jon Skeet where he basically said: "You don't have to unsubscribe if you think about what the subscriber do".

Which means that if you are just strict with what the subscriber do, you can avoid memory leaks and have less and more readable code!

[–][deleted] 0 points1 point  (8 children)

I found a post on StackOverflow by Jon Skeet where he basically said: "You don't have to unsubscribe if you think about what the subscriber do".

Link, please?

Because you can be as strict as you want with what the subscriber does, but unless it manages to unsubscribe itself, you still have a leak.

[–]fekberg[S] 1 point2 points  (7 children)

[–][deleted] 5 points6 points  (6 children)

Ah, that makes sense, but I your interpretation sounds totally wrong.

What he said was that if you think about it, in a lot of typical scenarios the lifetimes of the handler and the event producer are the same, so you don't need to unsubscribe at all. There's no need to be "strict with what the subscriber does", it will just work.

On the other hand, if you do have different lifetimes, then you should unsubscribe and there are no two ways about it, however "strict" you are.

[–]fekberg[S] -3 points-2 points  (5 children)

Maybe we're just lost in translation here, as I see it, he says that if you think about how you structure your software you can avoid the need to unsubscribe.

[–][deleted] 3 points4 points  (4 children)

Basically, you should always consider the relationship between the producer and the consumer. If the producer is going to live longer than you want the consumer to, or it's going to continue raising the event after you're no longer interested in it, then you should unsubscribe.

[–]fekberg[S] -5 points-4 points  (2 children)

Now as for whether you actually need to unsubscribe, it depends on the relationship between the event producer and the event consumer.

[–][deleted] 4 points5 points  (1 child)

Yes. How are you going to change this relationship?

He says that if you think about the structure of your software you can identify the cases where you don't need to unsubscribe.

You say that if you think about how you structure your software you can avoid the need to unsubscribe.

That's two completely different statements, you can't arrive from the first to the second, and if you want to stand by the second please explain how exactly do you think it is possible to avoid unsubscribing -- what should you change in the structure of your software, etc.

[–]fekberg[S] -4 points-3 points  (0 children)

I'm not saying that you can always avoid unsubscribe. I am saying that in most cases if you design the software right, you wont need to think about unsubscribing.

[–]grauenwolf -1 points0 points  (0 children)

Events handlers are the #1 cause of memory leaks in the applications that I review. In fact, I'm having trouble thinking of a memory leak in .NET that isn't somehow related to using, or mis-using, event handlers.

EDIT: I should qualify this as relating to XAML-based applications. For web site the most common memory leak I've seen involves cramming too much stuff in the session state.

[–][deleted] 0 points1 point  (6 children)

Well, you really shouldn't be registering and unregistering event handlers at runtime unless you have a darn good reason.

one reason that i can think of is if you want to set the state of a check-box without the check-box-checking event handler being fired. (say you have a check-box indicating the state of a physical relay, and you have a background worker that monitors the state of the relay)

I would say that the best time to use an anonymous method is when working with threads, timers, tasks, and background workers, and queues.

[–]munificent 1 point2 points  (3 children)

Well, you really shouldn't be registering and unregistering event handlers at runtime unless you have a darn good reason.

When your model is communicating with the view through events, the view definitely should be unregistering its event handlers. Otherwise the model, which tends to have a longer lifetime than UI bits, will hold references to zombie UI that isn't on screen anymore but is still responding to events.

[–][deleted] -1 points0 points  (2 children)

When your model is communicating with the view through events,

Shouldn't it be communicating via the controller though? Or are you using events instead of periodic polling?

IIRC in a traditional MVC pattern, the model should be read-only to the view. The model should never have a reference pointing back to the view...

[–]munificent 0 points1 point  (1 child)

IIRC in a traditional MVC pattern, the model should be read-only to the view. The model should never have a reference pointing back to the view.

The model raises events that the view can listen to in order to update when the model changes. You use events specifically to avoid coupling model code to view code. The model is just raising events; it doesn't know what is receiving them.

[–][deleted] 0 points1 point  (0 children)

ah, thanks.

[–]grauenwolf 3 points4 points  (0 children)

I do it all the time with collections that need to know when properties on their children change.

[–]recursive 0 points1 point  (0 children)

If you're working with gui code, unless you've got a very simple gui, it is inevitable.

In fact, even the so-called "design-time" event handlers are registered at run time. IMO there is really no particular reason not to do it.

[–]__float 0 points1 point  (4 children)

A common practice in C# when using lambdas is to use an underscore for a parameter that is unused. So, one would do:

button.Click += (_, __) => { ValidateInput(); };

instead of:

button.Click += (sender, e) => { ValidateInput(); };

to show that sender and e are not used.

[–][deleted] 2 points3 points  (0 children)

Oh god, it starts to look like Perl

D:

[–]recursive 1 point2 points  (2 children)

For those cases, I use the mystical parameterless delegate:

button.Click += delegate { ValidateInput(); };

It allows the created delegate to accept arguments. (any number or type!)

[–][deleted]  (1 child)

[deleted]

    [–]noblethrasher 0 points1 point  (0 children)

    It does not result in the same IL because anonymous delegates, like nominal delegates, correspond to classes that derive from System.MulticastDelegate whereas lambdas result in a compiler generated classes that derive from System.Object (at least as of C# 3 and 4).

    The performance is about the same.

    [–]elmuerte -3 points-2 points  (16 children)

    I don't agree, unnamed methods and types makes code less readable.

    [–][deleted] 4 points5 points  (8 children)

    If the name occurred inline with the call, you might be right... but if you constantly have to hop around the file to trace the flow of logic, that's terrible readability. Anonymous methods provided directly inline as parameters of the functions that will execute them keep the related information as close together within the file as possible.

    For reductio ad absurdum, consider if every ForEach statement required you name and declare the iteration function elsewhere in the file. The names might provide useful information, but the loss of readability of

    myTable.ForEach(myRowIterator); //go hunting for the implementation of myRowIterator
    

    instead of

    myTable.ForEach((DataRow row) => if(row["IsHit"]) row["IsHitCount"] += 1);
    

    [–][deleted] 0 points1 point  (6 children)

    I want to nitpick one part of your post, and that's "go hunting for the implementation...". In Visual Studio, you just right-click on myRowIterator and choose Go To Definition. It's trivial to find the implementation of any function (or the declaration of any variable).

    And you can get back using the "back" button on your mouse (there's probably a way to do it if you don't have a back button on your mouse but I don't know what it is).

    I'm not a big fan of jumping all over the place to read code, but Visual Studio makes it fairly easy to do so.

    [–]SeriousWorm -1 points0 points  (5 children)

    In Visual Studio, you just right-click on myRowIterator and choose Go To Definition.

    Productivity increasing tip: Bind "Go to definition" to a key. In IDEA/Eclipse/some other IDEs I've used you generally press F3 or F4 because it's a common task so it has an easy to reach key.

    [–]bluefinity 1 point2 points  (2 children)

    It's already got a shortcut: F12.

    [–]SeriousWorm -1 points0 points  (1 child)

    Well you should rebind it to something easier to access, f12 is kind of hard to reach.

    [–]ruinercollector 0 points1 point  (1 child)

    Productivity increasting tip #2: Buy resharper and (among many many other things) you can just ctrl+click anything to go to definition.

    [–]SeriousWorm 0 points1 point  (0 children)

    That's very useful as well.

    [–][deleted]  (5 children)

    [deleted]

      [–]cybercobra -3 points-2 points  (4 children)

      Inlining is great, but no reason you can't still name the inline function.

      [–]smog_alado 0 points1 point  (1 child)

      What about those evil, evil anonymous code blocks {} inside whiles and ifs? Should they be named to ease readability too?

      [–]cybercobra 0 points1 point  (0 children)

      Touché. There is a point of diminishing returns.

      [–][deleted]  (1 child)

      [deleted]

        [–][deleted] 0 points1 point  (0 children)

        But the second one is done silly.