6

I have a class that has a callback function that gets triggered on some event. The user of the class instance is expected to do stuff on the instance itself, inside this callback function. So I am wondering if I should add the instance itself as an argument to the callback.

type CallbackType = (foo: Foo, n: number) => void; // <-- 1
// type CallbackType = (n: number) => void;           <-- 2

class Foo {
  private n: number; // something that keeps changing
  private callback: CallbackType;

  addCallback(callback: CallbackType) { this.callback = callback; }

  eventloop() {
    /* do some stuff that changes `n` */
    if (someTrigger) this.callback(this, this.n); // <-- 1
    // if (someTrigger) this.callback(this.n);       <-- 2
  }
}

class Bar {
  private foo: Foo;

  constructor() {
    this.foo = new Foo();
    this.foo.addCallback((foo, n) => { // <-- 1
        this.something(n);
        foo.someFunc();
    });
    // this.foo.addCallback((n) => {      <-- 2
    //     this.something(n);
    //     this.foo.someFunc();
    // });
  }
}

In #1, with the foo argument, you expect stuff to happen with foo inside the callback (like in the visitor pattern). #2 looks shorter and simpler, but feels sneaky since you do something with the instance itself, within the callback, (kind of) without Foo knowing it. How should I do this?

5
  • 1
    Well with option one you can extract that to a function or a method, which you can then easily unit test. So I consider that a major upside.
    – inaba
    Commented Jun 24 at 10:55
  • Is this about TypeScript?
    – Bergi
    Commented Jun 24 at 17:21
  • 1
    With option 2, you must have an existing instance of Foo that you can lambda-capture. If your class Bar had fields foo and foo1, and you used the wrong one (say, by copy-pasting the code and forgetting to edit it), would that cause a problem? With option 1, you can write the callback anywhere in the code, without having an instance, and you have control over what is passed in as the Foo parameter, and when. The lambda capture is fine for ad hoc stuff; here, since "the user of the class instance is expected to do stuff on the instance itself", option 1 is a more natural way to model that. Commented Jun 24 at 20:43
  • @Bergi My application is in typescript (hence the ts sample code), but the question is really language agnostic.
    – doca
    Commented Jun 25 at 8:07
  • @doca I'd argue that there is no language-agnostic best practice, rather conventions depending on the ecosystem and application area. For example, in the DOM and libraries like jQuery, event handlers are called with the target instance as the this argument as well as the .currentTarget property of the event argument.
    – Bergi
    Commented Jun 25 at 13:09

6 Answers 6

7

No I think it's fairly common for events to pass the object raising the event and the event arguments to a handler.

I've seen it said that this is so events can be bubbled up through several handlers while retaining information from the original raiser.

ie

<page>
<control>
<button onclick="raiseevent">
</control>
</page>

function raiseevent(e) {
    console.log(e.target.name)
}
//output: 
//button

However, it has been suggested that your solution 2, with context capture can always be used to wrap a function that requires a reference to the target. ie.

<button onclick="(e) => { e.target= this; raiseevent(e); }">

Although here you can see that because we have to create the binding declaratively in the markup you don't have a reference to the button. to use in the scope of the lambda

