Problem
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;
}
});
}
}
Solution
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 😉