patternjavaMinor
Calculator Android App
Viewed 0 times
calculatorandroidapp
Problem
I was looking for a review on my coding. Since I'm planning to get a job as a Android Programmer in the long run.
This is my first calculator app, which I did by self learning Programming. I have no other Programming Langauge experience.
So I would love to hear how bad my coding is?
Which areas I need improving on?
What I should have used instead of repeating my self, which is bad coding as I understand.
```
private Button btn1, btn2, btn3, btn4, btn5, btn6, btn7, btn8, btn9, btnZero, btnDoubleZero, btnEqual, btnMultiply, btnDevide,
btnPlus, btnMinus, btnClean, btnPoint;
private TextView textView1, totalSum;
private String lastInputNumber = "";
private double num1 = 0;
private double num2 = 0;
private double tempCount = 0;
private double tempSum = 0;
private int myClickCount = 0;
private boolean nSwitch = true;
private boolean equalSwitch = false;
String stringMinus = " - ";
String[] operation = {
};
String[] lastNumber = {
};
ArrayList op = new ArrayList();
ArrayList ln = new ArrayList();
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_main);
btn1 = (Button) findViewById(R.id.button3);
btn2 = (Button) findViewById(R.id.button7);
btn3 = (Button) findViewById(R.id.button11);
btn4 = (Button) findViewById(R.id.button2);
btn5 = (Button) findViewById(R.id.button6);
btn6 = (Button) findViewById(R.id.button10);
btn7 = (Button) findViewById(R.id.button1);
btn8 = (Button) findViewById(R.id.button5);
btn9 = (Button) findViewById(R.id.button9);
btnZero = (Button) findViewById(R.id.button4);
btnDoubleZero = (Button) findViewById(R.id.button8);
btnEqual = (Button) findViewById(R.id.button20);
btnMultiply = (Button) findViewById(R.id.button14);
btnDevide = (Button) findViewById(R.id.button13);
btnPlus = (Button) findViewById(R.id.button16);
btnMinus = (Button) findViewById(R.id.button15);
btnClean
This is my first calculator app, which I did by self learning Programming. I have no other Programming Langauge experience.
So I would love to hear how bad my coding is?
Which areas I need improving on?
What I should have used instead of repeating my self, which is bad coding as I understand.
```
private Button btn1, btn2, btn3, btn4, btn5, btn6, btn7, btn8, btn9, btnZero, btnDoubleZero, btnEqual, btnMultiply, btnDevide,
btnPlus, btnMinus, btnClean, btnPoint;
private TextView textView1, totalSum;
private String lastInputNumber = "";
private double num1 = 0;
private double num2 = 0;
private double tempCount = 0;
private double tempSum = 0;
private int myClickCount = 0;
private boolean nSwitch = true;
private boolean equalSwitch = false;
String stringMinus = " - ";
String[] operation = {
};
String[] lastNumber = {
};
ArrayList op = new ArrayList();
ArrayList ln = new ArrayList();
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_main);
btn1 = (Button) findViewById(R.id.button3);
btn2 = (Button) findViewById(R.id.button7);
btn3 = (Button) findViewById(R.id.button11);
btn4 = (Button) findViewById(R.id.button2);
btn5 = (Button) findViewById(R.id.button6);
btn6 = (Button) findViewById(R.id.button10);
btn7 = (Button) findViewById(R.id.button1);
btn8 = (Button) findViewById(R.id.button5);
btn9 = (Button) findViewById(R.id.button9);
btnZero = (Button) findViewById(R.id.button4);
btnDoubleZero = (Button) findViewById(R.id.button8);
btnEqual = (Button) findViewById(R.id.button20);
btnMultiply = (Button) findViewById(R.id.button14);
btnDevide = (Button) findViewById(R.id.button13);
btnPlus = (Button) findViewById(R.id.button16);
btnMinus = (Button) findViewById(R.id.button15);
btnClean
Solution
You have a large amount of code duplication. The highly technical description of your problem is 'DRY' or Don't Repeat Yourself.
I would say that fixing three issues in your code would make the rest of the code a lot simpler to manage. Once those three are done, you should bring your code back for review.
You have completely munged the names of the resources in your Layout.xml file. Your button resources should have ID's that match the button. Why is
You should specify your buttons as an array of
Then, when you set up your Activity, you can loop over the enum members:
Similarly, there are a lot of other places where you can reduce the code footprint using the loop.
Switch on Enum
With your buttons represented as enum members, your big switch statement can become:
You should have better handling for what you do with the buttons as well, but these three changes will go a long way to making your code more readable.
I would say that fixing three issues in your code would make the rest of the code a lot simpler to manage. Once those three are done, you should bring your code back for review.
- Resources
You have completely munged the names of the resources in your Layout.xml file. Your button resources should have ID's that match the button. Why is
R.id.button3 actually Button btn1? You should re-identify all your resources and make the resource names match the use of the resource. This is a simple fix, but would have been better to get right the first time around.- Button Arrays
You should specify your buttons as an array of
Button and you should populate it accordingly. Even better would be an enum of Button.public enum CalculatorButton {
ZERO(R.id.CalcZero),
ONE(R.id.CalcOne),
....
POINT(R.id.CalcPoint),
...
;
public static final CalculatorButton getOwner(int buttonid) {
for (CalculatorButton cb : values()) {
if (cb.getButtonID() == button) {
return cb;
}
}
return null;
}
private final int calcbutton;
private CalculatorButton(int calcbutton) {
this.calcbutton = caclbutton;
}
public int getButtonID() {
return calcbutton;
}
}Then, when you set up your Activity, you can loop over the enum members:
for (CalculatorButton cb : CalculatorButton.values()) {
cb.getButton().setOnClickListener(this);
cb.getButton().setTypeface(btnTf)
}Similarly, there are a lot of other places where you can reduce the code footprint using the loop.
Switch on Enum
With your buttons represented as enum members, your big switch statement can become:
public void onClick(View v) {
switch (CalculatorButton.getOwner(v.getId())) {
case ONE:
....
case POINT:
....
}You should have better handling for what you do with the buttons as well, but these three changes will go a long way to making your code more readable.
Code Snippets
public enum CalculatorButton {
ZERO(R.id.CalcZero),
ONE(R.id.CalcOne),
....
POINT(R.id.CalcPoint),
...
;
public static final CalculatorButton getOwner(int buttonid) {
for (CalculatorButton cb : values()) {
if (cb.getButtonID() == button) {
return cb;
}
}
return null;
}
private final int calcbutton;
private CalculatorButton(int calcbutton) {
this.calcbutton = caclbutton;
}
public int getButtonID() {
return calcbutton;
}
}for (CalculatorButton cb : CalculatorButton.values()) {
cb.getButton().setOnClickListener(this);
cb.getButton().setTypeface(btnTf)
}public void onClick(View v) {
switch (CalculatorButton.getOwner(v.getId())) {
case ONE:
....
case POINT:
....
}Context
StackExchange Code Review Q#40807, answer score: 5
Revisions (0)
No revisions yet.