2

I develop/maintain an in-house library in a growing company with about 12 other developers that write code. This library is mostly used for scripting game behavior and we have many different projects that make use of it.

This library implements simple, typed, event-aggregation in a static class EventBus.

It has the expected methods for subscribing and unsubscribing event-handlers: Subcribe<TEvent>(Action<TEvent> handler) and Unsubscribe<TEvent>(Action<TEvent> handler)

My conundrum is a 'convenience method' I've added: SubscribeUntilTrue<TEvent>(Func<TEvent, bool> handler), which is allows creating (anonymous) event-handlers which can decide if they wish to receive further event-messages by returning a boolean value. Essentially, they will be unsubscribed after some condition is fulfilled.

Example:

EventBus.SubscribeUntilTrue<DialogEvent.End>(e =>
{
    if (e.Dialog.Name == "MyDialog")
    {
        DoSomething();
        return true;
    }
    return false;
});

The goal of having this method is to allow the developer using it to signal intent more clearly, reduce boilerplate and avoid declaring methods that are called only once. Below are two examples of code that do the same thing as the snippet above in a way which I think is less readable/intuitive/maintainable:

First Example: Using a named method, which...

EventBus.Subscribe<DialogEvent.End>(EndHandler);

...is declared somewhere else.

private void EndHandler(DialogEvent.End e)
{
    if (e.Dialog.Name == "MyDialog")
    {
        DoSomething();
        EventBus.Unsubscribe<DialogEvent.End>(EndHandler);
    }
}

This is not ideal because the intent can only be communicated through the name of the handler-method at the point of subscription and it is not clear if there are any other callers to the handler. I suspect that in many cases the name can easily become very verbose and or inaccurate as the typical code inside such a handler is also likely to change often while in development while also being short.

Second Example: A developer avoiding the declaration of a 'once-called' method might use a self-unsubscribing delegate/anonymous method which is a bit messy/ugly:

Action<DialogEvent.End> endHandler = null;
endHandler = e =>
{
    if (e.Dialog.Name == "MyDialog")
    {
        DoSomething();
        EventBus.Unsubscribe(endHandler);
    }
};
EventBus.Subscribe(endHandler);

Developers who are not familiar with this pattern can easily become confused.


I am not sure the name SubscribeUntilTrue is adequate for people to intuitively understand what the method does and what meaning it assigns the required return value of the handler.

It is generally considered to be a smell to include a condition(al) in the name of a method, like SubscribeUntilTrue. It was initially named just SubscribeUntil but I had to explain to a fellow developer what it does and he suggested to add the True part.


I've had an idea to change the signature of the method (or add an overload) so that the handler-function's return value is a sealed enum-like struct with two static properties that serve as 'constants' which would denote the intended usage more clearly:

EventBus.SubscribeUntil<DialogEvent.End>(e =>
{
    if (e.Dialog.Name == "MyDialog")
    {
        DoSomething();
        return EventHandlerResult.Unsubscribe;
    }
    return EventHandlerResult.KeepSubscribed;
});

This 'enum-like' struct with only two possible values has implicit conversions to/from bool so that switching between which overload is used is more convenient.

public struct EventHandlerResult
{
    private readonly bool shouldUnsubscribe;

    public static readonly EventHandlerResult Unsubscribe = true;

    public static readonly EventHandlerResult KeepSubscribed = false;

    private EventHandlerResult(bool shouldUnsubscribe)
    {
        this.shouldUnsubscribe = shouldUnsubscribe;
    }

    public static implicit operator bool(EventHandlerResult state)
    {
        return state.shouldUnsubscribe;
    }

    public static implicit operator EventHandlerResult(bool value)
    {
        return new EventHandlerResult(value);
    }
}

Though this has some obvious issues like; If I change the signature it would force consumers to change the signature of handlers they've already written. I could keep both versions though.


