https://kotlinlang.org logo
Join Slack
Powered by
```override suspend fun saveToDeviceRegistry(req: ...
# getting-started
a
Copy code
override suspend fun saveToDeviceRegistry(req: SaveToDeviceRegistryReq): Flow<RepositoryState<Unit>> =
    flow<RepositoryState<Unit>> {
        searchDeviceRegistryByCategoryId(req.deviceProfile.categoryId)?.let {
            it.categoryName = req.deviceProfile.categoryName
            it.categoryId = req.deviceProfile.categoryId
            it.vendor = req.deviceProfile.vendor
            it.model = req.deviceProfile.model
            it.mac = req.deviceProfile.address
            deviceRegistryBox.put(it)
            emit(RepositoryState.SuccessState(Unit))
        } ?: kotlin.run {
            deviceRegistryBox.put(
                DeviceRegistryEntity(
                    categoryName = req.deviceProfile.categoryName,
                    categoryId = req.deviceProfile.categoryId,
                    vendor = req.deviceProfile.vendor,
                    model = req.deviceProfile.model,
                    mac = req.deviceProfile.address,
                )
            )
            emit(RepositoryState.SuccessState(Unit))
        }
    }.flowOn(dispatcher)
Is there syntax sugar to write a better code in Kotlin with the above snippet,
👀 1
j
Is this a flow that never contains more than one element?
a
we can ignore the flow , for now, only optimization need is from the search call till the last block in kotlin.run
this routine will always assumed to be successful
j
I would do
Copy code
val registryEntity = searchDeviceRegistryByCategoryId(req.deviceProfile.categoryId)?.let {
            it.categoryName = req.deviceProfile.categoryName
            it.categoryId = req.deviceProfile.categoryId
            it.vendor = req.deviceProfile.vendor
            it.model = req.deviceProfile.model
            it.mac = req.deviceProfile.address
        } ?: 
         DeviceRegistryEntity(
                    categoryName = req.deviceProfile.categoryName,
                    categoryId = req.deviceProfile.categoryId,
                    vendor = req.deviceProfile.vendor,
                    model = req.deviceProfile.model,
                    mac = req.deviceProfile.address,
                )
and then call put on the registryEntity
a
1. This is unnecessarily busy code. I'd suggest separating off the 
it.*
assignments into a different function and do 
deviceRegistryBox.put(separatefn(it))
. 2. You don't need to do 
kotlin.run
. Simply 
run
is enough. 3. As this flow always returns Success, you don't need the last run block. Have the 
emit
as the last statement.
Copy code
searchDeviceRegistryByCategoryId(req.deviceProfile.categoryId).let {
    deviceRegistryBox.put(
        // `updateFields` is the separate function that does all the `it.*` assignments.
        if (it != null) updateFields(it) else DeviceRegistryEntity(…)
    )
}
emit(Success)
DeviceRegistryEntity(…)
could also be a separate fn with 
req
as an input parameter. Would help with unit tests.
d
Additional question whether this update-or-create pattern (which appears quite often) couldn't be made less repetitive. Without compromising on immutability of course.
j
Copy code
val registryEntity = searchDeviceRegistryByCategoryId(req.deviceProfile.categoryId)?.apply {
            updateRegistryEntity(req.deviceProfile)
        } ?: newRegistryEntity(req.deviceProfile)
a
Looks much cleaner and with a more obvious intent
j
This example clearly isn’t working with immutability
d
Sorry, I wanted to say nullability 🙂 The goal is basically to deduplicate the updateRegistryEntity and newRegistryEntity. With nullable default properties you could construct just 
DeviceRegistryEntity
and then use 
updateRegistryEntity
in both cases. That's not ideal and I wonder whether there's another possible solution for the deduplication.
j
Copy code
val registryEntity = searchDeviceRegistryByCategoryId(req.deviceProfile.categoryId).orElseNew().apply{update(req.deviceProfile)}
fun DeviceRegistryEntity?.orElseNew() = this ?: DeviceRegistryEntity()
Copy code
fun DeviceRegistryEntity.update(deviceProfile: DeviceProfile){
...
}
d
Yes, but this assumes that the fields in 
DeviceRegistryEntity
have default (most probably null) values.
y
Evidently, DeviceRegistryEntity has default values for fields that searchDeviceRegistry constructs it with, so if you make it a data class, you can simply do:
Copy code
val entity = DeviceRegistryEntity(...)
deviceRegistryBox.put(searchDeviceRegistryByCategoryId(req.deviceProfile.categoryId)?.mergeWith(entity) ?: entity)
emit(...)

fun searchDeviceRegistryByCategoryId(...): ThoseExtraFields
fun ThoseExtraFields.mergeWith(entity: DeviceRegistryEntity) = ...
The key here is that the search shouldn't be returning a DeviceRegistryEntity if it can never return a valid one. Instead, the search should have its own abstraction. If the search is really tightly coupled, you might instead want to do:
Copy code
val entity = DeviceRegistryEntity(...)
deviceRegistryBox.put(searchDeviceRegistryByCategoryId(entity))
emit(...)

fun searchDeviceRegistryByCategoryId(entity: DeviceRegistryEntity): DeviceRegistryEntity
d
I agree that the particular model in question looks strange and it should probably be redesigned using one of your suggested methods. As I don't know the original use case I can't comment further. I just spotted there the create-or-update pattern which generally is more akin to: • the goal is to ensure an entity with certain data is stored in a DB • either the entity already exists (given some identifier, usually composite) -> then update it with the given data • or it doesn't -> then create a new one with the given data • in either case save the updated/created entity to DB And this pattern leads to the duplication I would like to get rid of. Namely, in the update branch you have
Copy code
dataClassInstance.copy(a = 1, b = 2)
and in the create branch
Copy code
DataClass(a = 1, b = 2)
j
change the create branch to 
DataClass().copy(a = 1, b = 2)
and the duplication is removed
d
That would violate the "Without compromising on nullability" requirement 🙂
Of course, typically the fields are not just a primitive types where you could perhaps use a "natural" default value. So generally any other value than null is not an option.
a
Hi all, thank you for your inputs have a though i came up with this
Copy code
override suspend fun saveToDeviceRegistry(req: SaveToDeviceRegistryReq): Flow<RepositoryState<Unit>> =
    flow<RepositoryState<Unit>> {
        req.deviceProfile.apply {
            deviceRegistryBox.put(searchDeviceRegistryByCategoryId(req.deviceProfile.categoryId)?.let {
                this.toDeviceRegistryEntity(it.id))
            } ?: this.toDeviceRegistryEntity(0))
            emit(RepositoryState.SuccessState(Unit))
        }
    }.flowOn(dispatcher)
Copy code
fun DeviceProfile.toDeviceRegistryEntity(id: Long): DeviceRegistryEntity =
    DeviceRegistryEntity(
        id = id,
        categoryName = categoryName,
        categoryId = categoryId,
        vendor = vendor,
        model = model,
        mac = address
    )
you may have to open up this in a full screen window to read the code. Please let me know if this appears good or not. As ObjectBox db will replace the object with same id, i exploited this behavior to shorten the code.
y
Copy code
override suspend fun saveToDeviceRegistry(req: SaveToDeviceRegistryReq): Flow<RepositoryState<Unit>> = with(req.deviceProfile) {
    flow {
        deviceRegistryBox.put(toDeviceRegistryEntity(searchDeviceRegistryByCategoryId(categoryId)?.id ?: 0))
        emit(RepositoryState.SuccessState(Unit))
    }.flowOn(dispatcher)
}
Has the same logic but is immensely clearer IMO or:
Copy code
override suspend fun saveToDeviceRegistry(req: SaveToDeviceRegistryReq): Flow<RepositoryState<Unit>> = with(req.deviceProfile) {
    flow {
        val id = searchDeviceRegistryByCategoryId(categoryId)?.id ?: 0
        deviceRegistryBox.put(toDeviceRegistryEntity(id))
        emit(RepositoryState.SuccessState(Unit))
    }.flowOn(dispatcher)
}
✅ 1
j
I’d switch that 
apply
to a 
with
. apply changes the returned value to 
this
from the far more typical return type of the block. I wouldn’t use it unless I was leveraging the return type
Also 
searchDeviceRegistryByCategoryId(req.deviceProfile.categoryId)
should be using this.categoryId
a
Copy code
override suspend fun saveToDeviceRegistry(req: SaveToDeviceRegistryReq): Flow<RepositoryState<Unit>> =
    flow<RepositoryState<Unit>> {
        with(req.deviceProfile) {
            deviceRegistryBox.put(searchDeviceRegistryByCategoryId(categoryId)?.let {
                toDeviceRegistryEntity(it.id)
            } ?: toDeviceRegistryEntity(0))
            emit(RepositoryState.SuccessState(Unit))
        }
    }.flowOn(dispatcher)
@Jacob done 🙂
j
And I really agree with @Youssef Shoaib [MOD] s second option
a
thank you @Youssef Shoaib [MOD] will take your second approach
d
It also looks like 
req.deviceProfile
is what you're actually using, so perhaps that should be the parameter instead.
4 Views
Open in Slack
PreviousNext
Powered by