HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
adultsthenumberpaymentchildrencomboselectionsomebaseddetermine

Problem

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

Solution

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;

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.