Anyone out there have any idea of a better name for this method? Also is the enum-like struct a good/bad approach to guide/force the clarity/intent of code written consumers of the library?

  • 1
    I would discourage this. Events are best for code that must be uncoupled using a designer or something similar that generates code. If you have a one shot notification, it should be a function pointer (delegate in C#), not an event. And ideally it is passed in the constructor of the "operator". If you need to unsubscribe and subscribe on a regular basis, probably something about the design could be simplified: perhaps you should reduce the number of events and only put them on root nodes of your hierarchy (bubble up notifications). – Frank Hileman Apr 26 '18 at 21:42
  • 1
    The rationale for this is that subscription and unsubscription is a bookkeeping operation that provides one more opportunity for errors and memory leakage. Automatic unsubscription can also be handled on the caller end via a using statement pattern and the appropriate helper structure, but it is best to not need it. – Frank Hileman Apr 26 '18 at 21:44
  • I'm with Frank here. It's more acceptable to keep the handler (but have it do nothing based on an internal tracked state), as opposed to (un)subscribing at unspecified times. The overhead cost is negligible and it keeps the code better contained. – Flater May 28 '18 at 13:52
  • I am pretty sure that most of your EventHandlers would lack the context to decide whether they would be interested in more events. – tofro May 28 '18 at 14:09
1

It looks like when you use this code you're doing this:

Decide that as Dialog Event End happens, when condition is true, you need to do something, only once, then unsubscribe.

The problem with Frank Hilemans suggestion of just using a function pointer is that condition B might change state in the time from when you decide to when event A happens. If that's not an issue then please do find a way to simplify.

Otherwise what you're really looking for here is semantic help. You want something easy on the eyes that the newbies will understand at a glance. While we're at it, how about we stop making people write the code that signals the result?

Action<DialogEvent.End> endHandler = null;

endHandler = EventBusLambdaBuilder
    .DoneWhen( e -> Condition(e) )
    .DoOnceThenUnsubscribe( e -> Something() )
    .Build() 
;

EventBus.Subscribe(endHandler);

Now whether you use enums or not isn't an idea that is spreading all over the place. The implementation is well hidden. This is a little something called DSL (Domain Specific Language). But wait, this is C#. You have named arguments.

endHandler = EventBusLambdaBuilder(
    doneWhen: e -> Condition(e),
    doOnceThenUnsubscribe: e -> Something()
);

If that provides all the flexibility you need then you don't need to go as far as a DSL. It still makes whats going to happen clear without spelling out how.

  • @KaspervandenBerg forget my own head next. Thx again. – candied_orange May 28 '18 at 15:39
1

You are re-inventing the wheel a bit here.

Using System.Reactive.Linq you have the following naming and approach.

This

EventBus.SubscribeUntil<DialogEvent.End>(e =>
{
    if (e.Dialog.Name == "MyDialog")
    {
        DoSomething();
        return EventHandlerResult.Unsubscribe;
    }
    return EventHandlerResult.KeepSubscribed;
});

Assuming EventBus is cast to IObservable becomes this:

EventBus.Where(e=>e.Dialog.Name=="MyDialog").Take(1).Subscribe(_=>DoSomething());

If you are concerned about dependencies, performance, etc., you could perhaps look into your own Linq-to-YourEventBus approach.

0

Don't use the name, use the delegate type

Naming is one thing you have at your disposal. But you also have the delegate and event types you can use. You are right to think that using just the name is weak; using the type system to your advantage is much stronger.

I would suggest you define a delegate type specifically for handlers that can unsubscribe themselves, and give that delegate a simple and easy means to perform the unsubscription. That way, the intention is communicated to the developers-- they will be forced to use the special delegate type-- and will be obvious to anyone reading the code. Also, you will have the additional safety of compile-time checking.

Also, this technique allow the capability to unsubscribe to be injected into the handler, giving you all the advantages of dependency injection-- the handler, for example, won't have figure out what EventBus instance to call in order to unsubscribe, and you can easily mock the unsubscription interface in unit testing.

For example:

//Define an interface that we can pass to the handler
public interface IUnsubscribe
{
    void Unsubscribe(TransientEventHandler handler);
}

//Define the handler that accepts our new interface
public delegate  void TransientEventHandler(object sender, EventArgs e, IUnsubscribe unsubscribe);

//Implement the interface in the bus itself
public class EventBus : IUnsubscribe
{
    protected event TransientEventHandler Clicked;

    //Allow regular handlers
    public void Subscribe(EventHandler handler)
    {
        Clicked += delegate (object sender, EventArgs e, IUnsubscribe bus) { handler(sender, e); };
    }

    //Allow transient handlers
    public void Subscribe(TransientEventHandler handler)
    {
        Clicked += handler;
    }

    //Raise the event
    public void OnClicked()
    {
        if (Clicked != null)
        {
            Clicked(this, new EventArgs(), this);
        }
    }

    //Support unsubscription
    public void Unsubscribe(TransientEventHandler handler)
    {
        Clicked -= handler;
    }
}

//Now test it all
public class Program
{
    static private bool unsub = false;

    private static void SimulateClick(EventBus bus)
    {
        bus.OnClicked();
    }

    public static void MyHandler(object sender, EventArgs e, IUnsubscribe unsubscribe)
    {
        Console.WriteLine("MyHandler - Handling click");
        if (unsub)
        {
            Console.WriteLine("MyHandler - Unsubscribing");
            unsubscribe.Unsubscribe(MyHandler);
        }
    }

    public static void Main()
    {
        var e = new EventBus();
        e.Subscribe(MyHandler);

        Console.WriteLine("Main - sending click");
        SimulateClick(e);

        Console.WriteLine("Main - telling handler to unsubscribe next time it runs");
        unsub = true;

        Console.WriteLine("Main - sending click");
        SimulateClick(e);

        Console.WriteLine("Main - sending click");
        SimulateClick(e);

        Console.WriteLine("Main - sending click");
        SimulateClick(e);
    }

}

Output:

Main - sending click
MyHandler - Handling click
Main - telling handler to unsubscribe next time it runs
Main - sending click
MyHandler - Handling click
MyHandler - Unsubscribing
Main - sending click
Main - sending click

Code example on DotNetFiddle

P.S. regarding your original question, which was about naming, I would suggest a name such as Transient or Temporary, e.g. TransientEventHandler.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.