Improve how ParselyTracker is accessed

[wzieba]

Description

This PR is addressing 2 review comments from a different PR: #99 (comment) #99 (comment)

I identify 2 improvement opportunities here:

• confusing SDK initialization and re-initialization problem
• nullability of sharedInstance() getter and confusing usage

This PR should address both matters. I'll put details in comments.

How to test

No need for manual tests.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (704f372) 57.60% compared to head (c45ef7b) 69.30%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #105       +/-   ##
===========================================
+ Coverage   57.60%   69.30%   +11.70%     
===========================================
  Files          19       21        +2     
  Lines         401      404        +3     
  Branches       48       49        +1     
===========================================
+ Hits          231      280       +49     
+ Misses        157      110       -47     
- Partials       13       14        +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[wzieba]

I've decided to propose API change from usages like

ParselyTracker.sharedInstance().trackPageview()

to

ParselyTracker.trackPageview()

I think the second API is more expected (looking at API designs of e.g. Sentry or Crashlytics). To address possible null when calling SDK methods before init method, I've decided to propose "fail fast" approach of throwing our custom exception which clearly points the user to the problem.

[oguzkocer]

I think this approach should work well - but I am not sure why we need to give up on the interface. Shouldn't we keep the previous interface and implement it for this object to make mocking possible?

[wzieba]

I am not sure why we need to give up on the interface

I realized that the interface won't be needed anymore. Looking from the API consumer perspective, I can't find a good justification for it. If it'll be needed in the future for internal usage, we can always revert it. But for now, I would not introduce it to keep things simpler. WDYT?

[oguzkocer]

If the consumer wants to write a unit test, wouldn't an interface be necessary to mock the tracker? I could totally be missing something - as we discussed - I don't do much active Kotlin development and maybe it's possible to mock it as is.

[wzieba]

I'm not sure the interface will be possible with such a design of invoking methods statically. Methods declared in the interface would have to be either @JvmStatic or not.

With @JvmStatic
If we had an interface like in this POC commit: f4a618e then we can't call method as member:

making this still untestable.

Without @JvmStatic
Alternatively, if we remove @JvmStatic, then we can call method as member function, but can't as static method.

I'd like to keep static calls for the convenience of the clients and reduce the overhead of migration, even if it makes this part of the code untestable.

---

The alternative for the clients would be to prepare a wrapper - just like we do in other projects e.g. here.

WDYT? We can always revert to some variant of sharedInstance method, but I'm not sure if it's worth it. E.g. Sentry Java SDK, SDK with some simillarities to this one, doesn't provide an interface to its entrypoint (link).

[wzieba]

Actually, I don't think POC from f4a618e is possible at all: there's declaration clash between the interface and its companion object

[oguzkocer]

Couldn't we have the old design with the interface and have the new static objects on top as a wrapper? The wrapper could simply call the ParselyTracker.sharedInstance() underneath? I didn't give this a try though, so hopefully I am not missing anything.

---

As nice as it's to be able to call ParselyTracker.trackPageview directly, I feel like the benefit here is purely cosmetic. On the other hand, using an interface design has the benefit of making it mockable and unit testable by consumers. So, for me, the interface design is the clear winner.

I think we could do both, as suggested at the top of this comment - but if it was up to me, I'd only go with the interface design and not add the static object at all because having multiple APIs could be confusing.

Having said all that, if you want to go ahead with the static object design, I respect that decision and I can review the PR as is. Also, if you want a third opinion, feel free to ping other developers as well.

---

P.S: I think it's a great idea to look at other 3rd party SDKs, but I personally never liked Sentry's approach. I could totally be the odd man out, so please take that possibility into consideration.

[wzieba]

Gotcha, I've brought back the interface and sharedInstance method in fa7a23e

I agree, the benefit would be only cosmetic and would also cause more rework on client side. I also agree that a mockable interface is a nice thing to have for consumers. I hardly remember an Android SDK providing such, I'm not sure why, though. Or maybe I just missed it.

I'd only go with the interface design and not add the static object at all because having multiple APIs could be confusing.

I'm afraid, lack of static object will cause significant refactoring effort for the consumers because of the need of passing ParselyTracker object. I can imagine this API used in places like e.g. Android Services where it's sometimes not trivial to pass an object directly. Also, I think usually logging SDK I've seen have static APIs: Crashlytics, android.util.Log, Log4j

So, to sum up, with the proposal introduced in: fa7a23e the SDK will have 2 ways of using it:

Via interface
This usage allows for unit testing SomeClass by mocking ParselyTracker interface

fun onCreate(...) {
    ParselyTracker.init(...)
    val tracker = ParselyTracker.sharedInstance()

    val someClass = SomeClass(tracker)
    someClass.openArticle()
}

class SomeClass(val tracker: ParselyTracker) {
    
    fun openArticle() {
        ...
        tracker.startEngagement()
        ...
    }
}

Via static object
This makes SomeClass more difficult to test, but the API easier to use and backward compatible

fun onCreate(...) {
    ParselyTracker.init(...)

    val someClass = SomeClass()
    someClass.openArticle()
}

class SomeClass() {
    
    fun openArticle() {
        ...
        ParselyTracker.sharedInstance().startEngagement()
        ...
    }
}

[oguzkocer]

I assume the Via static object would use ParselyTracker.startEngagement() instead of ParselyTracker.sharedInstance().startEngagement()?

Also, I think it'll be safe to mix and match the usages, could you confirm that?

---

It's not ideal to have 2 different ways to achieve the same thing, but I understand the arguments for it. If you believe this is the way to go, I am good with it.

[wzieba]

I assume the Via static object would use ParselyTracker.startEngagement() instead of ParselyTracker.sharedInstance().startEngagement()?

No, ParselyTracker.startEngagement() will not work anymore (after the recent commit: fa7a23e). To use SDK statically, one would need to use ParselyTracker.sharedInstance().startEngagement() now.

I see that the name of this approach is confusing, sorry. What I meant was "via statically exposed object" and this object is ParselyTrackerInternal (hidden behind ParselyTracker interface).

Also, I think it'll be safe to mix and match the usages, could you confirm that?

It'll be safe to mix usages - the ParselyTracker will make sure that there's only one instance of the ParselyTrackerInternal ever created.

[wzieba]

hi @oguzkocer 👋 Here's my proposal to address comments from the last review. WDYT about them?

[oguzkocer]

@wzieba Is this PR a blocker for you? Can I review it again on Friday?

I am trying to finish something until EOD today, so if this is not a blocker for you, I can pay more attention to the review on Friday.

[wzieba]

@oguzkocer no worries, it's completely fine. Thanks for the heads up!

[oguzkocer]

@wzieba I left one comment, but other than that I think this is OK to merge as is. I am still tiny bit concerned about having multiple ways to use the SDK, but since you confirmed that it's safe to mix and match between them, I can't think of any major practical issues with it.