Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate EventsBuilder to Kotlin #100

Merged
merged 8 commits into from
Jan 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ class FunctionalTests {

// when
startTimestamp = System.currentTimeMillis().milliseconds
parselyTracker.trackPageview("url", null, null, null)
parselyTracker.startEngagement(engagementUrl, null)
}

Expand Down

This file was deleted.

63 changes: 63 additions & 0 deletions parsely/src/main/java/com/parsely/parselyandroid/EventsBuilder.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package com.parsely.parselyandroid

import com.parsely.parselyandroid.Logging.log
import java.util.Calendar
import java.util.TimeZone

internal class EventsBuilder(
private val deviceInfoRepository: DeviceInfoRepository,
private val siteId: String,
private val clock: Clock,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly going to be a nitpick, but I guess it affects the architecture, so I'll leave the decision to you whether it's worth addressing:


Taking Clock as a parameter in the constructor doesn't feel correct to me. If we are going to take Clock as a parameter, I think buildEvent is the better place for it, because that's where it's used. However, if we are going to do that, we might as well take now as a parameter and not deal with the Clock ourselves.

If we are going to deal with the Clock internally - and not involve the caller, then I think we can just initiate it ourselves and be the owner of it.


I am generally in favor of taking whatever is necessary at the point of its actual usage, which in this case would be taking now as a parameter. However, ParselyTracker doesn't own a Clock, so there is some practical benefit to owning it ourselves - unless there are unit testing considerations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just reviewed the unit test changes for this as well and I think taking now as a parameter simplifies that as well. I am curious to hear if you had any reservations to use that approach as opposed to taking it as a constructor argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, there are two solutions: either Clock in the constructor or now in the buildEvent. I'm okay with the second as well, but below I share my rationale behind Clock in the constructor.

I am curious to hear if you had any reservations to use that approach as opposed to taking it as a constructor argument.

I've put Clock mentally in the same category as DeviceInfoRepository. We don't execute deviceInfoRepository.collectDeviceInfo() outside EventsBuilder and pass the result in buildEvent in the case of this class, and I treated the Clock the same way.

Also putting Clock as a constructor parameter is that in this design, the code communicates that EventsBuilder needs a Clock for building an event, but it doesn't communicate externally why exactly. I think that's good, as it creates less overhead for the client of taking care of Clock at the time of invoking buildEvent.

WDYT such idea of thinking about Clock in the scope of EventsBuilder?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put Clock mentally in the same category as DeviceInfoRepository. We don't execute deviceInfoRepository.collectDeviceInfo() outside EventsBuilder and pass the result in buildEvent in the case of this class, and I treated the Clock the same way.

It's interesting that you mention this, because I actually drafted something about this in the review and then discarded it. I'd have done this outside of the EventsBuilder as well and passed in the data.

Also putting Clock as a constructor parameter is that in this design, the code communicates that EventsBuilder needs a Clock for building an event, but it doesn't communicate externally why exactly. I think that's good, as it creates less overhead for the client of taking care of Clock at the time of invoking buildEvent.

I see this as complete opposite. If possible, I want to know why you need a clock when you are building the event. So for example, if this was in an SDK and I needed to directly use it, as the client I'd ask, why do you need me to pass you a clock? If it's for the event time, why not ask for it. In fact, even at the cost of a bit extra memory, in my opinion the most ergonomic design would be something like:

fun buildEvent(
    url: String,
    urlRef: String,
    action: String,
    metadata: ParselyMetadata?,
    extraData: Map<String, Any>?,
    uuid: String,
    eventTime: Long = Clock().now.inWholeMilliseconds
): Map<String, Any> {

Anyhow - I don't think this is a particularly important one to pick one solution or the other. So, I am happy with whatever you decide to go with :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this as complete opposite. If possible, I want to know why you need a clock when you are building the event. So for example, if this was in an SDK and I needed to directly use it, as the client I'd ask, why do you need me to pass you a clock?

That's a very interesting perspective! I'd lean more towards asking API: "why do you bother me with passing now or anything that you could figure out before". So that's definitely a refreshing view on the topic to me and food for thoughts 🙂. Thanks for sharing this, I'm glad we exchanged the views!

Anyhow - I don't think this is a particularly important one to pick one solution or the other. So, I am happy with whatever you decide to go with :)

True - it's internal for the API, and we can always iterate without any risk of breaking public contract. I'll default to the currently proposed implementation, just because it doesn't involve more work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very interesting perspective! I'd lean more towards asking API: "why do you bother me with passing now or anything that you could figure out before".

That's a good point, but it's also applicable to why we are taking Clock as a constructor or function argument as well. It's something we can figure out ourselves. I assume the reason we do it is for testing and I have mixed feelings about that. Generally, I don't love having to change the API just for testing, but I understand that it's sometimes a necessary evil.

I also made my suggestion as eventTime instead of taking now as an argument, because I can see a use case for passing a different event time then the current time.

Anyhow, I am totally fine with keeping this as is, just wanted to clarify a few points.

) {
/**
* Create an event Map
*
* @param url The URL identifying the pageview/heartbeat
* @param action Action to use (e.g. pageview, heartbeat, videostart, vheartbeat)
* @param metadata Metadata to attach to the event.
* @param extraData A Map of additional information to send with the event.
* @return A Map object representing the event to be sent to Parse.ly.
*/
fun buildEvent(
url: String,
urlRef: String,
action: String,
metadata: ParselyMetadata?,
extraData: Map<String, Any>?,
uuid: String
): Map<String, Any> {
log("buildEvent called for %s/%s", action, url)

// Main event info
val event: MutableMap<String, Any> = HashMap()
event["url"] = url
event["urlref"] = urlRef
event["idsite"] = siteId
event["action"] = action

// Make a copy of extraData and add some things.
val data: MutableMap<String, Any> = HashMap()
if (extraData != null) {
data.putAll(extraData)
}
val deviceInfo = deviceInfoRepository.collectDeviceInfo()
data["ts"] = clock.now.inWholeMilliseconds
data.putAll(deviceInfo)
event["data"] = data
metadata?.let {
event["metadata"] = it.toMap()
}
if (action == "videostart" || action == "vheartbeat") {
event[VIDEO_START_ID_KEY] = uuid
}
if (action == "pageview" || action == "heartbeat") {
event[PAGE_VIEW_ID_KEY] = uuid
}
return event
}

companion object {
private const val VIDEO_START_ID_KEY = "vsid"
private const val PAGE_VIEW_ID_KEY = "pvid"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package com.parsely.parselyandroid

import java.util.Formatter

object Logging {
internal object Logging {

/**
* Log a message to the console.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ open class ParselyMetadata
*
* @return a Map object representing the metadata.
*/
open fun toMap(): Map<String, Any?>? {
open fun toMap(): Map<String, Any?> {
val output: MutableMap<String, Any?> = HashMap()
if (authors != null) {
output["authors"] = authors
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,12 @@ public class ParselyTracker {
*/
protected ParselyTracker(String siteId, int flushInterval, Context c) {
Context context = c.getApplicationContext();
clock = new Clock();
eventsBuilder = new EventsBuilder(
new AndroidDeviceInfoRepository(
new AdvertisementIdProvider(context, ParselyCoroutineScopeKt.getSdkScope()),
new AndroidIdProvider(context)
), siteId);
), siteId, clock);
LocalStorageRepository localStorageRepository = new LocalStorageRepository(context);
flushManager = new ParselyFlushManager(new Function0<Unit>() {
@Override
Expand All @@ -88,7 +89,6 @@ public Unit invoke() {
return Unit.INSTANCE;
});
flushQueue = new FlushQueue(flushManager, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy"), ParselyCoroutineScopeKt.getSdkScope(), new AndroidConnectivityStatusProvider(context));
clock = new Clock();
intervalCalculator = new HeartbeatIntervalCalculator(clock);

// get the adkey straight away on instantiation
Expand Down Expand Up @@ -266,6 +266,10 @@ public void startEngagement(
log("url cannot be empty");
return;
}
if (lastPageviewUuid == null) {
log("engagement session cannot start without calling trackPageview first");
return;
}

// Blank urlref is better than null
if (urlRef == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ class ParselyVideoMetadata
*
* @return a Map object representing the metadata.
*/
override fun toMap(): Map<String, Any?>? {
val output = super.toMap()?.toMutableMap()
output?.put("duration", durationSeconds)
override fun toMap(): Map<String, Any?> {
val output = super.toMap().toMutableMap()
output["duration"] = durationSeconds
return output
}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,22 @@
package com.parsely.parselyandroid

import kotlin.time.Duration
import kotlin.time.Duration.Companion.hours
import org.assertj.core.api.Assertions.assertThat
import org.assertj.core.api.MapAssert
import org.junit.Before
import org.junit.Test

internal class EventsBuilderTest {
private lateinit var sut: EventsBuilder
private val clock = FakeClock()

@Before
fun setUp() {
sut = EventsBuilder(
FakeDeviceInfoRepository(),
TEST_SITE_ID,
clock
)
}

Expand Down Expand Up @@ -187,20 +191,22 @@ internal class EventsBuilderTest {
assertThat(it)
.hasSize(2)
.containsAllEntriesOf(FAKE_DEVICE_INFO)
.hasEntrySatisfying("ts") { timestamp ->
assertThat(timestamp as Long).isBetween(1111111111111, 9999999999999)
}
}

companion object {
const val TEST_SITE_ID = "Example"
const val TEST_URL = "http://example.com/some-old/article.html"
const val TEST_UUID = "123e4567-e89b-12d3-a456-426614174000"

val FAKE_DEVICE_INFO = mapOf("device" to "info")
val FAKE_NOW = 15.hours
val FAKE_DEVICE_INFO = mapOf("device" to "info", "ts" to FAKE_NOW.inWholeMilliseconds.toString())
}

class FakeDeviceInfoRepository: DeviceInfoRepository {
override fun collectDeviceInfo(): Map<String, String> = FAKE_DEVICE_INFO
}

class FakeClock() : Clock() {
override val now: Duration = FAKE_NOW
}
}