I am creating a calculator. But there are too many setOnClickListeners() making it harder to progress. This belongs to a fragment and it also has a ViewModel. I am using dataBinding here. If there is any way I can write less code in the below mentioned context.

if there is any confusion about the question, please write in the comment. If my approach is wrong, share in the comments

MY code:

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
        binding.calculatorViewModel = viewModel
        binding.lifecycleOwner = viewLifecycleOwner
        viewModel.currentExpression.value = "814×122" //temporary value
        viewModel.currentResult.value = "99308" //temporary value
        binding.etInput.doAfterTextChanged {
            viewModel.currentExpression.value = it.toString()
            binding.tvOutputPreview.text = viewModel.currentExpression.value
        }
        binding.apply {
            // Extra operators - setOnClickListener
            btnClear.setOnClickListener { viewModel.onClear() }
            btnAllClear.setOnClickListener { viewModel.onAllClear() }
            btnPlusMinus.setOnClickListener {  }
            btnEqual.setOnClickListener {  }

            // Operators - setOnClickListener
            btnDivide.setOnClickListener {
                viewModel.mountOperator(btnDivide.text) }
            btnMultiply.setOnClickListener { viewModel.mountOperator(btnMultiply.text) }
            btnMinus.setOnClickListener { viewModel.mountOperator(btnMinus.text) }
            btnPlus.setOnClickListener { viewModel.mountOperator(btnPlus.text) }


            //Secondary operators - setOnClickListener
            btnPercent.setOnClickListener {  }
            btnDecimal.setOnClickListener {  }

            // Numbers - setOnClickListener
            btn0Num.setOnClickListener {  }
            btn1Num.setOnClickListener {  }
            btn2Num.setOnClickListener {  }
            btn3Num.setOnClickListener {  }
            btn4Num.setOnClickListener {  }
            btn5Num.setOnClickListener {  }
            btn6Num.setOnClickListener {  }
            btn7Num.setOnClickListener {  }
            btn8Num.setOnClickListener {  }
            btn9Num.setOnClickListener {  }



        }
        binding.btnClear.setOnClickListener { viewModel.onClear() }
        binding.btnAllClear.setOnClickListener { viewModel.onAllClear() }
        binding.btnPlusMinus.setOnClickListener {  }


    }

CodePudding user response：

That's okay to have too many click listeners.

but what you can do to make it look cleaner is you can set the click listener to this fragment or activity.

for example:

btn0Num.setOnClickListener(this)

and then implement View.OnClickListener in your class

and override the onClick method.

  override fun onClick(v: View?) {
    v?.let { 
        when(it){
            btn0Num -> {
              //Todo do something when the button is clicked
            }
            btn1Num -> {
              //Todo do something when the button is clicked
            }
        }
    }
}

CodePudding user response：

Calculator crew today is it

I just posted this on another question, but if you have repetitive code like that, use a loop!

listOf(btnDivide, btnMultiply, btnMinus, btnPlus).forEach {
    it.setOnClickListener { //bla bla }
}

In this case, since your buttons are logically grouped and you might need to refer to a group again, you might want to keep those lists around as a top-level variable:

// lateinit so we don't have to assign it yet, just make sure it's set
// before it's read!
lateinit var digitButtons: List<Button>

override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
    ...
    with(binding) {
        digitButtons = listOf(btn0Num, btn1Num...)
    }
}

then you can refer to it with things like digitButtons.forEach, or if (button in digitButtons) etc

If you're worried about creating so many click listeners, you can reuse a function:

fun handleClick(view: View) {
    // do the stuff
}

digitButtons.forEach {
    setOnClickListener { handleClick(it) }
}
// or with a function reference instead of a lambda
digitButtons.forEach {
    setOnClickListener(::handleClick)
}

And your handling code can use those lists from earlier too

fun handleClick(view: View) {
    when {
        view !is Button -> return
        view in digitButtons -> whateverYouDoWithThose(view)
    }
}

but personally I'd go for separate functions for each button type - for the digits, call a function that handles those. For an operator, call a different function. It's easier to read than one giant "a button was clicked here's how we handle each one" function, and it's more informative when you're assigning the click listeners too, since it reads like "when clicked, do this" and handleDigitPressed is better than handleClick, in my opinion anyway!

And setOnClickListener(::clear) is definitely better, you can immediately see what that button does without having to go look it up in the general click handler function. Having separate, well-named functions can make things a lot easier to parse