Discussion: Internal component design

[oguzkocer]

I had my first exposure to this repository while I was reviewing #92. I wanted to discuss some of the design elements related to the internal components we are using, but I didn't want to leave it as a PR comment because if we decided to make any changes, it'd go beyond the scope of the PR. I checked in with @wzieba and we agreed that a separate issue would be a good place to have this discussion.

Here is a summarized version of some of the components as of 409c42d commit of #92:

public class ParselyTracker {
    private final LocalStorageRepository localStorageRepository;
    private final FlushQueue flushQueue;

    protected ParselyTracker(String siteId, int flushInterval, Context c) {
        ...
        localStorageRepository = new LocalStorageRepository(context);
        flushManager = new ParselyFlushManager(new Function0<Unit>() {
            @Override
            public Unit invoke() {
                flushEvents();
                return Unit.INSTANCE;
            }
        },  flushInterval * 1000L, ParselyCoroutineScopeKt.getSdkScope());
        flushQueue = new FlushQueue(flushManager, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy"), ParselyCoroutineScopeKt.getSdkScope());

        flushManager.start();

        ProcessLifecycleOwner.get().getLifecycle().addObserver(
                (LifecycleEventObserver) (lifecycleOwner, event) -> {
                    if (event == Lifecycle.Event.ON_STOP) {
                        flushEvents();
                    }
                }
        );
    }

    void flushEvents() {
        if (!isReachable()) {
            PLog("Network unreachable. Not flushing.");
            return;
        }
        flushQueue.invoke(isDebug);
    }
}

internal class LocalStorageRepository(private val context: Context)

internal class FlushQueue(
    private val flushManager: FlushManager,
    private val repository: QueueRepository,
    private val restClient: RestClient,
    private val scope: CoroutineScope
)

internal class ParselyFlushManager(
    private val onFlush: () -> Unit,
    override val intervalMillis: Long,
    private val coroutineScope: CoroutineScope
)

My primary concern with this setup is that it is not clear what the responsibilities of each component are. For example, there is a a circular relationship between FlushQueue and ParselyFlushManager. This is a bit hard to see, because it's not a direct circular reference and doesn't suffer from some of the typical circular reference issues, but I think it's still something worth avoiding: ParselyFlushManager.onFlush calls ParselyTracker.flushEvents which calls FlushQueue.invoke which has a reference to FlushManager and makes internal updates to it.

---

I am finding it hard to explain my concerns with pure text, so let me share an example setup that addresses them. Note that this setup is definitely not the only one that would work or even the best one. I just want to share something concrete to better explain my points:

internal class FlushQueue(
    private val flushManager: FlushManager,
    private val repository: QueueRepository,
    private val restClient: RestClient,
    private val scope: CoroutineScope
)

I think this class in particular seems to know too much and looking at the implementation it's clearly the primary driver of the logic. It checks the storage state, handles stopping FlushManager, sends network request and updates local storage. On the other hand, ParselyFlushManager doesn't seem to be doing much besides tracking the state and invoking FlushQueue with a delay.

If we have a FlushManager, I think it'd make sense to make its primary responsibility to be what to flush and when. The other components would not flush and instead let FlushManager know about various events. For example, when Lifecycle.Event.ON_STOP occurs, instead of flushing the events directly, we would go through the FlushManager. Similarly, instead of stopping the FlushManager from FlushQueue, we would tell that we are done sending the events and let it decide what to do with it. Then, FlushQueue wouldn't need to know about FlushManager and we'd have the opposite relationship.

With this approach, we'd move QueueRepository to FlushManager, because as part of its job of deciding what to flush, it'd need to check the repository. We'd no longer have QueueRepository in FlushQueue and when we invoke the queue from the manager, we'd tell it which events to flush. We'd also remove RestClient and instead tell it how to send events.

In the final setup, we'd have ParselyTracker to be the owner of LocalStorageRepository, FlushManager and ParselyAPIConnection. FlushManager would have a reference to LocalStorageRepository and be the owner of FlushQueue. FlushQueue wouldn't own anything and instead know how to send events. This should make it so that these components all have clear responsibilities and we have a single owner of each component.

---

@wzieba I am hoping that this will be a good start to our discussion and that I was able to make my points clearly enough. I am looking forward to hear what you think about it!

[wzieba]

Thank you @oguzkocer for this insightful comment 🙇 . I read your suggestions and argumentation - you make good points and the SDK will have likely significantly more readable structure and responsibilities of units will be clearer.

As for this moment, I don't have many comments. The current architecture was mostly inherited, and I didn't want to introduce any unneeded change (and what's “unneeded” might be debatable). But the argument you make with:

If we have a FlushManager, I think it'd make sense to make its primary responsibility to be what to flush and when.

makes a lot of sense. I'd definitely give it a try 👍 .

---

I'd also like to mention current priorities for the project:

1. Finalize the Coroutines migration
2. Establish new public API (ideally, all Kotlin-based)
3. And then I'd prioritize the internal architecture

I just wanted to establish expectation with next steps on this issue - I won't start working on that right away, but after the user-facing work is finished. I hope that's fine!

[oguzkocer]

@wzieba That's totally OK. I appreciate you giving a look at my ideas 🙇‍♂️

I didn't expect us to work on any of this right now. I just wanted to share them immediately as they were fresh in my mind. Happy to continue this discussion whenever.