6
\$\begingroup\$

I have been learning Java for a few months now and have created a basic calculator application in Android Studio for a school project. Since this is my first attempt at programming, I am sure that my code is not as succinct as it could be.

public class MainActivity extends AppCompatActivity {
double no1;
double no2;
double total;
char operator;

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);

    final Button btn0 = (Button)findViewById(R.id.btn0);
    final Button btn1 = (Button)findViewById(R.id.btn1);
    final Button btn2 = (Button)findViewById(R.id.btn2);
    final Button btn3 = (Button)findViewById(R.id.btn3);
    final Button btn4 = (Button)findViewById(R.id.btn4);
    final Button btn5 = (Button)findViewById(R.id.btn5);
    final Button btn6 = (Button)findViewById(R.id.btn6);
    final Button btn7 = (Button)findViewById(R.id.btn7);
    final Button btn8 = (Button)findViewById(R.id.btn8);
    final Button btn9 = (Button)findViewById(R.id.btn9);
    final Button btnPlus = (Button)findViewById(R.id.btnPlus);
    final Button btnMinus = (Button)findViewById(R.id.btnMinus);
    final Button btnMultiply = (Button)findViewById(R.id.btnMultiply);
    final Button btnDivide = (Button)findViewById(R.id.btnDivide);
    final Button btnEquals = (Button)findViewById(R.id.btnEquals);
    final Button btnClear = (Button)findViewById(R.id.btnClear);
    final EditText editText = (EditText)findViewById(R.id.editText);

    btn0.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            String btn0Text = btn0.getText().toString();
            editText.setText(editText.getText() + btn0Text);
        }
    });

    btn1.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            String btn1Text = btn1.getText().toString();
            editText.setText(editText.getText() + btn1Text);
        }
    });

    btn2.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            String btn2Text = btn2.getText().toString();
            editText.setText(editText.getText() + btn2Text);
        }
    });

    btn3.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            String btn3Text = btn3.getText().toString();
            editText.setText(editText.getText() + btn3Text);
        }
    });

    btn4.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            String btn4Text = btn4.getText().toString();
            editText.setText(editText.getText() + btn4Text);
        }
    });

    btn5.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            String btn5Text = btn5.getText().toString();
            editText.setText(editText.getText() + btn5Text);
        }
    });

    btn6.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            String btn6Text = btn6.getText().toString();
            editText.setText(editText.getText() + btn6Text);
        }
    });

    btn7.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            String btn7Text = btn7.getText().toString();
            editText.setText(editText.getText() + btn7Text);
        }
    });

    btn8.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            String btn8Text = btn8.getText().toString();
            editText.setText(editText.getText() + btn8Text);
        }
    });

    btn9.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            String btn9Text = btn9.getText().toString();
            editText.setText(editText.getText() + btn9Text);
        }
    });

    btnClear.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            editText.setText("");
        }
    });

    btnPlus.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            no1 = Double.parseDouble(editText.getText().toString());
            operator = btnPlus.getText().charAt(0);
            editText.setText("");
        }
    });

    btnMinus.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            no1 = Double.parseDouble(editText.getText().toString());
            operator = btnMinus.getText().charAt(0);
            editText.setText("");
        }
    });

    btnMultiply.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            no1 = Double.parseDouble(editText.getText().toString());
            operator = btnMultiply.getText().charAt(0);
            editText.setText("");
        }
    });

    btnDivide.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            no1 = Double.parseDouble(editText.getText().toString());
            operator = btnDivide.getText().charAt(0);
            editText.setText("");
        }
    });

    btnEquals.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            no2 = Double.parseDouble(editText.getText().toString());
            switch(operator) {
                case '+':
                    total = no1 + no2;
                    break;
                case '-':
                    total = no1 - no2;
                    break;
                case 'x':
                    total = no1 * no2;
                    break;
                case '÷':
                    total = no1 / no2;
                    break;
            }
            editText.setText(Double.toString(total));
            no1 = 0.0;
            no2 = 0.0;
        }
    });

}
}
\$\endgroup\$
  • \$\begingroup\$ What do you want in answer? What are you expecting? \$\endgroup\$ – shridutt kothari Sep 7 '16 at 3:09
  • 1
    \$\begingroup\$ Some suggestions on how I can make my code more brief as a lot of it is just repeated code \$\endgroup\$ – Jenni Sep 7 '16 at 13:51
  • \$\begingroup\$ Ok than AxelH's answer is more than sufficient. \$\endgroup\$ – shridutt kothari Sep 9 '16 at 12:36
