Naming standard for events

[Grenkin1988]

I would like to discuss this. Why do we decided on such naming standard as – On… From my practice, events always called just – Changed, Removed, Closed, Click. Quite often methods attached to these events names as – OnClick, OnChanged… There are some discussions about this: Events - naming convention and style Naming, declaring and defining delegates and events conventions [closed]

And even .NET classes do not have this On… Form Events

Event is something that happened to object. It could Changed. And reaction to this could be OnChanged.

Names of Type Members

Names of Events Events always refer to some action, either one that is happening or one that has occurred. Therefore, as with methods, events are named with verbs, and verb tense is used to indicate the time when the event is raised. ✓ DO name events with a verb or a verb phrase. Examples include Clicked, Painting, DroppedDown, and so on. ✓ DO give events names with a concept of before and after, using the present and past tenses. For example, a close event that is raised before a window is closed would be called Closing, and one that is raised after the window is closed would be called Closed. X DO NOT use "Before" or "After" prefixes or postfixes to indicate pre- and post-events. Use present and past tenses as just described. ✓ DO name event handlers (delegates used as types of events) with the "EventHandler" suffix, as shown in the following example: public delegate void ClickedEventHandler(object sender, ClickedEventArgs e); ✓ DO use two parameters named sender and e in event handlers. The sender parameter represents the object that raised the event. The sender parameter is typically of type object, even if it is possible to employ a more specific type. ✓ DO name event argument classes with the "EventArgs" suffix.

[sboigelot]

Feel free to change, the naming convention you propose make sense :)

A side question is: when to use event and when to not use event. Events brings a lot of overhead, while sometime, if you just want to register one action and not N actions, you could just use an Action.

I would love to have your feedback on that too.

[Grenkin1988]

@sboigelot As for me, this no big difference 1 subscriber or more. This is used to decouple objects from each other. One of them is just saying - I did this, and it does not care whether anything is interested in this. But maybe I did not really get what you meant by

you could just use an Action

[sboigelot]

An event in c# will use a MultiCastDelegate, a not very fast piece of code checking if it is referenced by other object.

It's great when you need multiple observers.

But if you know you need only one observer you can just create a property or field of the type Action

for instance:

public class DialogBoxTrade : DialogBox
{
    public Action OnTradeCompleted;
    public Action OnTradeCancelled;
    public void AcceptTrade()
    {
        if (trade.IsValid())
        {
            ...
            if (OnTradeCompleted != null)
            {
                OnTradeCompleted();
            }
        }
    }
}

somewhere else:
dbm.dialogBoxTrade.OnTradeCancelled = () => { TradeCompleted = true; };

This is much faster to execute but will only work with 1 observer

(of course we can apply the same naming convention later)

[koosemose]

If we're going with this style, we need to watch the tenses. i.e from your example, OnChanged shouldn't respond to Change. Change is present tense, so that means the change is happening now, OnChanged means it has already happened.

[zwrawr]

off topic but Woo #1000 th thing :)

[TomMalbran]

I am more of a Javascript developer, and I am used to the jQuery way of registering events using on and off. Since now we don't have register and unregister methods, and we use Event += eventFunctionName, it feel like using something like OnEvent += eventFunctionName is clearer to read.

But I am also ok with not adding the On prefix.

[Grenkin1988]

@koosemose That was my typo @sboigelot It is good way, but you need really really be sure nobody would change your subscription.

[vogonistic]

The fact that we use On as a prefix is partially my fault. I thought I remember quill using it and went with it for StyleCop Saturday. I'm totally cool if someone wants to receive our events and remove the On prefix and ensure they are properly named.

[TomMalbran]

@sboigelot even if you only need 1 subscription now, you need to think if mods or future systems would want to register to those events for some reason.

[sboigelot]

@Grenkin1988 yep, it's more about optimization later down the developement process. I think we should accept both events and actions but have the same naming convention on both.

Someone changing the subscription would create a compiler exception as Actions do not support += -+

@TomMalbran yes that's true.

[Grenkin1988]

@sboigelot He can use = and be happy with it :-) But this is offtop, and I am not against delegate properties.

[koosemose]

Hard to be sure, but it was useful as an example. But it would help for us to try to ensure the tenses of the events correctly signal when the event is fired relative to the actual thing happening.

Though to be clear, my personal opinion is in favor of On... as it reads more clearly, and I feel code that reads smoothly is easier to mentally parse... but that may be my perl background coming through.

[sboigelot]

We could strawpoll the naming convention for a few days, everyone seems to be between yes, no, maybe and "can you repeat the question?"

[Raketfart]

I prefer OnChanged - it's easy to see that it's an eventlistener, but that maybe just old habits from Actionscript or something.

[Grenkin1988]

That make sense. Could someone from gatekeepers @vogonistic @Mizipzor @WardBenjamin @quill18 do this vote?

P.S.: I explained my point and gave some references. For it makes so many sense to have:

furniture.Changed += OnFurnitureChanged
character.Changed += OnCharactedChanged

But if majority will decide otherwise it's OK

[vogonistic]

I vote yes.

I also want to add that Action only makes sense if it's private and only you use it. If we make it publish it'll have to be an event for modders and whatnots.

[bjubes]

or we could do publiceventAction<T> onSomethingChanged so that you can't overrite the subscription and so it can't be called by an object other than itself (ie triggering it is protected)

