Use the private setter pattern on SessionGenerator.currentSession #4785
Conversation
| sessionIndex, | ||
| ) | ||
| var currentSession = | ||
| SessionDetails(sessionId = "", firstSessionId = "", collectEvents, sessionIndex = -1) |
There was a problem hiding this comment.
Do we still need this default instance? Would it make sense to make currentSession nullable or lateinit?
There was a problem hiding this comment.
It mostly depends on preference. Do you want to deal with nulls in the classes that use this, or assume that what you get is valid?
I think we've designed this class to always return a valid currentSession, so I lean towards keeping it not nullable.
There was a problem hiding this comment.
I don't think we need a default instance. That does not serve anything. SessionGenerator should generate session only when there is a need.
There was a problem hiding this comment.
Ok I changed it to lateinit not nullable. This will let us avoid the null check, but we have to guarantee we call generateNewSession() before accessing it.
| } | ||
|
|
||
| val currentSession: SessionDetails | ||
| get() = thisSession | ||
| private fun generateSessionId() = UUID.randomUUID().toString().replace("-", "").lowercase() |
There was a problem hiding this comment.
It might not be worth it, but if we made this an interface like SessionIdGenerator and passed in, we could make the unit tests deterministic. Then have a seperate test to validate the generated session ids. What do you think?
There was a problem hiding this comment.
I like that idea. 2 suggestions:
- You can pass in a function to the constructor instead of making a SessionIdGenerator class with a generate() method. Only reason is a function might be lighter weight.
- This class / function that we pass in should only return the
UUID.randomUUID()part of this statement. ThisSessionGeneratorshould be responsible for toString and.replaceing it so we can assert that the dashes are removed in tests.
There was a problem hiding this comment.
A separate interface just for sessionId sounds like an overkill. Do you think this structure is not testable? I think with the SessionDetails structure and generateNewSession, we should be able to cover all scenarios for testing.
There was a problem hiding this comment.
I was concerned about the randomness in the tests, and asserts on individual fields of the SessionDetails data class to avoid a big assert on the entire object since it contained randomness.
I played around with this. I made it take a function, not an interface, in the ctor. I left one test with the random UUIDs to verify it generates valid session ids.
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected Products
Test Logs |
firebase-sessions/src/main/kotlin/com/google/firebase/sessions/SessionGenerator.kt
Outdated
Show resolved
Hide resolved
firebase-sessions/src/main/kotlin/com/google/firebase/sessions/SessionGenerator.kt
Show resolved
Hide resolved
| sessionIndex, | ||
| ) | ||
| var currentSession = | ||
| SessionDetails(sessionId = "", firstSessionId = "", collectEvents, sessionIndex = -1) |
There was a problem hiding this comment.
I don't think we need a default instance. That does not serve anything. SessionGenerator should generate session only when there is a need.
| ) | ||
| var currentSession = | ||
| SessionDetails(sessionId = "", firstSessionId = "", collectEvents, sessionIndex = -1) | ||
| private set |
There was a problem hiding this comment.
Is this not associated with any var/val? Felt like a hanging private set.
There was a problem hiding this comment.
This is the setter for currentSession, I think it looks more clear now that we removed the default instance.
| } | ||
|
|
||
| val currentSession: SessionDetails | ||
| get() = thisSession | ||
| private fun generateSessionId() = UUID.randomUUID().toString().replace("-", "").lowercase() |
There was a problem hiding this comment.
A separate interface just for sessionId sounds like an overkill. Do you think this structure is not testable? I think with the SessionDetails structure and generateNewSession, we should be able to cover all scenarios for testing.
This is more Kotlin idiomatic. Also made some properties val instead of var, I think this is cleaner.