9
  • 2
    Retaining information about the original raiser is clearly a leaking abstraction.
    – Basilevs
    Commented Jun 24 at 18:03
  • I think in questions of "good practice" you have to take some account of popular usage. If Microsoft and all HTML use the pattern for the last 30 years, that's quite a lot of opinion on the "its good" side.
    – Ewan
    Commented Jun 24 at 21:27
  • There are plenty of older design flaws around. Microsoft is known for strong backward compatibility, they did worse hacks to adhere to it. Do not mistake perseverance for wisdom.
    – Basilevs
    Commented Jun 24 at 22:08
  • 2
    Sure, I'm not saying its "good" cos MS does it. But it solves a problem and is a common (ubiquitous?) pattern. Can we really say its bad practice?
    – Ewan
    Commented Jun 25 at 8:06
  • 1
    There is no problem is solves. Not since C. See my answer. Yes, propagating the original raiser in high-level languages is a bad practice. (I'm talking about C# and the like, not sure about HTML from your example, there is no "propagation" there).
    – Basilevs
    Commented Jun 25 at 8:25
5

It isn't that passing this is problematic. Variation number 1 is making some assumptions that are not immediately apparent — assumptions that variation number 2 does not make.

Variation 1: Publisher Passes Itself to Callback

this.foo = new Foo();
this.foo.addCallback((foo, n) => {
    this.something(n);
    foo.someFunc();
});

We have this.foo and foo. As someone unfamiliar with this code, my immediate question becomes "are this.foo and foo the same object?" If they are the same object, my immediate follow-up question is why are you not using this.foo to make this fact explicit in the code, so I don't need to guess? Given this one code example, I cannot see why the callback needs to be told which foo triggered the event, but context matters here. Within the context of these 5 lines of code, variation number 1 is confusing and over-engineered because the subscriber knows about the publisher.

The conversation changes when publisher and subscriber are decoupled so that neither has a reference to the other. Consider a situation where your class is subscribing to an event via an event bus. The subscriber knows about the event bus, but not the publishers. In this case, it makes sense for foo to be passed to the callback:

constructor(eventBus) {
    eventBus.addCallback("foo", (foo, n) => {
        // do stuff with the instance of foo that triggered the event.
    });
}

This feels like a more appropriate use case for passing the object that triggered the event. I've written lots of code that does this. The drawback is that a lot more code in your system knows about Foo than it might need to. Any change to the public interface of Foo could break subscribers, requiring more code changes than you might have anticipated. The pain sets in when you need to change Foo (more on that towards the end of my answer).

Variation 2: Subscriber Uses Its Own Foo

this.foo = new Foo();
this.foo.addCallback((n) => {
    this.something(n);
    this.foo.someFunc();
});

At first glance, this is easier to read in this specific use case. The use of the this keyword makes the intent of this code explicit: when the callback is executed, we do something(n) and then call another method on my own foo. The code is easier to trace through in my head. But why does my code need to call this.foo.someFunc() when this.foo is triggering the callback? Shouldn't calling this.foo.someFunc() be an implementation detail of this event? Why can't my callback simply be (n) => this.something(n) and have this.foo be responsible for calling someFunc() on its own? Again, we are missing context. Names like "foo" and "bar" remove much of the semantics necessary to understand the relationship between these two objects.

The question is focused on whether Foo should pass itself in the callback, when I think the real issue is that the consumer of this Foo event needs to tell Foo to do something in the callback. There could be legitimate use cases for this, so it's not immediately a sign of a design problem. It's just suspicious.

The Variation You Choose Depends On Intent

Either variation can work, and both have their use cases. Document Object Model events are a great example of this. The target of the event is passed as part of the event object, which is the DOM node that triggered the event. I would like to take each variation in your question and change the perspective from meaningless "foo" and "bar" names to DOM events:

DOM Events (Variation 1)

// containerElement is some ancestor HTML element that contains the button
constructor(containerElement) {
    this.button = containerElement.querySelector("button");
    this.button.addEventListener("click", async event => {
        event.target.setAttribute("aria-busy", "true");
        await this.saveData();
        event.target.setAttribute("aria-busy", "false");
    });
}

The programmer needs to know that event.target is the <button> element. Provided there is sufficient documentation for this event (and there is), you can assume the programmer knows that this.button and event.target reference the same object, as long as your button is in the form <button>Text</button>. This assumption becomes problematic if you have nested elements inside the button, for example an icon:

<button>
    <span class="icon-save"></span>
    Save
</button

The user can click on the icon, at which point event.target is a <span> tag, not a <button>. It makes no sense to mark the icon "busy" while you save information.

DOM Events (Variation 2)

// containerElement is some ancestor HTML element that contains the button
constructor(containerElement) {
    this.button = containerElement.querySelector("button");
    this.button.addEventListener("click", async event => {
        this.button.setAttribute("aria-busy", "true");
        await this.saveData();
        this.button.setAttribute("aria-busy", "false");
    });
}

The code is easy to read, and doesn't make any assumptions about which element event.target references. You can add an icon to your "Save" button and the callback still functions as it did before. In this narrow use case, variation #2 is preferable because it makes fewer assumptions (and we will ignore for a moment that event.target is still a reference to an element).

Yet again, context matters. DOM events are a general purpose notification that bubbles up and down a hierarchy of nodes. DOM events need to support many other use cases, such as event delegation. This allows us to decouple subscriber (the callback) from the publisher (the button being clicked) by listening for events on an ancestor element (which is effectively the event bus):

document.documentElement.addEventListener("click", event => {
    let message = event.target.getAttribute("data-confirm");
    let showConfirmationDialog = event.target.nodeName === "BUTTON" && message;

    if (showConfirmationDialog && !confirm(message)) {
        event.preventDefault();
    }
});

Now we need to assume that event.target is a <button> element, because we are binding the event listener (your callback) to the document.documentElement object, which is the <html> tag at the root of the DOM tree (the event bus). We make our assumption explicit by testing the object to see if it is a button and that it has the data-confirm attribute before showing a confirmation dialog to the user. We make an assumption, but we also test that assumption and gracefully handle situations where our assumption is invalid. Event delegation is a good reason to choose Variation 1, but we can also see how Variation 1 introduces some ambiguity in situations where we want to be specific about the object that triggered the event.

Deciding Which Variation To Use

Both variations can be the ideal choice, or a non-ideal choice depending on how you, as the code author of the interaction between Foo and Bar, intend for this interaction to take place. If you or your team built these components, then you have control over the code and how these components interact.

Variation 1: If you need Bar and Foo to be decoupled using an intermediary component, such as an event bus or some other framework code, then Bar will not hold a reference to Foo. Now it makes sense to pass Foo to the callback if Bar needs to call methods on Foo.

Variation 2: If Bar will always have a reference to Foo when the callback is executed, there is no need to pass Foo to the callback. You keep the relationship between these two objects explicit and easily understood by calling this.foo.someFunc() in the callback.

There is one other option, which you might want to consider:

Variation 3: If Bar and Foo must be decoupled, and you only need data from Foo, consider passing the data in the callback instead of an entire Foo object. This prevents knowledge about Foo from spreading around to a bunch of other objects. Changes to the public interface of the Foo class cascade to many other places in your application, making code changes more widespread than you initially planned for.

Subscribers to the callback are all coupled to Foo anyways, and the only way to truly decouple publisher from subscriber is to pass around information as its own object with its own interface. Now you can change Foo any way you like, and subscribers are not impacted as long as the event data interface does not change.

This design problem isn't a fork in the road where one path is always better than another. I think Yogi Berra put it best:

When you come to a fork in the road, take it.

Any of the variations will work. Choose the one that suits your current needs without causing confusion for other developers while minimizing the pain incurred by changes to Foo and Bar.

0
3

Passing an object as an callback argument is a common practice in languages without closures/lambdas/context capture.

For example, in C there is no way to make a callback enhanced with any kind of context - to access the context, every callback has to be paired up and passed around with an additional context object. Such context object is a pain to create, maintain and enhance.

It is no surprise then, that designers of any API accept (as an option) callbacks that do not require context at all - they were so much easier for the client to create. However, a callback without context either has limited functionality or relies on error prone global state to do anything useful. API designers realize this and try to supply the (potentially) context-less callback with as much useful information as they can. Obviously, there is not much they can do without knowledge of client specifics, but supplying a subject as an argument is a no-brainer.

Special patterns were developed around this technique. Instead of passing around a context object, data could be stored in customizable fields of subject and then queried by callbacks as needed.

Here is an example of a drawing procedure callback in GTK:

void
(* GtkDrawingAreaDrawFunc) (
  GtkDrawingArea* drawing_area,
  cairo_t* cr,
  int width,
  int height,
  gpointer user_data
)

Here, callback author has a choice - either they pass around their own customizable context as user_data in callback setter (at cost of its design and lifetime management) or they can propagate data via custom fields with get_data method of GtkDrawingArea.

High-level languages have closures, virtual dispatch, wide pointers and other means of late binding and no need for additional context. Some C++ components do not use virtual methods in an optimization effort and have to apply low-level techniques. Do not be like them unless you are an embedded software engineer.

Just use the tools you've been given and be glad, you were not born in 60s.

A rule of thumb is - do not pass anything unnecessary in any APIs including callbacks.

2

The example is artificial enough to make the answer clearly "it depends".

Ask yourself: will code like

  this.foo2 = new Foo();
  this.callback(foo2, this.n);

result in correctly working code? Or will it become a bug when the callback is called with a different Foo instance than the one where it belongs to?

If it results in a bug, you can make the code less errorprone by removing the redundancy and ommitting the superfluous Foo parameter. If it makes sense, then go ahead, use variant #1 with the additional Foo parameter.

5
  • +1 If there are multiple ways to provide information, all but one are unnecessary. Unnecessary code is more code to maintain and is a source of bugs. Avoid unnecessary APIs.
    – Basilevs
    Commented Jun 24 at 18:05
  • 1
    given that the callback is private and called by the instance. You cant pass random foo's into it
    – Ewan
    Commented Jun 24 at 21:29
  • @Ewan random or not, they could violate whatever weak contracts this callback was designed with.
    – Basilevs
    Commented Jun 24 at 22:05
  • 2
    @Ewan: thats why I don't like questions in terms of Foo, Bar, "something" or someFunc. Such questions often reflect the askers misconception about "structure matters", but what really matters is the semantics, the part they stripped out of their real code when posting here..
    – Doc Brown
    Commented Jun 25 at 5:59
  • sure, but i dunno, in this case the pattern and code is that the callback is invoked from inside the class. So it will always pass this and n to the callback
    – Ewan
    Commented Jun 25 at 8:00
-1

You do what you need to do, and preferably you use a pattern that others use successfully. The pattern depends on your environment.

An example: On macOS and iOS you have fields in the UI that allow entering text. When the user finishes editing a field, you need to check that the content is acceptable. This code needs to be flexible, so you use the open/closed principle (in this case, it’s the right thing to do).

The languages used allow calling a method on any instance implementing it. The typical way is: The developer creates an instance of avll with a method on any “bool finishedEditing (UITextField* field) and sets this instance as the delegate object. When the end user takes an action that would normally finish editing the textfield, the implementation calls the “finishedEditing” method of the delegate, passing the text field (in case there is more than one with different code)

The “finishedEditing” method supplied by the developer examines the text field and returns true if everything is fine, false otherwise. The implementation of Textfield then accepts changes or not.

With this scheme, the only thing that needs setting is the “delegate” instance. Different text fields will have different delegate instances. Each delegate has quite a few methods / callbacks to handle.

In Swift, there are closures (pieces of code that can catch arbitrary context). You would set a “finishedEditing” closure for each text field, which would have collected s as LLM kinds of context. In C++, if your callback is just a function, all context needed must be passed to a function. That might include a “this” pointer. The callback function might call an instance method of *this, you would not want large amounts of code in a free function.

Do you don’t want to reinvent the wheel. Implement your callback the callback the same way others do.

PS. In this example, there are three important objects: A view containing half a dozen text views and some others. The text field being edited. And whatever object the application is using to handle everything. So which one is “this”?

-2

The key difference between the two can be summarized as the "this" being injected versus captured.

Typically, there are two points to be considered:

The first point is the benefit of allowing the same instance of anonymous function to be reused for different instances of Foo. It seems that you won't benefit from this; as a result, the two approaches (inject vs capture) doesn't seem to matter to you.

anonymous_callback = (foo, n) => { // <-- 1
    this.something(n);             // this "this" refers to "Bar"
    foo.someFunc();                // this "foo" is passed in, i.e. injected
}

this.foo_one = new Foo();          // imagine Foo is a button
this.foo_two = new Foo();          // another button
this.foo_one.addCallback(anonymous_callback);
this.foo_two.addCallback(anonymous_callback);

The second point is the risk of object lifetime issue, i.e. whether the lambda might be invoked beyond the lifetime of its captures (the captured "this"). C++ is the only language that I know of that can potentially suffer from this type of risk. Thus, it doesn't seem to affect your chosen language.

Conclusion: I think the approaches are roughly equivalent in your use case.

1
  • 1
    I don't understand what "injected" or "captured" are and how are they related to the rest of the answer.
    – Basilevs
    Commented Jun 25 at 9:22

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