3
\$\begingroup\$

My review will mainly be readable code

When you have lots of button, I am used to regroup the code in a single listener. This have the advantage to regroup the code in one place (but the inconvenient to have the code in one place ;) ). In general, I use the activity or the Fragment to implements the listener and manager itself the buttons.

In your case, you use the same logic for a lot of buttons (numerics to set the value). Why not use a logic that will works for all.

Let's start step by step

First step, I would use an array of Resource ID to recover the buttons to remove this big block of Buttons attribution.

int [] ids = {
   R.id.btn0,
   R.id.btn1
   R.id.btn2,
   R.id.btn3,
   R.id.btn4,
   R.id.btn5,
   R.id.btn6,
   R.id.btn7,
   R.id.btn8,
   R.id.btn9,
   R.id.btnPlus,
   R.id.btnMinus,
   R.id.btnMultiply,
   R.id.btnDivide,
   R.id.btnEquals,
   R.id.btnClear
}

//Loop on each ids.
for(int i : ids){
    //find the resource id by "i"...
}

Then, instead of using one listener per button, you use one listener (I use the Activity in this case but you could create a class). This means you don't need to store the Button in a variable, you set the listener directly like this :

for(int i : ids){
    ((Button)findViewById(i)).setOnClickListener(this); //Like I said, my activity will implements ClickListener.
}

Then you regroup the code in the one method onClick and manage the code to execute by check the ID of the button (the view received).

public void onClick(View v) {
    switch(v.getId()){ //the View will be the button clicked
        case R.id.btn0:
        ...
        case R.id.btn9:
        //then the cases for operators, and others...
    }
}

This means that all the calculating codes are in this listener, this is better than to have to scroll in a lot of lines just reading the signature of the methods ;)

This is of course just an esthetic choice. This doesn't improve performance.

The code will look like

public class MainActivity extends AppCompatActivity implements OnClickListener {
    private EditText editText;
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        int [] ids = {
          R.id.btn0,
          R.id.btn1
          R.id.btn2,
          R.id.btn3,
          R.id.btn4,
          R.id.btn5,
          R.id.btn6,
          R.id.btn7,
          R.id.btn8,
          R.id.btn9,
          R.id.btnPlus,
          R.id.btnMinus,
          R.id.btnMultiply,
          R.id.btnDivide,
          R.id.btnEquals,
          R.id.btnClear
        }

        for(int i : ids){
            ((Button)findViewById(i)).setOnClickListener(this); //Like I said, my activity will implements ClickListener.
        }

        //Need this one since you need it later
        editText = (EditText)findViewById(R.id.editText);


    }

    public void onClick(View v) {
        switch(v.getId()){
            case R.id.btn0:
            case R.id.btn1:
            case R.id.btn2:
            case R.id.btn3:
            case R.id.btn4:
            ...
            case R.id.btn9:
                String text = ((Button)v).getText().toString();
                editText.setText(editText.getText() + text);
                break;
            //then the cases for operators, and others...
        }
    }
}

For the code itself, I did not really check so I will leave other to do it ;)

PS : I am open to suggestion, I tried to stay simple and explicit but I am still a beginner in Code Review ;)

| improve this answer | |
\$\endgroup\$

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.