5
\$\begingroup\$

Hello I am an absolute beginner with Kotlin and Android Studio. I want to hear what I could do better for the next time or any flaws my code has.

The code implements a four-function calculator (+ - ✕ ÷).

The MainActivity file:


import androidx.appcompat.app.AppCompatActivity
import android.os.Bundle
import android.view.View
import android.widget.Button
import android.widget.TextView
import android.widget.Toast

class MainActivity : AppCompatActivity(), View.OnClickListener {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)

        val button0 = findViewById<Button>(R.id.button0)
        val button1 = findViewById<Button>(R.id.button1)
        val button2 = findViewById<Button>(R.id.button2)
        val button3 = findViewById<Button>(R.id.button3)
        val button4 = findViewById<Button>(R.id.button4)
        val button5 = findViewById<Button>(R.id.button5)
        val button6 = findViewById<Button>(R.id.button6)
        val button7 = findViewById<Button>(R.id.button7)
        val button8 = findViewById<Button>(R.id.button8)
        val button9 = findViewById<Button>(R.id.button9)
        val buttonPlus = findViewById<Button>(R.id.buttonPlus)
        val buttonMinus = findViewById<Button>(R.id.buttonMinus)
        val buttonMultiply = findViewById<Button>(R.id.buttonMultiply)
        val buttonDivide = findViewById<Button>(R.id.buttonDivide)
        val buttonClear = findViewById<Button>(R.id.buttonClear)
        val buttonEqual = findViewById<Button>(R.id.buttonEqual)

        button0.setOnClickListener(this)
        button1.setOnClickListener(this)
        button2.setOnClickListener(this)
        button3.setOnClickListener(this)
        button4.setOnClickListener(this)
        button5.setOnClickListener(this)
        button6.setOnClickListener(this)
        button7.setOnClickListener(this)
        button8.setOnClickListener(this)
        button9.setOnClickListener(this)
        buttonPlus.setOnClickListener(this)
        buttonMinus.setOnClickListener(this)
        buttonMultiply.setOnClickListener(this)
        buttonDivide.setOnClickListener(this)
        buttonClear.setOnClickListener(this)
        buttonEqual.setOnClickListener(this)

    }

    var equOnScreen: String? = null

        override fun onClick(p0: View?) {
            val resultOnScreen = findViewById<TextView>(R.id.resultOnScreen)
            val message: String = "Please write two numbers and an operator between them"
            if(equOnScreen.isNullOrBlank() || equOnScreen == message) {
                equOnScreen = ""
            }

            when (p0?.id) {
                R.id.button0 -> equOnScreen = "$equOnScreen" + "0" + " "
                R.id.button1 -> equOnScreen = "$equOnScreen" + "1" + " "
                R.id.button2 -> equOnScreen = "$equOnScreen" + "2" + " "
                R.id.button3 -> equOnScreen = "$equOnScreen" + "3" + " "
                R.id.button4 -> equOnScreen = "$equOnScreen" + "4" + " "
                R.id.button5 -> equOnScreen = "$equOnScreen" + "5" + " "
                R.id.button6 -> equOnScreen = "$equOnScreen" + "6" + " "
                R.id.button7 -> equOnScreen = "$equOnScreen" + "7" + " "
                R.id.button8 -> equOnScreen = "$equOnScreen" + "8" + " "
                R.id.button9 -> equOnScreen = "$equOnScreen" + "9" + " "
                R.id.buttonPlus -> equOnScreen = "$equOnScreen" + "+" + " "
                R.id.buttonMinus -> equOnScreen = "$equOnScreen" + "-" + " "
                R.id.buttonMultiply -> equOnScreen = "$equOnScreen" + "*" + " "
                R.id.buttonDivide -> equOnScreen = "$equOnScreen" + "/" + " "
                R.id.buttonClear ->  equOnScreen = ""
                R.id.buttonEqual -> {

                    if(stringdataIntoCalc(equOnScreen!!) != null) {
                        equOnScreen = stringdataIntoCalc(equOnScreen!!).toString() + " "
                    }
                    else equOnScreen = message


                }
            }
            resultOnScreen.text = equOnScreen

        }

        fun stringdataIntoCalc(string : String) : Int? {
            var stringList : MutableList<String>  = string.split(" ").toMutableList()
            if(stringList.size != 4) {return null}
            var first : String = stringList[0]
            var second : String = stringList[2]
            var sign : String = stringList[1]
            var result : Int? = null

            when(sign) {
                "+" -> result =  first.toInt() + second.toInt()
                "-" -> result = first.toInt() - second.toInt()
                "*" -> result = first.toInt() * second.toInt()
                "/" -> result = first.toInt() / second.toInt()
            }

            if(result != null) {
                return result
            }
            return null
        }

    }


Here is the XML file:


<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto"
    xmlns:tools="http://schemas.android.com/tools"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    tools:context=".MainActivity"
    android:orientation="vertical"
    android:background="@color/black"
    >




    <LinearLayout
        android:layout_width="match_parent"
        android:layout_height="350dp"
        android:orientation="vertical"
        android:background="@color/hellblauoderso"
        >
        <TextView
            android:id="@+id/resultOnScreen"
            android:layout_width="match_parent"
            android:layout_height="match_parent"
            android:text="Result"
            android:gravity="center"
            android:textSize="50dp"
            android:textStyle="bold"
            />

    </LinearLayout>

    <LinearLayout
        android:layout_width="match_parent"
        android:layout_height="match_parent"
        android:orientation="vertical"
        >


        <LinearLayout
            android:layout_width="match_parent"
            android:layout_height="match_parent"
            android:layout_weight="1"
            android:gravity="center|bottom"
            android:orientation="horizontal"
            android:paddingVertical="0dp"
            >

            <Button
                android:id="@+id/button1"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="1" />

            <Button
                android:id="@+id/button2"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="2" />

            <Button
                android:id="@+id/button3"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="3" />

            <Button
                android:id="@+id/button4"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="4" />

        </LinearLayout>

        <LinearLayout
            android:layout_width="match_parent"
            android:layout_height="match_parent"
            android:layout_weight="1"
            android:gravity="center|bottom"
            android:orientation="horizontal"
            android:paddingVertical="0dp"
            >

            <Button
                android:id="@+id/button5"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="5" />

            <Button
                android:id="@+id/button6"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="6" />

            <Button
                android:id="@+id/button7"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="7" />

            <Button
                android:id="@+id/button8"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="8" />

        </LinearLayout>

        <LinearLayout
            android:layout_width="match_parent"
            android:layout_height="match_parent"
            android:layout_weight="1"
            android:gravity="center|bottom"
            android:orientation="horizontal"
            android:paddingVertical="0dp"
            >

            <Button
                android:id="@+id/button9"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="9" />

            <Button
                android:id="@+id/button0"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="0" />

            <Button
                android:id="@+id/buttonPlus"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="+" />

            <Button
                android:id="@+id/buttonMinus"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="-" />
        </LinearLayout>

        <LinearLayout
            android:layout_width="match_parent"
            android:layout_height="match_parent"
            android:layout_weight="1"
            android:gravity="center|bottom"
            android:orientation="horizontal"
            android:paddingVertical="0dp"
            >

            <Button
                android:id="@+id/buttonMultiply"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="*" />

            <Button
                android:id="@+id/buttonDivide"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="/" />

            <Button
                android:id="@+id/buttonEqual"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="="
                android:textSize="25dp" />

            <Button
                android:id="@+id/buttonClear"
                style="?android:attr/buttonStyleSmall"
                android:layout_width="100dp"
                android:layout_height="wrap_content"
                android:layout_gravity="center_horizontal|center"
                android:text="clear" />


        </LinearLayout>



    </LinearLayout>





</LinearLayout>

\$\endgroup\$
0

1 Answer 1

5
\$\begingroup\$

Crash

Dividing by 0 crashes the app ungracefully. Detect this and show an error message.

Warnings

Check your "Problems" tab in Android studio. It tells you to use @string resources rather than hardcoded strings, to use sp instead of dp for text sizes and to remove the unused import android.widget.Toast.

UX

Since your calculator can't handle multi-digit numbers or multiple operators, I'd prevent users from being able to enter these illegal expressions in the first place. As a user, it's frustrating to take the time to type in some long equation only to be told it's not possible. At the least, accepting multi-digit numbers is pretty easy to offer by not adding a space when a digit is pressed and equOnScreen's last character is also a digit.

As another UX improvement, having a visual color change upon button press can go a long way to making the app feel better.

Tweaks/nitpicks

  • There is excessive vertical whitespace in the layout.

  • Since you're using one click listener for all of your buttons, you could put them in an array and write a loop instead of setting the listener on each one by hand.

  • The code:

    if(result != null) {
        return result
    }
    return null
    

    Is the same as: return result.

  • Button styling is repetitive; you could add a values/styles.xml like:

    <?xml version="1.0" encoding="utf-8"?>
    <resources>
        <style name="calc_button" parent="@android:style/Widget.Button.Small">
            <item name="android:layout_width">100dp</item>
            <item name="android:layout_height">wrap_content</item>
            <item name="android:layout_gravity">center_horizontal|center</item>
        </style>
    </resources>
    

    then use it on each button like:

    <Button
        android:id="@+id/button1"
        style="@style/calc_button"
        android:text="1" />
    

Design

  • It seems indirect to build up a string representing the current equation, then split it into a list later. Why not append to the list right from the get go, skipping the string? The reason appears to be you're using equOnScreen to store error messages as well as results and intermediate equations. But there's no reason to use it for error messages; you can set resultOnScreen.text = "error message" (which should be a resource in res/strings.xml) in the error branch and resultOnScreen.text = stringdataIntoCalc in the other. This way, you can avoid all the string manipulation and splitting on equOnScreen.
  • Your single onClick might be a bit overburdened. It might make sense to give number buttons their own handler, operators another one, and equals and clear to each have one as well. This separates concerns a bit and reduces the huge when that will be difficult to maintain if you try to add features to the app.
  • stringdataIntoCalc is somewhat awkward and hard to extend, but it does the job for the stripped-down specification here.

All this said, I'm glad you haven't been distracted by excessive premature optimizations and your straightforward, direct approach pretty much gets the job done commensurate with the goal.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ thank you very much for your informative feedback. it was really helpful! \$\endgroup\$
    – cp54lory
    Commented Jan 18, 2022 at 17:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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