technically its not necessary if its a private one time thing, but this allows the code to be expanded on without seeing weird bugs in the future, which is necessary IMO in open source

[alexanderfast]

As for the name Im fine with thatever, but I think an On prefix on the handler is more common.

Though I prefer to avoid event as much as possible, not because of the overhead but the fact that you need to explicitly unhook the handler. An Action<> callback is always better. But thats maybe for another topic.

[Grenkin1988]

@Mizipzor

As for the name Im fine with thatever, but I think an On prefix on the handler is more common.

Yes for handler, but not for the name of event. Event handler is method that you attach to event.

Though I prefer to avoid event as much as possible, not because of the overhead but the fact that you need to explicitly unhook the handler. An Action<> callback is always better. But thats maybe for another topic.

This is drawback of event usage. But we just need to make sure everyone unsubscribe from event on object destruction. Simple delegate usage as I said earlier could lead to someone override your initially desired behaviour by simple new handler assigned

[alexanderfast]

Since C# is a managed language the need for unsubscribing to event is commonly forgotten. You trade that caveat and a slight performance overhead for the ability to have many listeners. As such, I think it's really important to not default to events just in case that someone, some day maybe might want to add another listener. Start as Action, become event only when you need.

Changing between them as refactoring is easy. Except that events needs lots of boiler plate code. Which is another argument against them.

[vogonistic]

I just want to point out that modders can't change it from an action to an event, so if we think it's going to be relevant to a modder, it might be worth going with event from the start.

[sboigelot]

Modders will only add lua & xml files, so any extensions points should already be defined by our mod API. If we wish to expose a callback to the mod API, then yes, we should have an event.

[Grenkin1988]

Let's stick to discussion :-) How are we going to count votes? As I see it is not very obvious what solution is "winning"

[sboigelot]

@Grenkin1988 http://www.strawpoll.me/11107530 ?

[Grenkin1988]

@sboigelot I've voted already

[Grenkin1988]

How long this and vote should be open?

[sboigelot]

@Grenkin1988 The results are obvious for now, I think we can assume it won't change if we leave it open longer

[koosemose]

With the low number of total votes, it doesn't take much to swing it one way or another. Earlier today it was tied, and only 3 votes for the second option puts it back to tied.

[NogginBops]

@koosemose Yeah, I agree we are a lot of people on this project and because it's moving so fast a lot of people could have missed this issue.

And if I remember right the guidelines were close to the one Microsoft suggests, and they suggest the OnBlah naming scheme. I makes it easier to instantly recognize that it's an event.

[Grenkin1988]

@NogginBops Please show me where Microsoft suggests OnBlah... I have gave link where it says opposite.

[dusho]

OnBlah is I think old school.. in WPF e.g. there is no On.. anymore Either way is ok, On... keeps events separated from properties for autocompletion.. ..ed is according to latest used guidelines

[Grenkin1988]

I am a little bit tired from this. People saying that use Microsoft standard for everything and at the same time saying that On.. is good name. As I said On... is good name for event handler, not for event itself. Someone need to decide what to do with this. @vogonistic @Mizipzor @WardBenjamin @quill18

[Raketfart]

I think the vote already decided that we do as you suggested

[alexanderfast]

We have to redo the vote due to ambiguity. To me its not clear if we were voting on the event itself or the handler. From this:

This is the event itself:

public delegate void ChangedEventHandler(object sender, EventArgs);

This is the event handler:

private void ListChanged(object sender, EventArgs e) 
{
    Console.WriteLine("This is called when the event fires.");
}

And to be extra clear, heres how you hook it up:

List.Changed += new ChangedEventHandler(ListChanged);

Going by this pattern, the event has the name of VerbInPastTenseEventHandler, a suffix. This is the important one to get right because its visible and fixing that means fixing in a lot of other places. If the event isnt used in a lot of other places, it should have been an Action<> callback to begin with.

The event handler here is private so I dont think its that important to get right. Its also clear to me that it is an event handler from the signature, it has sender and event args.

From the example I linked it seems Microsoft is using an On prefix in the class that actually invokes the event. Which, since its private, I care less about.

Have we nailed the terminology now?

[Grenkin1988]

@Mizipzor This:

public delegate void ChangedEventHandler(object sender, EventArgs);`

is not event! This is declaration of delegate This is event that use this delegate:

public event ChangedEventHandler Changed;

And event can have whatever delegate as its type. It could be custom created type as you shown or it could be one of existing delegate Action, Action<>

[sboigelot]

The vote is about event, not event handlers.

[vogonistic]

The MSDN guidelines says:

✓ DO name events with a verb or a verb phrase. Examples include Clicked, Painting, DroppedDown, and so on. ✓ DO give events names with a concept of before and after, using the present and past tenses. For example, a close event that is raised before a window is closed would be called Closing, and one that is raised after the window is closed would be called Closed.

Based on this:

1. The vote is at least slightly 57% (8/14) in favor of going down the MSDN route
2. The discussion on this issue has stagnated
3. The issue been open for more than 2 days
4. Following the MSDN guidelines is in line with what we already state in CONTRIBUTING.md

I'm hereby using the dark gatekeeper powers vested in me to declare this issue settled in favor of following the MSDN guidelines when it comes specifically to naming events.

Come to me on Discord for complaints regarding my use of dark gatekeeper powers.

[alexanderfast]

@Grenkin1988 You are absolutely right, I was in a hurry and copied the wrong line. >.<