2
\$\begingroup\$

The following app basically sums up the costs for an evening and calculates the costs per person. The user can input the description of the costs [String] (for example, round one = Spare Ribs Place, round two = Beer Place, etc.), the costs of the round [Double] (for example, 50$, 10.15$, etc.) and the amount of people who participated [Int].

The description of the costs and the costs are being taken via an Alertdialog after having pressed the FAB.

After pressing "Calculate" the total sum is divided by the amount of people and displayed in a textview.

I know I have still a long way to go but I am willing to learn and therefore more than thankful for any advice in any respect.

MAIN ACTIVITY.KT

package com.hooni.nbun1kotlin

import android.os.Bundle
import android.view.LayoutInflater
import android.view.View
import android.widget.Button
import android.widget.EditText
import android.widget.TextView
import android.widget.Toast
import androidx.appcompat.app.AlertDialog
import androidx.appcompat.app.AppCompatActivity
import androidx.recyclerview.widget.LinearLayoutManager
import com.google.android.material.snackbar.Snackbar
import kotlinx.android.synthetic.main.activity_main.*
import kotlinx.android.synthetic.main.alertdialog_addvalue.view.*
import kotlinx.android.synthetic.main.toolbar.*

class MainActivity : AppCompatActivity() {

    // Declaring member variables:
    // - mutableList contains data class Entry (Name of Entry, Value of Entry)
    // - sumValue is the sum of the values of all entries
    private val entryList: MutableList<Entry> = mutableListOf()
    private var sumValue: Double = 0.0
    private lateinit var textViewTotalValue: TextView
    private lateinit var textViewValuePerPerson: TextView

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

        initUI()
    }

    private fun initUI() {
        initToolbar()
        initRecyclerView()
        initFAB()
        initButtons()
        initSumFields()
    }
    private fun initRecyclerView() {
        recyclerView_ListOfItems.apply {
            layoutManager = LinearLayoutManager(this@MainActivity)
            adapter = ItemAdapter(entryList)
        }
    }

    private fun initToolbar() {
        toolbar.inflateMenu(R.menu.menu_main)
        toolbar.setOnMenuItemClickListener {
            when(it.itemId) {
                R.id.settings -> Toast.makeText(this, "Settings", Toast.LENGTH_SHORT).show()
                R.id.share -> Toast.makeText(this, "Share", Toast.LENGTH_SHORT).show()
            }
            true
        }
    }

    private fun initFAB() {
        val fab: View = findViewById(R.id.fab)
        fab.setOnClickListener {
            val mDialogView = LayoutInflater.from(this).inflate(R.layout.alertdialog_addvalue,null)
            val mBuilder = AlertDialog.Builder(this).setView(mDialogView).setTitle("Add Entry")
            val mAlertDialog = mBuilder.show()

            // Action when pressing "Cancel" in the popup
            mDialogView.button_cancel.setOnClickListener {
                mAlertDialog.dismiss()
            }

            // If "OK" is being pressed the values from the edit text field (Name & Value of the entry)
            // are converted to the corresponding data types (String, Double) and then added to the
            // list of entries (entryList)
            //
            // The adapter is being notified that there has been a change in the mutableList
            // the sumValue (Double) is being increased by the entered value and the textView
            // is being updated
            mDialogView.button_ok.setOnClickListener {
                mAlertDialog.dismiss()
                val itemName = mDialogView.editText_itemName.text.toString()
                val itemValue = mDialogView.editText_itemValue.text.toString().toDouble()

                entryList.add(Entry(itemName,itemValue))
                recyclerView_ListOfItems.adapter?.notifyDataSetChanged()
                sumValue += itemValue
                textViewTotalValue.text = sumValue.toString()

                // A snackbar is being shown to undo the action (i.e. removing the entry from the
                // mutableList (entryList), updating sumValue (Double) & the corresponding textView
                val snackbar = Snackbar.make(mainActivity,"Item added",Snackbar.LENGTH_SHORT)
                snackbar.setAction("Undo") {
                    entryList.remove(Entry(itemName,itemValue))
                    recyclerView_ListOfItems.adapter?.notifyDataSetChanged()
                    sumValue -= itemValue
                    textViewTotalValue.text = sumValue.toString()
                }
                snackbar.show()
            }
        }
    }


    private fun initButtons() {

        // the UI for increasing the amount of people & calculate button are initialized
        val buttonIncrease = findViewById<Button>(R.id.button_increasePeople)
        val buttonDecrease = findViewById<Button>(R.id.button_decreasePeople)
        val editTextAmountOfPeople = findViewById<TextView>(R.id.textView_amountOfPeople)
        val buttonCalculate = findViewById<Button>(R.id.button_calculate)

        // the textView displaying the amount of People is set to "2"
        editTextAmountOfPeople.text = "2"

        // OnClickListener for increasing/decreasing amount of People & Calculating are set
        buttonIncrease.setOnClickListener {
            if(editTextAmountOfPeople.text.toString().toInt() < 10) editTextAmountOfPeople.text = (editTextAmountOfPeople.text.toString().toInt() + 1).toString()
            else (Toast.makeText(this@MainActivity,"10 People Maximum",Toast.LENGTH_SHORT)).show()
        }

        buttonDecrease.setOnClickListener {
            if(editTextAmountOfPeople.text.toString().toInt() > 2) editTextAmountOfPeople.text = (editTextAmountOfPeople.text.toString().toInt() - 1).toString()
            else (Toast.makeText(this@MainActivity,"2 People Minimum",Toast.LENGTH_SHORT)).show()
        }

        buttonCalculate.setOnClickListener {
            val valuePerPerson = sumValue / editTextAmountOfPeople.text.toString().toDouble()
            textViewValuePerPerson.text = valuePerPerson.toString()
        }
    }

    private fun initSumFields() {
        textViewTotalValue = findViewById(R.id.textView_totalValue)
        textViewValuePerPerson = findViewById(R.id.textView_valuePerPerson)
    }

    // Entry containing the name and the value
    data class Entry(var name: String, var value: Double)
}

