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>
1 Answer 1
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 setresultOnScreen.text = "error message"
(which should be a resource inres/strings.xml
) in the error branch andresultOnScreen.text = stringdataIntoCalc
in the other. This way, you can avoid all the string manipulation and splitting onequOnScreen
. - 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 hugewhen
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.
-
1\$\begingroup\$ thank you very much for your informative feedback. it was really helpful! \$\endgroup\$cp54lory– cp54lory2022年01月18日 17:34:13 +00:00Commented Jan 18, 2022 at 17:34