Hi, this piece of code works fine but it looks odd...

# getting-started

[Nat Strangerweather]

Hi, this piece of code works fine but it looks odd to me (all the exclamation marks!)... I suspect I am not making the best use of the Kotlin language. Could you please give me your opinion? If it can be done better, what would I need to work on?

val skillRating = MutableLiveData(0)
    fun calculateSkillRating() {
        if (readWinCount.value != 0 && readLossCount.value != 0) {
            skillRating.value = averageWinningScore.value?.plus(
                (400 * (readWinCount.value?.minus(readLossCount.value!!)!!) /
                        (readWinCount.value?.plus(readLossCount.value!!)!!))
            )
        }
    }

[Jurriaan Mous]

If variables are mutable the type system needs castings to make sure they are not null. Better to put some values into variables so the type system can be sure they do not change during use. Also the elvis ?: operator can be useful to set some values to 0 if they are null

val skillRating = MutableLiveData(0)
    fun calculateSkillRating() {
        val readWinCountValue = readWinCount.value ?: 0
        val readLossCountValue = readLossCount.value ?: 0
        val averageWinningScoreValue = averageWinningScore.value ?: 0        
        if (readWinCountValue != 0 && readLossCountValue != 0) {
            skillRating.value = averageWinningScoreValue + 400 * (readWinCountValue - readLossCountValue) /
                        (readWinCountValue + readLossCount)
            
        }
    }

[Nat Strangerweather]

Thanks, this does look nicer! 😊

[Stephan Schroeder]

the explamation marks are the "trust me, this isn't null operator". I think "the code is running fine" means the values aren't null, because otherwise you'd get NullPointerExceptions.
My solution would be:

fun calculateSkillRating() {
        if (readWinCount.value != 0 && readLossCount.value != 0) {
          val avarageWinningScoreValue = checkNotNull(averageWinningScore.value)
          val readWinCountValue = checkNotNull(readWinCount.value)
          val readLossCountValue = checkNotNull(readLossCount.value)
            skillRating.value = averageWinningScoreValue + (400 * (readWinCountValue -readLossCountValue) /
                        (readWinCountValue + readLossCountValue)
            )
        }
    }

[Stephan Schroeder]

So @Jurriaan Mous chose to select a default value of 0 in case the variable is null while my code throws an IllegalStateException, the original code threw a NullPointerException if one of the properties was null which I assume wasn't handled anywhere, so technically my code is closer to the original implementation.
You have to decide if using 0 instead is ok in the context of the code.

[Nat Strangerweather]

I think I handled the possibility of a NullPointerException with this in the main code:

val skillRating by statsViewModel.skillRating.observeAsState(initial = 0)

But yes, 0 is the value I would want to start with.

[davec]

I'll add that in general you pretty much never want to use !! in Kotlin, that's basically telling the compiler you know better than it does, and you're introducing the possibility of a NullPointerException. That's Java thinking! Instead, you should check if something is null before using it using any one of a number of mechanisms that Kotlin provides (if, when, elvis operator, let, etc.)

[davec]

The other thing to consider is whether the values you're using really need to be nullable in the first place. In Java, nulls are everywhere, but in a lot of cases in Kotlin you don't need nulls at all.

[Nat Strangerweather]

Thanks for these interesting points. I have tons of !! in my code and often get nullpointers. I think I need to read up on nullables. 😅

[davec]

One other suggestion: perhaps a better functional approach would be to have the function be a pure function (https://en.wikipedia.org/wiki/Pure_function) meaning calculateSkillRating should have input parameters and return a value rather than having side effects:

fun calculateSkillRating(readWinCountValue: Int, readLossCountValue: Int, averageWinningScore: Int): Double =
    averageWinningScore + (400 * (readWinCountValue-readLossCountValue) /
                        (readWinCountValue + readLossCountValue))

(The code may not be exactly correct, I'm not sure of the types, but you get this idea)

Here the purpose of the function is clear, easy to unit test, and gets rid of all the noisy error cases. You might also have a special case for when the win/loss count are zero, but I'd argue that none of those input parameters should be nullable, and that the function doesn't mutate state skillRating.value = ... but instead just does a simple calculation and you do the assignment elsewhere.

Disclaimer though: I don't have your full source code, and this is sort of just a sketch of what I'd recommend.