Wrote a list/table syncing function in idiomatic Kotlin and without. I find the non-idiomatic one mo...

# codereview

[Afzal Najam]

Wrote a list/table syncing function in idiomatic Kotlin and without. I find the non-idiomatic one more readable right now. Would love to hear a better way to write it. Both pass my unit tests.

https://gist.github.com/AfzalivE/48a9e225246c5cee144b06695f68731a

[gildor]

.map {
            it.id to it
        }.toMap()

can be replaced with

.associateBy { it.id }

Also it more efficient

[gildor]

also I don’t think that it idiomatic vs non-idiomatic, both are idiomatic enough, you can sligntly improve each of them, second is more efficeint and simple

[gildor]

Obviously second algorithm is more simple to understand, I woul also get rid of this:

newLocalNotes[id]?.let { localNote ->
  ...
} ?: remoteNote

[gildor]

instead just do

val localNote = newLocalNotes[id] ?: return@map remoteNote\

[gildor]

also would probably remove multiple returns and replace with one single return

[gildor]

Something like this:

fun findUpdatedNotes(
        allLocalNotes: List<Note>,
        allRemoteNotes: List<Note>
): List<Note> {
    val newLocalNotes = allLocalNotes.minus(allRemoteNotes).associateBy(Note::id)
    val newRemoteNotes = allRemoteNotes.minus(allLocalNotes).associateBy(Note::id)

    return newRemoteNotes.map { (id, remoteNote) ->
        val localNote = newLocalNotes[id]
        when {
            localNote == null -> remoteNote
            remoteNote.creationId == localNote.creationId -> {
                if (remoteNote.updatedAt.isAfter(localNote.updatedAt)) {
                    remoteNote
                } else {
                    localNote
                }
            }
            else -> remoteNote.copy(id = 0)
        }
    }.sortedByDescending {
        // we add copied notes at the end to avoid auto-incremented ID
        // from being the same as later non-conflicting notes
        it.id
    }
}

[Afzal Najam]

Thanks so much @gildor!!

Ooh that when block makes it look even simpler

[gildor]

It probably also worth to extract this if which checks date to own function, like newestNote(note1, note2)
So function would be even more readable

[Afzal Najam]

I was wondering if there was a clean way. Maybe I should even make it a function in the data class

[gildor]

I would just make private function on the same class where you have findUpdatedNotes

[gildor]

it’s not a responsibility of data class imo

[Afzal Najam]

hmm true, done. Thank you! Looks much simpler and much more readable.
I think you're right, it wasn't about idiomatic, maybe it was more about using the right features of the language/stdlib 🙂

Really appreciate you taking the time out of your day to help!

[gildor]

I also found, that looks that you don’t need create a map from newRemoteNotes, it’s just unnecessary for your case, only difference would be those lines:

return newRemoteNotes.map { remoteNote ->
        val localNote = newLocalNotes[remoteNote.id]

[gildor]

to get id from remote note, so you will save one map

[Afzal Najam]

😮 Nice!

[asad.awadia]

@Afzal Najam can you post the final version - these sync two list type things come up quite often

[Afzal Najam]

@asad.awadia, updated the gist with the final version.

This code only finds the updated ones from the two and doesn't handle any deleted ones. I have separate code that tracks deletions and then excludes the deleted notes from the result of findUpdatedNotes function. Still contending with whether to keep a separate table in room to track deletions or just soft-delete notes. (It's the former at the moment)