patterncsharpModerate
Determine payment based in the number of adults, number of children, and some combo box selection
Viewed 0 times
adultsthenumberpaymentchildrencomboselectionsomebaseddetermine
Problem
How can I shorten this code with a while loop? Is it possible?
```
public int pay;
private void Calculate_Click(object sender, EventArgs e)
{
int TheValue;
int Matatanda = int.Parse(Adults.Text);
int Bata = int.Parse(Children.Text);
int Total = Matatanda + Bata;
if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 0) { TheValue = 1000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 1) { TheValue = 2000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 2) { TheValue = 3000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 3) { TheValue = 4000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 4) { TheValue = 5000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 5) { TheValue = 6000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 6) { TheValue = 7000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 7) { TheValue = 8000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 8) { TheValue = 9000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 9) { TheValue = 10000; Bata = Bata * TheValue / 2; p
cboFrom and cboTo is a combobox. I only know how to do simple while loops and I feel like it's a waste of time to write all the indexes and values of the combox down```
public int pay;
private void Calculate_Click(object sender, EventArgs e)
{
int TheValue;
int Matatanda = int.Parse(Adults.Text);
int Bata = int.Parse(Children.Text);
int Total = Matatanda + Bata;
if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 0) { TheValue = 1000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 1) { TheValue = 2000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 2) { TheValue = 3000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 3) { TheValue = 4000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 4) { TheValue = 5000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 5) { TheValue = 6000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 6) { TheValue = 7000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 7) { TheValue = 8000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 8) { TheValue = 9000; Bata = Bata TheValue / 2; pay = (TheValue Matatanda + Bata) * 2; }
else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 9) { TheValue = 10000; Bata = Bata * TheValue / 2; p
Solution
First of all - you have awful duplication of payment calculation
Next - nothing is changing when you change
So you can move
Now let's look what changes when
Now bring all together. You don't need loop:
I would suggest you to check if your controls have integer value before parsing text (
Then define display and value members of this class for combobox and after that assign collection of
Now getting of selected value is simple as:
Also it would be nice if you'll explain in code why only
pay = (TheValue Matatanda + Bata) 2; you do this in each row. Move calculation to the bottom of method:if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 0)
else if
else if
//..
pay = (TheValue * Matatanda + Bata) * 2;Next - nothing is changing when you change
cboFrom selected item. These lines are completely identicalif (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 0) { TheValue = 1000; Bata = Bata * TheValue / 2; }
if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 0) { TheValue = 1000; Bata = Bata * TheValue / 2; }So you can move
cboFrom selected index out of this code:if (cboFrom.SelectedIndex != 0 cboFrom.SelectedIndex != 1)
return;
if (cboTo.SelectedIndex == 0) { TheValue = 1000; Bata = Bata * TheValue / 2; }
//...
if (cboTo.SelectedIndex == 16) { TheValue = 17000; Bata = Bata * TheValue / 2; }Now let's look what changes when
cboTo selected index is changed. It's only TheValue value. It changes from 1000 to 17000. Which is TheValue = (cboTo.SelectedIndex + 1) * 1000;Now bring all together. You don't need loop:
if (cboFrom.SelectedIndex != 0 cboFrom.SelectedIndex != 1)
return;
if (cboTo.SelectedIndex < 0 || 16 < cboTo.SelectedIndex)
return;
int TheValue = (cboTo.SelectedIndex + 1) * 1000;
int Matatanda = int.Parse(Adults.Text);
int Bata = int.Parse(Children.Text) * TheValue / 2;
int Total = Matatanda + Bata; // NOTE: value is not used
pay = (TheValue * Matatanda + Bata) * 2;I would suggest you to check if your controls have integer value before parsing text (
int.TryParse), separate UI logic from business logic and assign to your comboboxes some objects which have TheValue value. So that you will be able to use it directly without checking indexes. You should create some class with appropriate name:public class Foo // use appropriate name
{
public int Value { get; set; }
public string Name { get; set; }
}Then define display and value members of this class for combobox and after that assign collection of
Foo objects as combobox data source (I use array just as example):cboTo.DisplayMember = "Name";
cboTo.ValueMember = "Value";
cboTo.DataSource = new[]
{
new Foo { Name = "First value description", Value = 1000 },
new Foo { Name = "Second value description", Value = 2000 },
};Now getting of selected value is simple as:
if (cboFrom.SelectedIndex < 0 || 1 < cboFrom.SelectedIndex)
return;
int matatanda;
if (!int.TryParse(Adults.Text, out matatanda)
return; // and show error
int bata;
if (int.TryParse(Children.Text, out bata)
return; // and show error;
int value = (int)cboTo.SelectedValue;
bata = bata * value / 2;
pay = (value * matatanda + bata) * 2;Also it would be nice if you'll explain in code why only
0 and 1 are appropriate values for cboFrom. Something likebool insuranceSelected = cboFrom.SelectedIndex == 0 || cboFrom.SelectedIndex == 1;
if (!insuranceSelected)
return;Code Snippets
if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 0)
else if
else if
//..
pay = (TheValue * Matatanda + Bata) * 2;if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 0) { TheValue = 1000; Bata = Bata * TheValue / 2; }
if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 0) { TheValue = 1000; Bata = Bata * TheValue / 2; }if (cboFrom.SelectedIndex != 0 cboFrom.SelectedIndex != 1)
return;
if (cboTo.SelectedIndex == 0) { TheValue = 1000; Bata = Bata * TheValue / 2; }
//...
if (cboTo.SelectedIndex == 16) { TheValue = 17000; Bata = Bata * TheValue / 2; }TheValue = (cboTo.SelectedIndex + 1) * 1000;if (cboFrom.SelectedIndex != 0 cboFrom.SelectedIndex != 1)
return;
if (cboTo.SelectedIndex < 0 || 16 < cboTo.SelectedIndex)
return;
int TheValue = (cboTo.SelectedIndex + 1) * 1000;
int Matatanda = int.Parse(Adults.Text);
int Bata = int.Parse(Children.Text) * TheValue / 2;
int Total = Matatanda + Bata; // NOTE: value is not used
pay = (TheValue * Matatanda + Bata) * 2;Context
StackExchange Code Review Q#108479, answer score: 17
Revisions (0)
No revisions yet.