https://kotlinlang.org logo
#codereview
Title
# codereview
a

Afzal Najam

08/13/2020, 3:56 AM
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
g

gildor

08/13/2020, 3:59 AM
Copy code
.map {
            it.id to it
        }.toMap()
can be replaced with
Copy code
.associateBy { it.id }
Also it more efficient
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
Obviously second algorithm is more simple to understand, I woul also get rid of this:
Copy code
newLocalNotes[id]?.let { localNote ->
  ...
} ?: remoteNote
instead just do
Copy code
val localNote = newLocalNotes[id] ?: return@map remoteNote\
also would probably remove multiple returns and replace with one single return
Something like this:
Copy code
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
    }
}
a

Afzal Najam

08/13/2020, 4:11 AM
Thanks so much @gildor!! Ooh that when block makes it look even simpler
g

gildor

08/13/2020, 4:15 AM
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
🙌 1
a

Afzal Najam

08/13/2020, 4:19 AM
I was wondering if there was a clean way. Maybe I should even make it a function in the data class
g

gildor

08/13/2020, 4:19 AM
I would just make private function on the same class where you have findUpdatedNotes
it’s not a responsibility of data class imo
a

Afzal Najam

08/13/2020, 4:21 AM
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!
g

gildor

08/13/2020, 4:27 AM
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:
Copy code
return newRemoteNotes.map { remoteNote ->
        val localNote = newLocalNotes[remoteNote.id]
to get id from remote note, so you will save one map
a

Afzal Najam

08/13/2020, 4:31 AM
😮 Nice!
a

asad.awadia

08/17/2020, 7:06 PM
@Afzal Najam can you post the final version - these sync two list type things come up quite often
a

Afzal Najam

08/18/2020, 4:42 AM
@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)
2 Views