Determine payment based in the number of adults, number of children, and some combo box selection

Question

How can I shorten this code with a while loop? Is it possible? 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; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 10) { TheValue = 11000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 11) { TheValue = 12000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 12) { TheValue = 13000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 13) { TheValue = 14000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 14) { TheValue = 15000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 15) { TheValue = 16000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 0 && cboTo.SelectedIndex == 16) { TheValue = 17000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 0) { TheValue = 1000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 1) { TheValue = 2000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 2) { TheValue = 3000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 3) { TheValue = 4000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 4) { TheValue = 5000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 5) { TheValue = 6000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 6) { TheValue = 7000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 7) { TheValue = 8000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 8) { TheValue = 9000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 9) { TheValue = 10000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 10) { TheValue = 11000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 11) { TheValue = 12000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 12) { TheValue = 13000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 13) { TheValue = 14000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 14) { TheValue = 15000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 15) { TheValue = 16000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
    else if (cboFrom.SelectedIndex == 1 && cboTo.SelectedIndex == 16) { TheValue = 17000; Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2; }
}

Sergey Berezovskiy · Accepted Answer · 2015-10-23 11:19:42Z

First of all - you have awful duplication of payment calculation 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 identical

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; }

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 like

 bool insuranceSelected = cboFrom.SelectedIndex == 0 ||  cboFrom.SelectedIndex == 1;
 if (!insuranceSelected)
     return;

I think you should check if SelectedIndex isn't -1 too in case nothing is selected. — t3chb0t
– t3chb0t, Commented Oct 23, 2015 at 10:44
I don't get why he uses SelectedIndex anyway. Wouldn't it make for more sense to use ComboBox.SelectedItem and use an actual value instead? — BCdotWEB
– BCdotWEB, Commented Oct 23, 2015 at 11:43

t3chb0t · Accepted Answer · 2015-10-23 10:42:30Z

@SergeyBerezovskiy has shortend your ifs very nicely so I'd like to just add that you might consider using an array for the values instead of directly using this calculation:

int TheValue = (cboTo.SelectedIndex + 1) * 1000;

As long as you know that there won't be other values it's ok but if you think some of them might change an array could help:

private int[] _values = 
{
    1300,
    2045,
    3000,
    // other values...
}

For now you can however generate all the values:

private int[] _values = Enumerable.Range(1, 18).Select(x => x * 1000).ToArray();

and get them later:

int TheValue = _values[cboTo.SelectedIndex];

Community · Accepted Answer · 2017-05-23 12:40:48Z

Is this an over-simplification of a bigger function? Because you could replace that whole If-ElseIf block (both of them) with a simple function, if the example is a "real" representation:

From example:

cboFrom has no effect on "TheValue" (and thus, the rest). 0 or 1, TheValue result is the same.
- cboTo == 0: TheValue = 1000
- cboTo == 1: TheValue = 2000
- cboTo == 2: TheValue = 3000
- ...
The rest of the function is all copy/pasta. EVERY branch has this same block:
- Bata = Bata * TheValue / 2; pay = (TheValue * Matatanda + Bata) * 2;
Total is unused

So, you have a simple pattern and lots of duplication.

Simplified Provided Example: So all you really need to do is add one to cboTo's index, multiply by 1000 and then do the rest of the function (based on the sample code):

public int pay;      
private void Calculate_Click(object sender, EventArgs e)
{
    // Check for SelectedIndex between 0/1 and 0/16
    if (cboFrom.SelectedIndex < 0 and cboFrom.SelectedIndex >  1) throw new InvalidArgumentException();
    if (cboto.SelectedIndex   < 0 and cboTo.SelectedIndex   > 16) throw new InvalidArgumentException();

    int Matatanda = int.Parse(Adults.Text);
    int Bata = int.Parse(Children.Text);

    int TheValue= (cboTo.SelectedIndex + 1) * 1000;
    Bata = Bata * TheValue / 2; 
    pay = (TheValue * Matatanda + Bata) * 2;
}

Another Option: If the "real" code is more complicated, and From actually affects the outcome... I would probably look into generating an array automatically (as t3's answer) or manually:

int[,] array = new int[2, 17];
array[0,0] = 1000;
array[0,1] = 2000;
...
array[0,16] = 1800;
array[1,0]  = 1000;
...
int TheValue = array[cboFrom.SelectedIndex, cboTo.SelectedIndex];
...

Input Verification: Also, you might want to verify that the int.Parses are successful.

int Matatanda;
bool MatatandaResult = int.TryParse(Adults.Text, out Matatanda);
if (MatatandaResult == false) { throw new ArgumentException; }

Number Rounding:

Not sure what the goal is, but do you want Bata to round up? Down? Does it not matter? What is the expected outcome when Bata * TheValue / 2 resolves?

Right now, it's all integer math. Look at this question for examples. Integer math truncates on the /2 - it doesn't "round" up for .5 if that's what you are expecting. Just something to consider.

Stack Exchange Network

Determine payment based in the number of adults, number of children, and some combo box selection

3 Answers 3

Your Answer

Hot Network Questions

Determine payment based in the number of adults, number of children, and some combo box selection

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

Related

Hot Network Questions