ITEMADAPTER.KT

package com.hooni.nbun1kotlin

import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.recyclerview.widget.RecyclerView
import kotlinx.android.synthetic.main.listview_item.view.*

class ItemAdapter (private val items: MutableList<MainActivity.Entry>) :
    RecyclerView.Adapter<ItemAdapter.ItemViewHolder>() {

    override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): ItemViewHolder {
        return ItemViewHolder(LayoutInflater.from(parent.context)
            .inflate(R.layout.listview_item,parent,false))
    }

    override fun getItemCount(): Int {
        return items.size
    }

    override fun onBindViewHolder(holder: ItemViewHolder, position: Int) {
        when(holder) {
            is ItemViewHolder -> {
                holder?.itemDescription?.text = items.get(position).name
                holder?.itemValue?.text = items.get(position).value.toString()
            }
        }
    }

    class ItemViewHolder constructor(view: View) : RecyclerView.ViewHolder (view) {
        val itemDescription = view.listView_valueDescription!!
        val itemValue = view.listView_value!!
    }

}

ACTIVITY_MAIN.XML

<?xml version="1.0" encoding="utf-8"?>
<androidx.constraintlayout.widget.ConstraintLayout 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:id="@+id/mainActivity"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    tools:context=".MainActivity"
    android:orientation="vertical">

    <include
        layout="@layout/toolbar"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        app:layout_constraintEnd_toEndOf="parent"
        app:layout_constraintStart_toStartOf="parent"
        app:layout_constraintTop_toTopOf="parent" />


    <com.google.android.material.floatingactionbutton.FloatingActionButton
        android:id="@+id/fab"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
        android:layout_marginRight="16dp"
        android:layout_marginBottom="8dp"
        android:src="@mipmap/fab_add"
        app:layout_constraintBottom_toTopOf="@id/linearLayout_BottomBar"
        app:layout_constraintRight_toRightOf="parent" />

    <androidx.recyclerview.widget.RecyclerView
        android:id="@+id/recyclerView_ListOfItems"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        app:layout_constraintEnd_toEndOf="parent"
        app:layout_constraintHorizontal_bias="0.0"
        app:layout_constraintStart_toStartOf="parent"
        app:layout_constraintTop_toTopOf="parent"
        app:layout_constraintBottom_toTopOf="@id/linearLayout_BottomBar">

    </androidx.recyclerview.widget.RecyclerView>

    <LinearLayout
        android:id="@+id/linearLayout_BottomBar"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:orientation="horizontal"
        app:layout_constraintBottom_toBottomOf="parent"
        app:layout_constraintEnd_toEndOf="parent"
        app:layout_constraintStart_toStartOf="parent">

        <LinearLayout
            android:layout_width="wrap_content"
            android:layout_height="match_parent"
            android:orientation="horizontal">

            <Button
                android:id="@+id/button_decreasePeople"
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="-" />

            <TextView
                android:id="@+id/textView_amountOfPeople"
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="2" />

            <Button
                android:id="@+id/button_increasePeople"
                android:layout_width="wrap_content"
                android:layout_height="wrap_content"
                android:text="+" />
        </LinearLayout>

        <Button
            android:id="@+id/button_calculate"
            android:layout_width="match_parent"
            android:layout_height="match_parent"
            android:text="Calculate" />

    </LinearLayout>

    <LinearLayout
        android:id="@+id/linearLayout"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:orientation="horizontal"
        app:layout_constraintEnd_toEndOf="parent"
        app:layout_constraintStart_toStartOf="parent"
        app:layout_constraintTop_toBottomOf="@+id/toolbar">

        <TextView
            android:id="@+id/textView_valuePerPerson"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:gravity="center"
            tools:text="50000" />

        <TextView
            android:id="@+id/textView_totalValue"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:gravity="center"
            tools:text="100000" />
    </LinearLayout>


