2
\$\begingroup\$

I am writing a very simple test application in Kotlin for Android, where I just want to calculate the time it takes for someone to reach a specific amount of money, taking into count his salary and increase in salary each year.

I am very new to Android Studio, which is why I don't really know about the best practises yet. Currently, I wonder wheather I should put the EditText into global scope, so that I can access them without passing any parameters to all the functions.

This is my class:

class MainActivity : AppCompatActivity() {

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)

        findViewById(R.id.resultsButton).setOnClickListener {

            val moneyField: EditText = findViewById(R.id.moneyField) as EditText
            val moneyGoal: Float = getNumberFromTextField(moneyField)

            val salaryField: EditText = findViewById(R.id.salaryField) as EditText
            val salary: Float = getNumberFromTextField(salaryField)

            val increaseField: EditText = findViewById(R.id.increaseField) as EditText
            val increase: Float = getNumberFromTextField(increaseField)

            calculateAndDisplayTimeUntilMoneyGoal(moneyGoal, salary, increase)
        }
    }

    fun getNumberFromTextField(field: EditText): Float {
        return field.text.toString().toFloat()
    }

    fun calculateAndDisplayTimeUntilMoneyGoal(goal: Float, salary: Float, increaseInPercent: Float){
        var moneyEarned: Float = 0f
        var currentSalary = salary
        var months: Int = 0
        var yearsPassed: Int = 0

        while (moneyEarned < goal){
            moneyEarned += currentSalary
            months++

            if (oneYearPassed(months)){
                currentSalary += increaseSalary(currentSalary, increaseInPercent)
                months = 0
                yearsPassed++
            }
        }

        val resultsString = "After $yearsPassed years and $months months you will have earned $moneyEarned!"
        (findViewById(R.id.displayText) as TextView).text = resultsString
    }

    fun oneYearPassed(months: Int): Boolean = (months == 12)

    fun increaseSalary(currentSalary: Float, increaseInPercent: Float): Float {
        return (currentSalary / 100) * increaseInPercent
    }
}

Any suggestions on how I can improve it? (Altough Kotlin is new to me, I have experience in other programming languages)

| improve this question | | | | |
\$\endgroup\$
1
\$\begingroup\$

I'm not an Android developer, but I can give you some general advice.

You put one listener on resultsButton, but maybe you should put listeners on all input fields so that when a user modifies one field, the answer is updated right away, or at least the result field should be blanked out. I don't know Android, so maybe this is done automatically.

There is the whole issue of making sure the values entered by the user are valid floats. You should find out more how Android deals with this. Hopefully adding android:inputType="numberDecimal" will take care of everything and you don't have to do anything on your side. But you should test it, trying to enter letters, or nothing. If you need to handle problematic values, you can look at toFloatOrNull, but then you have to extend your code to handle Float? instead of Float.

As pointed out by @Marcel Blanck, you should probably make the input fields class members. Otherwise it would be better to redefine getNumberFromTextField to grab the field and extract the value. At the moment you have the same pattern of getting the field and extracting the value repeated three times, and you don't use the field values for anything else.

You output the final result in years and months, but you might change your mind and switch to weeks or days. The best thing would be to compute the result in years, ie. 3.67 years, and prepare the output string with months or weeks. This is called "separation of concerned": the computation of the result and the way you display the result are two separate things.

Units can lead to confusion and bugs. It seems the salary is monthly (I usually see yearly or weekly salary), but the raise is annual. You should probably stick to annual values for both and make it clear to the user. Also your main computation loop is quite complicated because of the whole kerfuffle with years and months. Either you can switch the whole computation to years as I suggested, but if you don't want that, you should at least switch to using only months. You can get for example an answer of 43 months, where there was an increase in the monthly salary at months 12, 24 and 36.

For percentage, I personally ask for percent from the user, ie. 10%, but then convert it immediately to a ratio, ie. 1.1, and use that throughout. I obviously name such a variable with ratio and not percent. I have been burned too many times by forgetting to multiply or divide by 100. But that is a very minor preference on my part.

I'm a big fan of breaking everything in smaller functions, but I think oneYearPassed is excessive, and probably also increaseSalary.

However you should break calculateAndDisplayTimeUntilMoneyGoal. As its own name indicates, it does two different things. This is again "separation of concern": the computation and the displaying are two separate things.

Instead of using a while-loop, I tried to solve the problem using a more functional style with Kotlin sequences (similar to Java Streams). I'm copying it here in case someone might be interested, but it is probably less readable than a while-loop.

fun main(args: Array<String>) {
    val salary = 50000f // annual
    val annualIncreaseRatio = 1.1f
    val goal = 200000f

    data class Accumulator(val year: Int, val salary: Float, val total: Float)
    val seed = Accumulator(0, salary, 0f)
    val accumulators = generateSequence(seed) { accum ->
        Accumulator(year = accum.year + 1,
                salary = accum.salary * annualIncreaseRatio,
                total = accum.total + accum.salary)
    }
    println(accumulators.take(4).toList())

    val terminal = accumulators.find { it.total >= goal }
    println(terminal)

    if (terminal != null) {
        val unusedYear = (terminal.total - goal) / terminal.salary
        val yearsToGoal = terminal.year - unusedYear
        println(yearsToGoal)

        val ceilingYears = Math.ceil(yearsToGoal.toDouble()).toInt()
        val unusedMonths = Math.floor((ceilingYears - yearsToGoal) * 12.0).toInt()
        if (unusedMonths > 0) {
            print("${ceilingYears - 1} years and ${12 - unusedMonths} months")
        } else {
            print("${ceilingYears} years")
        }
    }
}
| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ Although I stopped working on it a long time, I really think you have valid points there! Thank you :) Yes, I also think I might have gone a little overkill with the oneYearPassed() function... will keep that in mind for the future \$\endgroup\$ – OhMad Aug 2 '17 at 19:51
1
\$\begingroup\$

Hi yes I would store the textEdit references in a field of the class (not actual global scope), if you want to access them more often.

Running through the view hierarchy to find it on each click is not the most performant thing for more complex layout and so not best practice. These references stay valid until onDestroyed is called, so it's ok to store them.

BUT with Kotlin you would usually do it like this without casts etc. https://kotlinlang.org/docs/tutorials/android-plugin.html

import kotlinx.android.synthetic.main.activity_main.*

class MyActivity : Activity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)
        resultsButton.setOnClickListener {
            // Either try catch NumberFormatException or validating the string with
            // TextWatcher will be required to avoid problems here, but also in the original code.
            calculateAndDisplayTimeUntilMoneyGoal(
                goal: Float.valueOf(moneyField.getText().toString()),
                salary: Float.valueOf(salaryField.getText().toString()), 
                increaseInPercent: Float.valueOf(increaseField.getText().toString()))
        }
    }
};
| improve this answer | | | | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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