You probably already have noticed that there is a lot of "similar" code there (and asked your question for that reason). And in fact you can simplify it a lot.
Before we start with that, let's correct one flaw in your code:
You parse the text entered into
txtFixAmount
into
fixAmount
and on the following line you assign the result of
15000 / 26 / 8 to
fixAmount
. So the text from
txtFixAmount
has no effect at all. Maybe your intention was to multiply or divide the entered value by the result of
15000 / 26 / 8 ? I'll go with the assumption that you wanted to multiply it.
First code-simplification: The only thing that makes the code of the "if"-branch different from the "else if"-branch is your multiplier for
fixAmount
(
1.25 resp.
2.5). If you introduce a new variable (I will call it
multiplier
here) you can reduce the code that depends on the "if else-if" to assigning either 1.25 or 2.5 to
multiplier
. And everything else just follows and uses
multiplier
instead of a hard-coded value.
Also: If you need a converted value (in this case:
cbType1.SelectedItem.ToString()
) multiple times, assign it to a variable (
selectedItem
here) and use that variable in the following code instead:
private void txtTime1_TextChanged(object sender, EventArgs e)
{
double fixAmount = double.Parse(txtFixAmount.Text);
fixAmount *= 15000 / 26 / 8;
string selectedItem = cbType1.SelectedItem.ToString();
double multiplier = 0;
if (selectedItem == "Regular")
{
multiplier = 1.25;
}
else if (selectedItem == "Double")
{
multiplier = 2.5;
}
double x = fixAmount * multiplier;
if (txtTime1.Text.Length > 0)
{
txtAmount1.Text = (x * double.Parse(txtTime1.Text)).ToString();
}
else if (txtTime1.Text.Length <= 0 || txtTime1.Text == null)
{
txtAmount1.Text = "0.00";
}
}
Next up: Good practices.
1)
double.Parse
will throw an Exception if the entered text can't be parsed into a double. If you use
double.TryParse
instead, you will get a boolean result indicating whether the parsing succeeded or not which allows you to handle that case gracefully. We can apply this to both lines where you currently use double.Parse.
2) Whenever you use an "else-if" you should also have a concluding "else", because, in this case, you
might change your program so that there are more options than "Regular" and "Double" but might forget to include an extra "else-if" that deals with that case at first. If you have a concluding "else" that throws an Exception you will become aware of your mistake very quickly and won't have to search through your code to find the bug.
private void txtTime1_TextChanged(object sender, EventArgs e)
{
double fixAmount;
if (Double.TryParse(txtFixAmount.Text, out fixAmount))
{
fixAmount *= 15000 / 26 / 8;
string selectedItem = cbType1.SelectedItem.ToString();
double multiplier;
if (selectedItem == "Regular")
{
multiplier = 1.25;
}
else if (selectedItem == "Double")
{
multiplier = 2.5;
}
else
{
throw new NotImplementedException(String.Format("Unrecognized selection: {0}", selectedItem));
}
double x = fixAmount * multiplier;
double time;
if (Double.TryParse(txtTime1.Text, out time))
{
txtAmount1.Text = (x * time).ToString();
}
else
{
txtAmount1.Text = "0.00";
}
}
else
{
txtFixAmount.Text = "0.00";
}
}
Next code-simplification: Looking at your original code, you will recognize that
txtTime1_TextChanged
and
txtTime2_TextChanged
essentially do the exact same thing, just for different ComboBoxes and TextBoxes. You can eliminate this de-facto code-duplication as well. For that you have to replace the hard-coded use of specific ComboBox- and TextBox-objects by variables and have your TextChanged-EventHandler-Methods call that new method with the appropriate ComboBox- and TextBox-objects:
private void txtTime1_TextChanged(object sender, EventArgs e)
{
ProcessTextChanged(cbType1, txtTime1, txtAmount1);
}
private void txtTime2_TextChanged(object sender, EventArgs e)
{
ProcessTextChanged(cbType2, txtTime2, txtAmount2);
}
private void ProcessTextChanged(ComboBox cbType, TextBox txtTime, TextBox txtAmount)
{
double fixAmount;
if (Double.TryParse(txtFixAmount.Text, out fixAmount))
{
fixAmount *= 15000 / 26 / 8;
string selectedItem = cbType.SelectedItem.ToString();
double multiplier;
if (selectedItem == "Regular")
{
multiplier = 1.25;
}
else if (selectedItem == "Double")
{
multiplier = 2.5;
}
else
{
throw new NotImplementedException(String.Format("Unrecognized selection: {0}", selectedItem));
}
double x = fixAmount * multiplier;
double time;
if (Double.TryParse(txtTime.Text, out time))
{
txtAmount.Text = (x * time).ToString();
}
else
{
txtAmount.Text = "0.00";
}
}
else
{
txtFixAmount.Text = "0.00";
}
}
Final words:
- Theoretically
cbType.SelectedItem
could be null. It would be good practise to check that before accessing it with
.ToString()
.
- In case some input can't be parsed you shouldn't just silently reset the text to "0.00" but also show some notification to the user, e.g. a
MessageBox
.
- The strings "Regular" and "Double" shouldn't be hard-coded here - it would be better to have one "central" point in your program where you initialize variables with it and use these to fill your ComboBoxes as well as use them for the comparisons here.
Hope it helps :-)