</androidx.constraintlayout.widget.ConstraintLayout>

ALERTDIALOG_ADDVALUE.XML

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:orientation="vertical">

    <EditText
        android:id="@+id/editText_itemName"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:ems="10"
        android:inputType="text"
        android:hint="Enter Description"/>

    <EditText
        android:id="@+id/editText_itemValue"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:ems="10"
        android:inputType="numberDecimal"
        android:hint="Enter Value"/>

    <LinearLayout
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:orientation="horizontal">

        <Button
            android:id="@+id/button_cancel"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:text="@android:string/cancel" />

        <Button
            android:id="@+id/button_ok"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:text="@android:string/ok" />
    </LinearLayout>
</LinearLayout>

LISTVIEW_ITEM.XML

<?xml version="1.0" encoding="utf-8"?>
<androidx.cardview.widget.CardView 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="wrap_content"
    android:layout_margin="4dp"
    app:cardElevation="4dp">

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

        <TextView
            android:id="@+id/listView_valueDescription"
            android:layout_width="match_parent"
            android:layout_height="match_parent"
            android:layout_gravity="center"
            android:layout_marginStart="16dp"
            android:layout_marginLeft="16dp"
            android:layout_marginEnd="8dp"
            android:layout_marginRight="8dp"
            android:layout_weight="2"
            android:gravity="center_vertical|start"
            android:textSize="24sp"
            tools:text='맛있는 식당"'>

        </TextView>

        <TextView
            android:id="@+id/listView_value"
            android:layout_width="match_parent"
            android:layout_height="match_parent"
            android:layout_gravity="center"
            android:layout_weight="4"
            android:gravity="center"
            android:textSize="24sp"
            tools:text="60,000">

        </TextView>
    </LinearLayout>

</androidx.cardview.widget.CardView>

TOOLBAR.XML

<?xml version="1.0" encoding="utf-8"?>
<androidx.appcompat.widget.Toolbar 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:id="@+id/toolbar"
    android:layout_width="match_parent"
    android:layout_height="wrap_content"
    android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
    app:popupTheme="@style/ThemeOverlay.AppCompat.Light"
    android:background="@color/colorPrimary"
    android:elevation="4dp"
    app:title="Toolbar"
    tools:menu="@menu/menu_main"
    />

MENU_MAIN.XML

<?xml version="1.0" encoding="utf-8"?>
<menu xmlns:app="http://schemas.android.com/apk/res-auto"
    xmlns:android="http://schemas.android.com/apk/res/android">
    <item
        android:id="@+id/share"
        android:icon="@android:drawable/ic_menu_share"
        android:title="@string/action_share"
        app:showAsAction="always"></item>
    <item
        android:id="@+id/settings"
        android:icon="@android:drawable/ic_menu_preferences"
        android:title="@string/action_settings"></item>
</menu>
\$\endgroup\$
3
  • 6
    \$\begingroup\$ The site standard is for the title to simply state the task accomplished by the code, and not your concerns about it. Please see How do I ask a good question? for examples, and revise the title accordingly. \$\endgroup\$
    – Martin R
    Commented Sep 15, 2019 at 6:01
  • 5
    \$\begingroup\$ Welcome to Code Review. This question is currently what we call a "code dump". As Martin says, please tell us more about what your app accomplishes, and also retitle the question accordingly. \$\endgroup\$ Commented Sep 15, 2019 at 6:07
  • 1
    \$\begingroup\$ Once you've fixed your question, we'll reopen it so it can be answered. \$\endgroup\$
    – Mast
    Commented Sep 15, 2019 at 10:35

1 Answer 1

2
\$\begingroup\$

variable names

Personally, I recommend to use the Hungarian notation.
Don't search for the Hungarian notation or you would find not to use it ;-)
On a serious note, the Hungarian notation is great, but it's overused and that's why you find websites advocating against it. But in GUI, you have the perfect place for it...

I don't exactly use these abbreviations inside the link, but it can give you ideas.
When you use a standard abreviation like this for your names, recognizing the types will be very short and you will read over them if you don't need it. Also, making variable names shorter will make it easier to think about them.

