any suggestion to improves below code? ```data cl...

# codingconventions

[miqbaldc]

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
   }
}

[arekolek]

Are you trying to get us to solve some recruitment assignment for you by any chance? 😄

[arekolek]

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),
            )
}

[miqbaldc]

😄 no by any chance, but thanks this is more readable now

[miqbaldc]

did you have any references/book to learning about kotlin conventions outside from official sites & K.E.E.P?

[elizarov]

It would look even better as a single expression function (it now became) without having to use return.

[therealbluepandabear]

@arekolek that overcomplicates it 😕

[miqbaldc]

any alternative suggestion comes in mind? @therealbluepandabear

[therealbluepandabear]

@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.

[arekolek]

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

[miqbaldc]

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))

[arekolek]

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))

[miqbaldc]

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 K IDK why previously we go to such length over-complicated things that should be a simple if-else 😞

[therealbluepandabear]

@miqbaldc Ha ha, you should have a look at the trailing-comma debate 😆