Add SQLDelight #22

tonilopezmr · 2019-10-11T17:19:16Z

🎩 What is the goal?

Add SQLDelight on Kotlin Multiplatform on Android and iOS
Add TTL Cache to store data on SQLite

📝 How is it being implemented?

Create a TTL small cache
Add preferences on Multiplatform using SharedPreferences for Android and NSUserDefaults for iOS to save time for cache.
Add SQLDelight Gradle plugin

Android:

Create Android driver and share it with the injector to initialize dependencies.

val androidSqliteDriver = AndroidSqliteDriver(GalleryDb.Schema, this, "gallerydb")

GalleryInjector().init(androidSqliteDriver, ...)

IOS:

We need to create the iOS SQLite driver on Kotlin in the iosMain module on common lib, which will create this method only accessible by swift or ios module in common lib.

@Suppress("unused")
fun defaultDriver(): SqlDriver = NativeSqliteDriver(GalleryDb.Schema, "gallerydb")

Initialize the injector with the iOS driver like we made it on Android.

GalleryInjector().invoke().doInit(driver: IosSqliteKt.defaultDriver(), ... )

💥 How can it be tested?

On it, this is a draft

Serchinastico

Still a draft and most comments are about naming stuff, feature-wise this is awesome Toni 💯

Serchinastico · 2019-10-11T17:37:12Z

shared/src/androidMain/kotlin/com/karumi/gallery/domain/TimeProvider.kt