Example, I always use rec for RecyclerView. this means I would change recyclerView_ListOfItems to recListOfItems. Also, I personally remove all the prepositions and change the name in a single noun instead. So I would change ListOfItems to ItemList so recyclerView_ListOfItems would become recItemList.

  • fab -> fabAdd
    fab doesn't tell you what its purpose. fabAdd does.
  • buttonIncrease -> btnIncrease
  • editTextAmountOfPeople -> etPeopleAmount
    The type doesn't match the name!! either change the type to EditTextView or change et to tv
  • btn_increasePeople -> btnIncrease don't change names in the code and the XML. This can be confusing

use temporary variables

When you use something twice after eachother and it takes some code or it is difficult for the computer (takes long/does work), store it in a temporary variable (A temporary variable is just a variable which lives short so is in a small function block).

btnIncrease.setOnClickListener {
    if(etPeopleAmount.text.toString().toInt() < 10) etPeopleAmount.text = (etPeopleAmount.text.toString().toInt() + 1).toString()
    else (Toast.makeText(this@MainActivity,"10 People Maximum",Toast.LENGTH_SHORT)).show()
}

when you store the people amount in an int, the code will become easier to read:

btnIncrease.setOnClickListener {
    val peopleAmount = etPeopleAmount.text.toString().toInt()
    if(peopleAmount < 10) etPeopleAmount.text = (peopleAmount + 1).toString()
    else (Toast.makeText(this@MainActivity,"10 People Maximum",Toast.LENGTH_SHORT)).show()
}

Item adapter

quick side-step to Item adapter

MutableList

Eveytime you change something in an adapter, you need to call notifyDataSetChanged.
If you ask for a MutableList, the list can be changed so you promise you call the function on a change, which you don't.
It's therefor way better to ask for a List, as a List can't change.
(If you want to change the adapter, add functions which will replace the list and call notifyDataSetChanged)

single-expression functions

When the first word in the body of your function is return, you can simplify it.

override fun getItemCount(): Int {
     return items.size
 }
 // can be simplified to
override fun getItemCount(): Int = items.size
//or even to
override fun getItemCount() = items.size

onbindViewHolder

override fun onBindViewHolder(holder: ItemViewHolder, position: Int) {
    when(holder) {
        is ItemViewHolder -> {
            holder?.itemDescription?.text = items.get(position).name
            holder?.itemValue?.text = items.get(position).value.toString()
        }
    }
}

You know that holder is an ItemViewHolder, as it is the only thing the parameter allows. So let's remove the second check.

override fun onBindViewHolder(holder: ItemViewHolder, position: Int) {
    holder?.itemDescription?.text = items.get(position).name
    holder?.itemValue?.text = items.get(position).value.toString()
}

Next holder cannot be null as ItemViewHolder doesn't have a question mark.
This means the questionmark after holder can be removed

override fun onBindViewHolder(holder: ItemViewHolder, position: Int) {
    holder.itemDescription?.text = items.get(position).name
    holder.itemValue?.text = items.get(position).value.toString()
}

Next, there are some functions that can use symbols instead of text, called operator overloading. Almost every get in Kotlin can therefor be replaced with [].

override fun onBindViewHolder(holder: ItemViewHolder, position: Int) {
    holder.itemDescription?.text = items[position].name
    holder.itemValue?.text = items[position].value.toString()
}

Ok, now back to the activity

Utility functions

You should create functions for code that you reuse.
After you did the former refactoring you read an Int from a TextView 3 times.
(OK two, but sumValue / peopleAmount gives already a Double as sumvalue is a Double and if one of the values in a devision is a Double, it gives you a Double. change it and you have three).

fun readInt(textview: TextView) : Int {
    textview.text.toString().toInt()
}

now you can refactor to:

btnIncrease.setOnClickListener {
    val peopleAmount = readInt(etPeopleAmount)
    if(peopleAmount < 10) etPeopleAmount.text = (peopleAmount + 1).toString()
    else (Toast.makeText(this@MainActivity,"10 People Maximum",Toast.LENGTH_SHORT)).show()
}

extension function
We can improve or function by making it an extension function.
This means that you can create a function for TextView which looks like it is created in the real class.

fun TextView.readInt() : Int {
    //it acts like it's created inside the TextView class, so this refers to TextView
    return this.text.toString().toInt()
}

As you probably know, you don't have to call this, so the function can be changed to:

fun TextView.readInt() : Int {
    return text.toString().toInt()
}

or even simpler:

fun TextView.readInt() = text.toString().toInt()

so the code now becomes:

btnIncrease.setOnClickListener {
    val peopleAmount = etPeopleAmount.readInt()
    if(peopleAmount < 10) etPeopleAmount.text = (peopleAmount + 1).toString()
    else (Toast.makeText(this@MainActivity,"10 People Maximum",Toast.LENGTH_SHORT)).show()
}

You can now change the Toast.makeText().show() calls into an extension-function shortToast(...).

I won't check the XML, as I don't find XML-layouts interesting 😜. I use Anko for this.

\$\endgroup\$

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.