2
\$\begingroup\$

I have certain error states that I am displaying in my Android TextView but only one message is displayed at a time and if all values are set to 0, no message is displayed. So, I check all the values with if-else and set the associated switch in the chargeCurrentMap. I then clear the error message if all values in the map are 0.

My concern is that my Kotlin code is too long and clunky. How can I improve it?

private var chargeCurrentMap = HashMap<String?, Int?>()
private lateinit var chargeCurrentErrorMessage: String

private fun processChargeModes(data: ByteArray, increment: Int = 0) {
        deviceData.restMode = data[28].toInt()
        if (deviceData.restMode == 1) {
            chargeCurrentErrorMessage = "Battery full, charger is off"
            chargeCurrentMap["rest"] = 1
            binding.textviewChargeCurrentStatus.text = "Off"
            binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
            binding.questionIcon.visibility = View.VISIBLE
        } else {
            chargeCurrentMap["rest"] = 0
        }
        deviceData.misbalancedMode = data[29].toInt()
        if (deviceData.misbalancedMode == 1) {
            chargeCurrentErrorMessage = "Charging is paused, battery is resolving a minor issue"
            chargeCurrentMap["misbalanced"] = 1
            binding.textviewChargeCurrentStatus.text = "Off"
            binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
            binding.questionIcon.visibility = View.VISIBLE
        } else {
            chargeCurrentMap["misbalanced"] = 0
        }
        deviceData.leftChargedMode = data[30].toInt()
        if (deviceData.leftChargedMode == 1) {
            chargeCurrentErrorMessage = "Charger is off, pack left fully charged for too long. Drive golf cart until battery reaches less than 90%"
            chargeCurrentMap["leftCharged"] = 1
            binding.textviewChargeCurrentStatus.text = "Off"
            binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
            binding.questionIcon.visibility = View.VISIBLE
        } else {
            chargeCurrentMap["leftCharged"] = 0
        }
        deviceData.highVoltageMode = data[31].toInt()
        if (deviceData.highVoltageMode == 1) {
            chargeCurrentErrorMessage = "Charger is off, battery voltage is too high"
            chargeCurrentMap["highVoltage"] = 1
            binding.textviewChargeCurrentStatus.text = "Off"
            binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
            binding.questionIcon.visibility = View.VISIBLE
        } else {
            chargeCurrentMap["highVoltage"] = 0
        }
        deviceData.highTempMode = data[32].toInt()
        if (deviceData.highTempMode == 1) {
            chargeCurrentErrorMessage = "Charger is off, battery temperature is too high"
            chargeCurrentMap["highTemp"] = 1
            binding.textviewChargeCurrentStatus.text = "Off"
            binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
            binding.questionIcon.visibility = View.VISIBLE
        } else {
            chargeCurrentMap["highTemp"] = 0
        }
        deviceData.lowTempMode = data[33 + increment].toInt()
        if (deviceData.lowTempMode == 1) {
            chargeCurrentErrorMessage = "Charger is off, battery temperature is too low"
            chargeCurrentMap["lowTemp"] = 1
            binding.textviewChargeCurrentStatus.text = "Off"
            binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
            binding.questionIcon.visibility = View.VISIBLE
        } else {
            chargeCurrentMap["lowTemp"] = 0
        }

        if (increment == 0 && !chargeCurrentMap.containsValue(1)) {
            chargeCurrentErrorMessage = ""
            binding.textviewChargeCurrentStatus.text = "On"
            binding.textviewChargeCurrentStatus.setTextColor(currentTextColor)
            binding.questionIcon.visibility = View.GONE
        } else if (increment == 1) {
            deviceData.hotChargerMode = data[33].toInt()

            if (deviceData.hotChargerMode == 1) {
                chargeCurrentErrorMessage = "Charger resting for a moment to let the battery cool"
                chargeCurrentMap["hotCharger"] = 1
                binding.textviewChargeCurrentStatus.text = "Off"
                binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
                binding.questionIcon.visibility = View.VISIBLE
            } else {
                chargeCurrentMap["hotCharger"] = 0
            }

            if (!chargeCurrentMap.containsValue(1)) {
                chargeCurrentErrorMessage = ""
                binding.textviewChargeCurrentStatus.text = "On"
                binding.textviewChargeCurrentStatus.setTextColor(currentTextColor)
                binding.questionIcon.visibility = View.GONE
            }
        }
    }
\$\endgroup\$
7
  • \$\begingroup\$ What is the difference between deviceData and DeviceData? The two different cases confuses me. \$\endgroup\$
    – Simon Forsberg
    Commented Aug 30, 2022 at 21:01
  • \$\begingroup\$ Can the increment value be anything else than 0 or 1 ? And the same with DeviceData.misbalancedMode and the other values for the if-checks you are checking? \$\endgroup\$
    – Simon Forsberg
    Commented Aug 30, 2022 at 21:04
  • \$\begingroup\$ @SimonForsberg Yeah, sorry. All those Uppercase "D" characters are supposed to be lowercase. I will edit that real quick. Also, increment does have to stay either 1 or 0. There are certain error states that only occur on certain devices so the increment is just for me to differentiate between two possible cases of device types. \$\endgroup\$
    – Dylon Jaynes
    Commented Aug 30, 2022 at 21:14
  • \$\begingroup\$ And the deviceData.leftChargedMode etc, can that be anything besides 1 or 0 ? \$\endgroup\$
    – Simon Forsberg
    Commented Aug 30, 2022 at 21:28
  • \$\begingroup\$ @SimonForsberg It cannot. This is data being sent from firmware code on physical bluetooth transmitting devices. Technically it could be changed to another byte value in the firmware code but that's not really an option right now. \$\endgroup\$
    – Dylon Jaynes
    Commented Aug 30, 2022 at 21:29

