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>
Toby Speight
87.1k14 gold badges104 silver badges322 bronze badges
asked Jan 16, 2022 at 9:16
\$\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.

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

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.