Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a toy app that contains:

  • editText1 for entering a number. Uses inputType of number.
  • editText2 for entering a second number. Uses inputType of number.
  • adderButton for triggering addition.
  • textVeiw for displaying the result of the addition.

I know:

  • The naming could be better
  • I should check for overflow/underflow on the addition and elsewhere.

This code is working for simple cases.

 public void adder_click(View view) {
        EditText et1 = (EditText) findViewById(R.id.editText1);
        EditText et2 = (EditText) findViewById(R.id.editText2);
        Integer s = Integer.valueOf(et1.getText().toString()) + Integer.valueOf(et2.getText().toString());
        TextView tv = (TextView) findViewById(R.id.textView);
        tv.setText(s.toString());   
    }

Is there a more direct way of reading the values and performing the addition?

share|improve this question

2 Answers 2

up vote 5 down vote accepted

No, that's as direct as it's going to be with an EditText (you could try it with a NumberPicker), as you cannot directly get an integer from it.

But you could extract the process into its own method:

private int getInt(EditText editText) throws NumberFormatException {
    return Integer.valueOf(editText.getText().toString());
}

If you also use what @mleyfman said, your code would look like this:

public void adder_click() {
    tv.setText(getInt(et1) + getInt(et2));   
}

Also note that I removed the method argument, as it's unused.

And for the record, yes, the naming isn't very good. It's easy to just leave the names as editText1, editText2, etc, but this will get confusing really fast (as will tv, et1, et2, etc). Also, use camelCase (adder_click should be adderClick)

share|improve this answer
2  
Welcome to the 1K club! –  syb0rg Aug 20 at 17:13
    
@syb0rg thanks :) –  tim Aug 20 at 17:18
    
I accepted this answer because it offered number picker as an alternative method. –  ben rudgers Aug 24 at 4:25

That code is a bit dangerous. Integer.valueOf() can throw NumberFormatException which you make no effort to catch. This will happen if you have commas in numbers or non-digit characters. Your other comments are also very important.

As for simplifying the method, it is not good that you call findViewById 3 times every time you press a button. I would store references to editText1, editText2 and textView externally (of this method), as then you can reuse all three here and elsewhere in your code with subtract/multiply etc. You can then remove three lines of code and just have:

Integer s = Integer.valueOf(et1.getText().toString()) + Integer.valueOf(et2.getText().toString());
tv.setText(s.toString());

Remember that you should probably wrap the code in a try catch block unless you can guarantee the text field will only contain decimal numbers. Also, add in your overflow/underflow logic as well.

share|improve this answer
    
For edtitText1 and editText2 the inputType is set to number in the activity's XML. So only digits can be input from the onscreen keyboard in the emulator. But could non-digits be pasted into the text field? I am using Android Studio if this context doesn't make sense in Eclipse or another development environment. –  ben rudgers Aug 20 at 19:40
    
If that is the case, then the code is safe. If at some point someone touches the XML and removes that, then the code might blow up then. In general, I always code with the philosophy: better safe than sorry, so that is why I urge you to be diligent. –  mleyfman Aug 20 at 19:58
    
I appreciate the wisdom. The adder program was literally just my very first attempt to wire up an event. And so I stumbled blindly into the the realization that I had to use reflection. –  ben rudgers Aug 20 at 20:21
1  
@benrudgers you could also add checks like if(!TextUtils.isEmpty(et1.getText().toString())){//do something} to check emptiness –  Raghunandan Aug 27 at 6:31

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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