1 Answer 1

Reset to default
3
\$\begingroup\$

As a first step, we can move all the deviceData properties (except hotChargerMode) to the top of the method and change them to booleans, as the only thing you care about is if they are 0 or not.

deviceData.restMode = data[28].toInt() != 0
deviceData.misbalancedMode = data[29].toInt() != 0
deviceData.leftChargedMode = data[30].toInt() != 0
deviceData.highVoltageMode = data[31].toInt() != 0
deviceData.highTempMode = data[32].toInt() != 0
deviceData.lowTempMode = data[33 + increment].toInt() != 0

The last else if (increment == 1) { can also be changed into an else { as increment can only be 1 or 0.

Then we can clearly see that we have a lot of the following pattern:

if (deviceData.something) {
    chargeCurrentErrorMessage = "...some message..."
    chargeCurrentMap["something"] = 1
    binding.textviewChargeCurrentStatus.text = "Off"
    binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
    binding.questionIcon.visibility = View.VISIBLE
} else {
    chargeCurrentMap["something"] = 0
}

As we can see, some chargeCurrentMap are directly affected by the deviceData properties, so we can move them to the top as well:

chargeCurrentMap["rest"] = deviceData.restMode
chargeCurrentMap["misbalanced"] = deviceData.misbalancedMode
chargeCurrentMap["leftCharged"] = deviceData.leftChargedMode
chargeCurrentMap["highVoltage"] = deviceData.highVoltageMode
chargeCurrentMap["highTemp"] = deviceData.highTempMode
chargeCurrentMap["lowTemp"] = deviceData.lowTempMode

Then we can see that we're basically doing the same thing if one of the values matches, and we're changing a text based on this, so we can use a when-statement for the text:

val newMessage = when {
    deviceData.restMode -> "Battery full, charger is off"
    deviceData.misbalancedMode -> "Charging is paused, battery is resolving a minor issue"
    deviceData.leftChargedMode -> "Charger is off, pack left fully charged for too long. Drive golf cart until battery reaches less than 90%"
    deviceData.highVoltageMode -> "Charger is off, battery voltage is too high"
    deviceData.highTempMode -> "Charger is off, battery temperature is too high"
    deviceData.lowTempMode -> "Charger is off, battery temperature is too low"
    else -> null
}

Then, if the message has a value, we use it, and set the textviewChargeCurrentStatus and questionIcon accordingly:

if (newMessage != null) {
    chargeCurrentErrorMessage = newMessage
    binding.textviewChargeCurrentStatus.text = "Off"
    binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
    binding.questionIcon.visibility = View.VISIBLE
}

That leaves us with this:

private var chargeCurrentMap = HashMap<String?, Int?>()
private lateinit var chargeCurrentErrorMessage: String

private fun processChargeModes(data: ByteArray, increment: Int = 0) {
    deviceData.restMode = data[28].toInt() != 0
    deviceData.misbalancedMode = data[29].toInt() != 0
    deviceData.leftChargedMode = data[30].toInt() != 0
    deviceData.highVoltageMode = data[31].toInt() != 0
    deviceData.highTempMode = data[32].toInt() != 0
    deviceData.lowTempMode = data[33 + increment].toInt() != 0
    chargeCurrentMap["rest"] = deviceData.restMode
    chargeCurrentMap["misbalanced"] = deviceData.misbalancedMode
    chargeCurrentMap["leftCharged"] = deviceData.leftChargedMode
    chargeCurrentMap["highVoltage"] = deviceData.highVoltageMode
    chargeCurrentMap["highTemp"] = deviceData.highTempMode
    chargeCurrentMap["lowTemp"] = deviceData.lowTempMode

    val newMessage = when {
        deviceData.restMode -> "Battery full, charger is off"
        deviceData.misbalancedMode -> "Charging is paused, battery is resolving a minor issue"
        deviceData.leftChargedMode -> "Charger is off, pack left fully charged for too long. Drive golf cart until battery reaches less than 90%"
        deviceData.highVoltageMode -> "Charger is off, battery voltage is too high"
        deviceData.highTempMode -> "Charger is off, battery temperature is too high"
        deviceData.lowTempMode -> "Charger is off, battery temperature is too low"
        else -> null
    }

    if (newMessage != null) {
        chargeCurrentErrorMessage = newMessage
        binding.textviewChargeCurrentStatus.text = "Off"
        binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
        binding.questionIcon.visibility = View.VISIBLE
    }

    if (increment == 0 && !chargeCurrentMap.containsValue(1)) {
        chargeCurrentErrorMessage = ""
        binding.textviewChargeCurrentStatus.text = "On"
        binding.textviewChargeCurrentStatus.setTextColor(currentTextColor)
        binding.questionIcon.visibility = View.GONE
    } else {
        deviceData.hotChargerMode = data[33].toInt()

        if (deviceData.hotChargerMode == 1) {
            chargeCurrentErrorMessage = "Charger resting for a moment to let the battery cool"
            chargeCurrentMap["hotCharger"] = 1
            binding.textviewChargeCurrentStatus.text = "Off"
            binding.textviewChargeCurrentStatus.setTextColor(Color.RED)
            binding.questionIcon.visibility = View.VISIBLE
        } else {
            chargeCurrentMap["hotCharger"] = 0
        }

        if (!chargeCurrentMap.containsValue(1)) {
            chargeCurrentErrorMessage = ""
            binding.textviewChargeCurrentStatus.text = "On"
            binding.textviewChargeCurrentStatus.setTextColor(currentTextColor)
            binding.questionIcon.visibility = View.GONE
        }
    }
}

Which can still be shortened a bit, but I think that this is a good start.

Note that you might need to re-order the when-statement conditions, as only the first matching one will be performed.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.