+import android.content.SharedPreferences
+import java.util.Calendar
+
+actual class TimeProvider(


I find it really odd that a class called TimeProvider stores anything. Provider usually means that its gets you something. I'd separate this class into two, TimeProvider having a getCurrentTime and timeAgo methods and a TimeStorage that can persist times.

Serchinastico · 2019-10-11T17:41:13Z

shared/src/androidMain/kotlin/com/karumi/gallery/domain/TimeProvider.kt

+  }
+
+  actual fun persistTime() {
+    sharedPreferences.edit().apply {


Using apply twice in the same method with such a different meaning hurts readability IMO. I'd use an preferences variable even though it's like ~9 more characters 😝

If the problem is duplicate names and readability, I'd use run instead apply 👍

Serchinastico · 2019-10-11T17:42:13Z

shared/src/androidMain/kotlin/com/karumi/gallery/domain/TimeProvider.kt

+
+  private fun timeAgo(): Long {
+    val cal = Calendar.getInstance()
+    cal.set(1969, Calendar.JULY, 21)


Serchinastico · 2019-10-11T17:44:08Z

shared/src/commonMain/kotlin/com/karumi/gallery/data/local/LocalPhotosDataSource.kt

+  }
+
+  private fun insert(photos: PhotoShot) {
+    photosQueries.transaction {


Do we need a transaction for a single insertion?

Same for the deletion below

the number of insertions isn't related to the transaction right?, I mean if some other thread, try to insert in the same time, thanks to use transaction will be done in the right way if I am not wrong, it's not about things you are doing inside of a transaction is related to the possible calls from outside.

Serchinastico · 2019-10-11T17:49:07Z

shared/src/commonMain/kotlin/com/karumi/gallery/domain/TimeProvider.kt

+expect class TimeProvider {
+
+  fun persistTime()
+  fun getPersistedTime(): Long


When talking about time I'd either use stronger typing like Duration or specify the unit in the name: getPersistedTimeInMs

I thought I couldn't use Duration in kotlin native 🤔, but I can use it like getCurrentTime().toDuration(DurationUnit.MILLISECONDS)

Also instead to use getPersistedTimeInMs it can be renamed to getPersistedTimestamp() because It will be a Long, except if you are working on python or swift xD

Finally, I changed it with getPersistedTimeInMs

Serchinastico · 2019-10-11T17:51:12Z

shared/src/commonMain/kotlin/com/karumi/gallery/data/local/LocalPhotosDataSource.kt

+class LocalPhotosDataSource(private val cache: TTLCache) {
+
+  fun insert(photos: Photos) {
+    photos.map(::insert)


Not a map but a forEach

Suggested change

photos.map(::insert)

photos.forEach(::insert)

Serchinastico · 2019-10-11T17:51:47Z

shared/src/commonMain/kotlin/com/karumi/gallery/domain/PhotosFlow.kt

+  private suspend fun getPhotos(): Photos =
+    if (localPhotosDataSource.isExpired()) {
+      logInfo(TAG, "Getting from internet")
+      photosApiClient.getPhotos().also { photos ->
+        localPhotosDataSource.removeAll()
+        localPhotosDataSource.insert(photos)
+      }
+    } else {
+      logInfo(TAG, "Getting from local database")
+      localPhotosDataSource.getPhotos()
+    }


Serchinastico · 2019-10-11T17:54:12Z

shared/src/iosMain/kotlin/com/karumi/gallery/domain/TimeProvider.kt

+
+  companion object {
+    private const val TIME_ARG = "time_arg"
+    private const val THREE_MONTHS_IN_MILLIS = 259200


Suggested change

private const val THREE_MONTHS_IN_MILLIS = 259200

private val THREE_MONTHS_IN_MILLIS = 90.days.toLongMilliseconds()

Now that the specific time is encoded into the value we can rename the variable to its actual meaning like TIME_IN_MS_TO_EXPIRE_DATA or something like that

yes, I think either is fine, three months in millis is the number and time in ms to expire data is the purpose of that number.

But I forgot to use the kotlin.time the approach I used for the TTLCache -.-, good catch 💪

Serchinastico · 2019-10-11T17:56:36Z

shared/src/iosMain/kotlin/com/karumi/gallery/domain/TimeProvider.kt

+    private const val THREE_MONTHS_IN_MILLIS = 259200
+  }
+
+  private val delegate: NSUserDefaults = NSUserDefaults.standardUserDefaults


Where does the delegate name comes from? In iOS a delegate is just a listener and it doesn't look like we are using that pattern in here. What about just calling it userDefaults?

I don't know too much in iOS and I guess it was something that it's used in iOS development, I'll change it! 👍

Serchinastico · 2019-10-11T17:56:52Z

shared/src/iosMain/kotlin/com/karumi/gallery/domain/TimeProvider.kt

+    val timeVal = alloc<timeval>()
+    gettimeofday(timeVal.ptr, null)
+    val sec = timeVal.tv_sec
+    val usec = timeVal.tv_usec
+    ((sec * 1_000L) + (usec / 1_000L))


I spent too much time to find how to get a long timestamp on swift, it returns Double -.- with NSDate.timeIntervalSince1970

tonilopezmr added 2 commits October 11, 2019 18:01

Add sqldelight on android

53a234b

Working on IOS

450d129

tonilopezmr requested a review from a team October 11, 2019 17:21

Serchinastico suggested changes Oct 11, 2019

View reviewed changes

tonilopezmr added 10 commits November 20, 2019 15:45

Fix android adapter

464227d

Split TimeProvider in TimeStorage

68f4c86

Use appropriate methods

c91c298

Fix misspelling

8b264f9

Add unit of time in method

af78201

Change variable name

6491921

Change delegate name to userDefaults

c921bb6

Fix tests

7779817

Add tests

70fd007

Fix tests

7576db1

tonilopezmr marked this pull request as ready for review December 5, 2019 13:01

Update ktlint version and plugin to ignore sqldelight files

89a9dc6

Serchinastico approved these changes Dec 5, 2019

View reviewed changes

tonilopezmr merged commit fb6a0ab into master Dec 5, 2019

tonilopezmr deleted the add-sqldelight branch December 5, 2019 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SQLDelight #22

Add SQLDelight #22

tonilopezmr commented Oct 11, 2019 •

edited

Loading

Serchinastico left a comment

Serchinastico Oct 11, 2019

Serchinastico Oct 11, 2019

tonilopezmr Nov 20, 2019

Serchinastico Oct 11, 2019

tonilopezmr Oct 11, 2019

Serchinastico Oct 11, 2019

Serchinastico Oct 11, 2019

tonilopezmr Oct 11, 2019

Serchinastico Oct 11, 2019

tonilopezmr Oct 11, 2019 •

edited

Loading

tonilopezmr Nov 20, 2019

Serchinastico Oct 11, 2019

tonilopezmr Oct 11, 2019

Serchinastico Oct 11, 2019

Serchinastico Oct 11, 2019

Serchinastico Oct 11, 2019

tonilopezmr Oct 11, 2019

Serchinastico Oct 11, 2019

tonilopezmr Oct 11, 2019

Serchinastico Oct 11, 2019

tonilopezmr Oct 11, 2019

	private const val THREE_MONTHS_IN_MILLIS = 259200
	private val THREE_MONTHS_IN_MILLIS = 90.days.toLongMilliseconds()

Add SQLDelight #22

Add SQLDelight #22

Conversation

tonilopezmr commented Oct 11, 2019 • edited Loading

🎩 What is the goal?

📝 How is it being implemented?

💥 How can it be tested?

Serchinastico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonilopezmr Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonilopezmr commented Oct 11, 2019 •

edited

Loading

tonilopezmr Oct 11, 2019 •

edited

Loading