https://kotlinlang.org logo
Title
m

miqbaldc

04/02/2021, 5:50 AM
any suggestion to improves below code?
data class State(title: String, description: String)

fun State.toSessionExpired(): State {
    val state = this
    return with(state) {
        // when token expired
        if (httpCode == 401) {
           copy(
               title = getString(R.string.error_expired_title),
               description = getString(R.string.error_expired_description),
           )
        } else this
   }
}
a

arekolek

04/02/2021, 8:22 AM
Are you trying to get us to solve some recruitment assignment for you by any chance? 😄
I'll bite:
data class State(val title: String, val description: String)

fun State.toSessionExpired(): State {
    return takeUnless { httpCode == 401 }
            ?: State(
                    title = getString(R.string.error_expired_title),
                    description = getString(R.string.error_expired_description),
            )
}
m

miqbaldc

04/02/2021, 4:54 PM
😄 no by any chance, but thanks this is more readable now
did you have any references/book to learning about kotlin conventions outside from official sites & K.E.E.P?
e

elizarov

04/02/2021, 8:48 PM
It would look even better as a single expression function (it now became) without having to use return.
❤️ 3
t

therealbluepandabear

04/06/2021, 1:49 AM
@arekolek that overcomplicates it 😕
🤔 1
m

miqbaldc

04/06/2021, 11:46 AM
any alternative suggestion comes in mind? @therealbluepandabear
t

therealbluepandabear

04/07/2021, 2:36 AM
@miqbaldc your original version is good... It does the job, and it looks concise and easy-to-read. Elvis operators - in my opinion - make things hard to understand in certain scenarios; just my preference though 🙂 You can one hundred percent think I am wrong, it is only my preference.
✍️ 1
a

arekolek

04/07/2021, 6:42 AM
Note there's more that changed than replacing
if
with `takeUnless`: •
val state = this
followed by
with (state)
was just redundant •
copy
with all arguments is just equivalent to calling the constructor, so why not do that, after all you don't need anything from the original, so it's not really a copy
Elvis operators - in my opinion - make things hard to understand in certain scenarios
that's probably true, I'm surprised you think this is one of those scenarios though
m

miqbaldc

04/07/2021, 8:36 AM
to give more context, currently our code implementation was far complex than the above snippet 🙏 and yes, the copy actually needed, and here’s our final results instead:
data class OnScreenState(
    @DrawableRes
    val logo: Int = R.drawable.ic_illustration_astronaut,
    val title: String?,
    val description: String?,
    val buttonOkText: String?,
    val buttonNoText: String?,
)

private fun OnScreenState.withDefault(context: Context): OnScreenState =
    with(this) {
        takeUnless { it.description == TOKEN_EXPIRED }
            ?: copy(
                title = context.getString(R.string.error_expired_title),
                description = context.getString(R.string.error_expired_description),
            )
    }.copy(buttonOkText = buttonOkText ?: context.getString(R.string.all_ok_understand))
a

arekolek

04/07/2021, 9:03 AM
In such case I'd go in a different direction:
private fun OnScreenState.withDefault(context: Context): OnScreenState =
    copy(
        title = if (description != TOKEN_EXPIRED) title else context.getString(R.string.error_expired_title),
        description = if (description != TOKEN_EXPIRED) description else context.getString(R.string.error_expired_description),
        buttonOkText = buttonOkText ?: context.getString(R.string.all_ok_understand),
    )
And I'd say @therealbluepandabear has a point then, because I'd prefer
if-else
if you needed the two
copy
calls, so you can avoid the
with(this)
(which is used instead of parentheses I suppose):
private fun OnScreenState.withDefault(context: Context): OnScreenState =
    if (it.description == TOKEN_EXPIRED) {
        copy(
            title = context.getString(R.string.error_expired_title),
            description = context.getString(R.string.error_expired_description),
        )
    } else {
        this
    }.copy(buttonOkText = buttonOkText ?: context.getString(R.string.all_ok_understand))
✍️ 1
m

miqbaldc

04/08/2021, 4:38 AM
private fun OnScreenState.withDefault(context: Context): OnScreenState =
copy(
title = if (description != TOKEN_EXPIRED) title else context.getString(R.string.error_expired_title),
description = if (description != TOKEN_EXPIRED) description else context.getString(R.string.error_expired_description),
buttonOkText = buttonOkText ?: context.getString(R.string.all_ok_understand),
)
This is more readable than our approach, thanks @arekolek @therealbluepandabear :kotlin-intensifies: IDK why previously we go to such length over-complicated things that should be a simple
if-else
😞
t

therealbluepandabear

04/08/2021, 4:43 AM
@miqbaldc Ha ha, you should have a look at the trailing-comma debate 😆
😂 1