I learned to program by myself and just decided to go with Kotlin.
I made a simple program that solves a triangle which works well but my code is ugly and repetitive.
Can you help me on how to improve it, any suggestion will help since even my code works it does not look correct.
private fun validateNumbers() {
if(binding.etHip.text.toString().isNotEmpty()){
Hip = (binding.etHip.text.toString()).toFloat()
}
if(binding.etAdj.text.toString().isNotEmpty()){
Adj = (binding.etAdj.text.toString()).toFloat()
}
if(binding.etOpp.text.toString().isNotEmpty()){
Opp = (binding.etOpp.text.toString()).toFloat()
}
if(binding.etAngle.text.toString().isNotEmpty()){
Angle = (binding.etAngle.text.toString()).toFloat()
}
}
private fun countSelected() {
binding.cbHyp.setOnCheckedChangeListener { compoundButton, b ->
if(binding.cbHyp.isChecked){
numberCB++
}
else{
numberCB--
}
}
binding.cbAdj.setOnCheckedChangeListener { compoundButton, b ->
if(binding.cbAdj.isChecked){
numberCB++
}
else{
numberCB--
}
}
binding.cbOpp.setOnCheckedChangeListener { compoundButton, b ->
if(binding.cbOpp.isChecked){
numberCB++
}
else{
numberCB--
}
}
binding.cbAngle.setOnCheckedChangeListener { compoundButton, b ->
if(binding.cbAngle.isChecked){
numberCB++
}
else{
numberCB--
}
}
}
2
Answers
How about this? The logic is the same. it’s only simplified syntactically.
For both functions, you can wrap them in
with(binding)
so you don’t have to keep writingbinding.
For
validateNumbers
, you can usetoFloatOrNull()
to convert values to Float, or null if it doesn’t evaluate as a Float (like an empty string would be invalid). Then you can use the Elvis operator?:
to provide a default. So if you don’t want the value to change, it can return itself.Also, since you’re doing it repeatedly, you can make a shortcut for getting the value from an EditText. which is a type of TextView, so you can write it as an extension of TextView.
With
countSelected()
since they’re all doing exactly the same thing in their listeners, you can give them all the same listener. The second parameter in the lambda it whether it was just checked, so you can simplify that logic a bit too. You can uselistOf(...).forEach
to get them all.I would also change the function name to something more descriptive. Your function isn’t counting something…it’s setting up the checkboxes to count themselves.
Note, property and variable names should start with lowercase letters by convention. It will make your code easier to read. Uppercase first letters are generally reserved for class and interface names. (They are also used for function names in some other languages, but you shouldn’t do that in Kotlin or they’ll get confused with constructor calls.)