1
\$\begingroup\$

I have the following code which works perfectly fine. I am using the dial pad to set the number and also to send the DTMF tones. But I find the code to bit more repitive. Is there any better way to optimize the code or the approach I have is good enough ? Thank you in advance for your valuable suggestions. Cheers :)

public class MainActivity extends AppCompatActivity implements View.OnClickListener {

Button btnZero, btn1, btn2, btn3, btn4, btn5, btn6, btn7, btn8, btn9, btnAsh, btnHash;

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);

    setContentView(R.layout.activity_main);

    // Buttons
    btn1 = findViewById(R.id.btnOne);
    btn2 = findViewById(R.id.btnTwo);
    btn3 = findViewById(R.id.btnThree);
    btn4 = findViewById(R.id.btnFour);
    btn5 = findViewById(R.id.btnFive);
    btn6 = findViewById(R.id.btnSix);
    btn7 = findViewById(R.id.btnSeven);
    btn8 = findViewById(R.id.btnEight);
    btn9 = findViewById(R.id.btnNine);
    btnZero = findViewById(R.id.btnZero);
    btnAsh = findViewById(R.id.btnAsterisk);
    btnHash = findViewById(R.id.btnHash);

    dialPadTone();
}

public dialPadTone() {
    btn1.setOnClickListener(this);
    btn2.setOnClickListener(this);
    btn3.setOnClickListener(this);
    btn4.setOnClickListener(this);
    btn5.setOnClickListener(this);
    btn6.setOnClickListener(this);
    btn7.setOnClickListener(this);
    btn8.setOnClickListener(this);
    btn9.setOnClickListener(this);
    btnZero.setOnClickListener(this);
    btnAsh.setOnClickListener(this);
    btnHash.setOnClickListener(this);
}

@Override
public void onClick(View view) {
    // Read current phone dialPadnumber
    String phoneNo = dialPadnumber.getText().toString();
    int value;
    if (btnZero.equals(view)) {
        value = 48;
        phoneNo += btnZero.getText();
    } else if (btn1.equals(view)) {
        value = 49;
        phoneNo += btn1.getText();
    } else if (btn2.equals(view)) {
        value = 50;
        phoneNo += btn2.getText();
    } else if (btn3.equals(view)) {
        value = 51;
        phoneNo += btn3.getText();
    } else if (btn4.equals(view)) {
        value = 52;
        phoneNo += btn4.getText();
    } else if (btn5.equals(view)) {
        value = 53;
        phoneNo += btn5.getText();
    } else if (btn6.equals(view)) {
        value = 54;
        phoneNo += btn6.getText();
    } else if (btn7.equals(view)) {
        value = 55;
        phoneNo += btn7.getText();
    } else if (btn8.equals(view)) {
        value = 56;
        phoneNo += btn8.getText();
    } else if (btn9.equals(view)) {
        value = 57;
        phoneNo += btn9.getText();
    } else if (btnAsh.equals(view)) {
        value = 42;
    } else if (btnHash.equals(view)) {
        value = 35;
    } else {
        throw new IllegalStateException("Unexpected value: " + view.getId());
    }

    setPhoneNum(phoneNo);
    Intent intent = new Intent("send_dtmf_tones");
    Message message = new DTMFtones(
            value
    );
    sendBroadcast(intent);
}
}
\$\endgroup\$
1
\$\begingroup\$

Use the android:onClick attribute to get rid of the boilerplate calls to setOnClickListener and findViewById.

The giant if-else statement is very repetitive. The actions taken in all branches are assigning a value to the value variable and appending a digit to the phone number. You could convert the code to data by mapping the button label to a value and a string.

final String label = ((Button) view).getText();
final int value = buttonLabelToValue.get(label);
phoneNo += buttonLabelToPhoneNumberDigit.get(label);

Handle the two special cases '#' and '*' by mapping the PhoneNumberDigit to an empty string.

I never write code to prepare for an UI component calling a wrong listener method. I define the UI and the listeners and if my UI calls the wrong method then it's a bug. My tests must be good enough to catch such scenarios. In this example, not checking the return value of buttonLabelToValue.get(...) for nulls results in NullPointerException instead of the IllegalStateException thrown in your version. Not much difference for the user as the app crashes in both cases.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your suggestion. I will rework it :) \$\endgroup\$
    – cantona_7
    Nov 21 '19 at 11:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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