5

I am implementing an ITracker interface that looks something like:

public interface ITracker
{
    void Track(ITrackerEvent trackerEvent);
}

I initially created an implementation of this interface wrapping Mixpanel.NET. I then created another one that wraps Application Insights. However, the Application Insights one requires Flush() to send the data to the server.

I don't want to pollute the ITracker interface with a Flush() method just because one of the implementations needs one. It would feel like a leaky abstraction.

However, I need to call this method at some point (probably on application shutdown) and don't want to do so every time Track is called.

Is is possible to call a method when the Tracker is garbage collected at the end of the session? Is this even a good approach?

I feel like I'm missing a trick here!

James Bateson
  • 1,069
  • 13
  • 20
  • 1
    What you should describe in detail is in what cases exactly you would like `Flush` to be called. That's the only important question. My guess is, either "on each call", or "every now and then", and a final flush on dispose. – vgru Mar 09 '16 at 13:29
  • Why are you associating a `Flush()` with garbage collection? I mean, you could call `Flush` repeatedly during a socket session with a remote server and the only thing you're intending there is to send the message across the wire to the other machine... – code4life Mar 09 '16 at 13:44
  • @Groo. To some extent, it doesn't matter when `Flush` has to be called. My concern is that I don't want the fact I have to Flush (whether occasionally or on application shutdown) to leak through into my `ITracker` interface. – James Bateson Mar 09 '16 at 13:55
  • Maybe it is also option for you to implement some kind of buffering inside Track() method of your concrete ITracker implementation, so that it serves itself and checks time since last send or track call count or track data size and decide is it time to Flush – Dzianis Yafimau Apr 05 '16 at 16:32

4 Answers4

5

I'd make ITracker to use transaction alike pattern because there may be cases when you might want to discard tracker events, rollback or modify them:

public interface ITracker
{
    void Track(ITrackerEvent trackerEvent);

    void Commit();
}

And then per implementations:

  • I'd call Flush inside Commit for Application insights.
  • I'd write tracker events in in-memory collection (List<ITrackerEvent> or BlockingCollection<ITrackerEvent> if concurrency involved) inside Track method and then use your current logic to call Mixpanel.NET api inside Commit method implementation for Mixpanel.NET.

Recommendation: ITracker should also implement IDisposable since trackers usually use resources that need to be disposed.

Leri
  • 12,367
  • 7
  • 43
  • 60
  • 2
    This is clearly a case where `IDisposable` will keep the contracts clear. – code4life Mar 09 '16 at 13:45
  • Thanks for your answer. This will work and isn't a bad solution, but I was hoping that there might be another approach that would mean I could avoid changing my interface. On the other hand, perhaps my interface is simply incorrect and needs to change to properly reflect the abstraction at hand. – James Bateson Mar 09 '16 at 14:00
  • @Gibbonfiend You can't really think of a way without changing your interface, since your interface was violating srp from the very beginning, imho. – Leri Mar 09 '16 at 14:04
5

Building on Leri, I would think more along the lines of what a tracker might need to do.

I would be inclined to do something like this:

public interface ITracker {
    void BeginTracking();
    void Track(ITrackerEvent trackerEvent);
    void EndTracking();
}

Then all trackers have a sense of when they're starting and when they're finishing. This is important because a tracker may be holding onto resources that shouldn't be held longer than necessary. If a tracker doesn't need to use either BeginTracking or EndTracking, the implementation is trivial. A trivial implementation is not a leaky abstraction. A leaky abstraction is one that doesn't work for all implementations.

Now suppose you are flat-out against having two extra methods in each tracker (why?). You could instead have ITrackerEvents that are out of band and cover the semantic meaning of Begin and End. I don't like this. It requires every tracker to have special code to handle out of band events.

You can also have a separate interface

public interface IDemarcatedTracker : ITracker {
    void BeginTracking();
    void EndTracking();
}

which requires you to have special case code in your calling code to check to see if the ITracker is also an IDemarcatedTracker:

public void BeginTracking(ITracker tracker)
{
    IDemarcatedTracker demarcatedTracker = tracker as IDemarcatedTracker;
    if (demarcatedTracker != null)
        demarcatedTracker.BeginTracking();
}

And not to over blow things too much, but I would also wonder what is supposed to happen when a tracker fails? Just blindly throw an exception? And here is where the abstraction is actually leaky. There is no process for a tracker to let you know that it can't track.

In your case you might want to return a boolean (limited information), an error code (somewhat more information), or an error class/struct. Or you might want to have a standard exception that gets thrown. Or you might want the Begin() method to include a delegate to call when an error happens in tracking.

plinth
  • 48,267
  • 11
  • 78
  • 120
  • +1 nice answer. However, this opens a room for another layer (`ITrackerManager`) which should decide when to begin/end tracking and which trackers to use. That might be going a bit beyond OP's requirements. – Leri Mar 09 '16 at 14:08
  • Thanks for your detailed answer plinth. I think you and @Leri are correct that I simply need to modify my abstraction. I particularly like your explanation of "leakyness": **A trivial implementation is not a leaky abstraction. A leaky abstraction is one that doesn't work for all implementations.** – James Bateson Mar 09 '16 at 14:14
  • A problem with this approach is that it introduces [temporal coupling](http://blog.ploeh.dk/2011/05/24/DesignSmellTemporalCoupling/). `BeginTracking` must be called before `Track`, which must be called before `EndTracking`. The interface has no way to enforce the way in which it is used. I've opened up a separate question for this: http://stackoverflow.com/questions/35895097/how-to-enforce-how-an-interface-is-consumed – James Bateson Mar 10 '16 at 09:43
  • The temporal coupling here is obvious and because the interface is simple, you would deserve a dope slap for screwing it up. If you're doing something more complicated (for example, an OCR engine), it helps to burn the process into exactly one place (maybe abstract base class). – plinth Mar 10 '16 at 13:05
3

I'd just derive ITracker from IDisposable. Classes that implement this interface may choose to perform some action in the Dispose method, e.g. your Flush, or do nothing.

public interface ITracker : IDisposable
{
    void Track(ITrackerEvent trackerEvent);
}

Also, have a look at the Observable pattern, the ITracker name suggests that you may want to perform some action when an object's state is changed.

Community
  • 1
  • 1
oleksii
  • 35,458
  • 16
  • 93
  • 163
  • 1
    Doesn't this force implementations that have nothing to dispose to implement a `Dispose()` method? I.e. your, inherently abstract, interface is making concrete decisions. – David Osborne Mar 10 '16 at 12:23
  • Look at IObservable an IObserver interfaces, they might fit well. – Beachwalker Feb 15 '18 at 22:10
1

It sounds like you're buffering something - the things that get tracked need to get flushed from time to time. Your interface hides that behavior, which is good - you shouldn't be able to tell from that interface when it's getting flushed or even if it's getting buffered.

If it's high-volume I like to set two parameters - a maximum flush interval and a maximum buffer size. The first uses a timer to flush at regular intervals. The second triggers a flush when the capacity is reached. And then I flush again when the object is being disposed or collected.

I separated out the buffer into its own class so I could reuse it and unit test it independently. I'll see if I can find it, but it's not much code to write.

Scott Hannen
  • 27,588
  • 3
  • 45
  • 62
  • 1
    Too much work for a single class. That violates SRP. Your described flow is correct however in that case you need `ITrackerManager` which takes care of when data should be flushed. – Leri Mar 10 '16 at 06:18
  • I think your first paragraph highlights the core of this question. A consumer of the interface shouldn't have to worry about flushing or deciding when to flush; this should be the job of the implementation. – James Bateson Mar 10 '16 at 10:51