Reputation: 199
Well, I have a code here for my calculator but it seems so messy.
The logic here is when the display is equal to 0 the next input well replace the display except for 0 itself. Just try seeing this image. https://www.dropbox.com/s/qtec2r6mcsmjvvt/refactor.jpg?dl=0
private void btn1_Click(object sender, EventArgs e)
{
num = btn1.Text;
if (isEqual)
{
if (operator_pressed == true)
{
if (expr.EndsWith("0"))
{
tbScreen.Clear();
tbScreen.Text += num;
expr = expr.Remove(expr.Length - 1, 1);
expr += num;
}
else
{
tbScreen.Clear();
expr = "";
tbScreen.Text += num;
expr += num;
}
}
else
{
if (tbScreen.Text == "0")
{
tbScreen.Clear();
tbScreen.Text += num;
expr += num;
}
else
{
tbScreen.Clear();
expr = "";
tbScreen.Text += num;
expr += num;
}
}
}
else {
if (operator_pressed == true)
{
if (expr.EndsWith("0"))
{
tbScreen.Clear();
tbScreen.Text += num;
expr = expr.Remove(expr.Length - 1, 1);
expr += num;
}
else
{
tbScreen.Clear();
tbScreen.Text += num;
expr += num;
}
}
else
{
if (tbScreen.Text == "0")
{
tbScreen.Clear();
tbScreen.Text += num;
expr += num;
}
else
{
tbScreen.Text += num;
expr += num;
}
}
}
isEqual = false;
operator_pressed = false;
btnEqual.Focus();
}
Hoping for a positive response!
Upvotes: 0
Views: 107
Reputation: 236208
First of all, you should improve naming of your variables. It's not clear what the purpose of this method. I can only suggest that you are creating some kind of calculator. When you write your code business value should be clear, and when you ask to refactor code, you should explain what your code should do in business terms. E.g.
I want to be able to type in math expression "1 + 5 - 2" then when I press 'Equals' button I should see result of calculation and expression in calculation history. This method calculates expression result and updates controls. operator_pressed is a flag which is set when user press some operator button. Etc
Without knowing purpose of your code I can only focus on removing duplicates. But intent is still will not be very clear for other developers. All your current code can be simplified to
num = btn1.Text;
if (IsInputStarted)
tbScreen.Clear();
if (IsExpressionStarted)
expr = "";
if (operator_pressed && expr.EndsWith("0"))
expr = expr.Remove(expr.Length - 1, 1);
tbScreen.Text += num;
expr += num;
isEqual = false;
operator_pressed = false;
btnEqual.Focus();
With two properties (or you can use methods) extracted. One checks whether screen should be cleared:
private bool IsInputStarted
{
get { return isEqual || operator_pressed || tbScreen.Text == "0"; }
}
Second verifies whether you should clear current expression
private bool IsExpressionStarted
{
get
{
if (!isEqual)
return false;
if (operator_pressed)
return !expr.EndsWith("0");
return tbScreen.Text != "0";
}
}
Further recommendations - do not mix your UI code (UI controls, UI events) with your business logic. These things should live and change separately. I would recommend you to create some kind of Calculator
class which will be responsible for making calculations and storing expressions. And your code will look like
private void EqualsButton_Click(object sender, EventArgs e)
{
double result = calculator.ExecuteExpression();
resultsTextBox.Text = result.ToString();
historyListBox.Items.Add(calculator.Expression);
}
Upvotes: 2
Reputation: 898
in your case I would suggest to think of a condition for every single action you want to perform.
When do you want to execute tbScreen.Clear()
?
if (isEqual || operator_pressed || tbScreen.Text == "0" )
tbScreen.Clear();
When do you want to execute tbScreen += num;
? - always, so just write
tbScreen += num;
When do you want to execute expr = "";
?
if (isEqual && ((operator_pressed && !expr.EndsWith("0")) || (!operator_pressed && tbScreen.Text != "0"))
expr = "";
and so on..
you will get much shorter and easier to overlook code
Upvotes: 1