6
\$\begingroup\$

So I'm almost 100% certain I've done something wrong simply due to the inefficiency of what I've written, I'm entirely a novice to coding and am working on an assignment for my introductory programming course. The only way I could think to display the input from 4 check boxes was to go down 16 different branches to cover every possible input option, is there any simpler or quicker way to show the results on a listbox? This project is for a mock cruise invoice

        //Onboard services expense clarification//
        int finedining;
        finedining = int.Parse("200");

        int fitnesstrainer;
        fitnesstrainer = int.Parse("50");

        decimal excursions;
        excursions = decimal.Parse("399.9");

        decimal airporttransfer;
        airporttransfer = decimal.Parse("45.5");

        decimal noservices;
        noservices = decimal.Parse("0.00");


        //Check Box Data//
        if (diningCheckBox.Checked && !fitnessCheckBox.Checked && !excursionsCheckBox.Checked && !airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fine Dining");
            valueListBox.Items.Add(finedining);
        }
        else if (diningCheckBox.Checked && fitnessCheckBox.Checked && !excursionsCheckBox.Checked && !airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fine Dining, Fitness Trainer");
            valueListBox.Items.Add(finedining + fitnesstrainer);
        }
        else if (diningCheckBox.Checked && fitnessCheckBox.Checked && excursionsCheckBox.Checked && !airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fine Dining, Fitness Trainer, Excursions");
            valueListBox.Items.Add(finedining + fitnesstrainer + excursions);
        }
        else if (diningCheckBox.Checked && fitnessCheckBox.Checked && excursionsCheckBox.Checked && airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fine Dining, Fitness Trainer, Excursions, Airport Transfer");
            valueListBox.Items.Add(finedining + fitnesstrainer + excursions + airporttransfer);
        }
        else if (diningCheckBox.Checked && !fitnessCheckBox.Checked && excursionsCheckBox.Checked && airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fine Dining, Excursions, Airport Transfer");
            valueListBox.Items.Add(finedining + excursions + airporttransfer);
        }
        else if (diningCheckBox.Checked && fitnessCheckBox.Checked && !excursionsCheckBox.Checked && airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fine Dining, Fitness Trainer, Airport Transfer");
            valueListBox.Items.Add(finedining + fitnesstrainer + airporttransfer);
        }
        else if (diningCheckBox.Checked && !fitnessCheckBox.Checked && !excursionsCheckBox.Checked && airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fine Dining, Airport Transfer");
            valueListBox.Items.Add(finedining + airporttransfer);
        }
        else if (diningCheckBox.Checked && !fitnessCheckBox.Checked && excursionsCheckBox.Checked && !airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fine Dining, Excursions");
            valueListBox.Items.Add(finedining + excursions);
        }
        else if (!diningCheckBox.Checked && fitnessCheckBox.Checked && excursionsCheckBox.Checked && airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fitness Trainer, Excursions, Airport Transfer");
            valueListBox.Items.Add(fitnesstrainer + excursions + airporttransfer);
        }
        else if (!diningCheckBox.Checked && fitnessCheckBox.Checked && excursionsCheckBox.Checked && !airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fitness Trainer, Excursions");
            valueListBox.Items.Add(fitnesstrainer + excursions);
        }
        else if (!diningCheckBox.Checked && fitnessCheckBox.Checked && !excursionsCheckBox.Checked && airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fitness Trainer, Airport Transfer");
            valueListBox.Items.Add(fitnesstrainer + airporttransfer);
        }
        else if (!diningCheckBox.Checked && fitnessCheckBox.Checked && !excursionsCheckBox.Checked && !airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Fitness Trainer");
            valueListBox.Items.Add(fitnesstrainer);
        }
        else if (!diningCheckBox.Checked && !fitnessCheckBox.Checked && excursionsCheckBox.Checked && airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Excursions, Airport Transfer");
            valueListBox.Items.Add(excursions + airporttransfer);
        }
        else if (!diningCheckBox.Checked && !fitnessCheckBox.Checked && excursionsCheckBox.Checked && !airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Excursions");
            valueListBox.Items.Add(excursions);
        }
        else if (!diningCheckBox.Checked && !fitnessCheckBox.Checked && !excursionsCheckBox.Checked && airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("Airport Transfer");
            valueListBox.Items.Add(airporttransfer);
        }
        else if (!diningCheckBox.Checked && !fitnessCheckBox.Checked && !excursionsCheckBox.Checked && !airportCheckBox.Checked)
        {
            titlesListBox.Items.Add("No onboard services requested");
            valueListBox.Items.Add(noservices);``
\$\endgroup\$
  • 1
    \$\begingroup\$ Where is this code located exactly? It seems to be living in a form's code-behind.. is it in some Click handler for some Ok button? \$\endgroup\$ – Mathieu Guindon Feb 1 '16 at 16:40
  • 1
    \$\begingroup\$ The code is from a button click to compile the results of all imputed data onto a listbox. If this question isn't intended for this site I'm sorry I'm not quite sure where to ask it nor where to find the answer \$\endgroup\$ – Jason Feb 1 '16 at 16:42
2
\$\begingroup\$

From the comments:

The code is from a button click to compile the results of all imputed data onto a listbox.

It's not the job of a button's Click handler to to all that work... and the work in question should be much, much simpler ;-)

How?

Your approach is very prone to bugs and errors, and writing a test to programmatically ensure that this complicated logic works as intended, is pretty much impossible.

Create an abstraction to represent any of the items - think in terms of objects here, ask yourself what's common to all these things? - the answer is probably something along these lines:

public class PackageItem
{
    public PackageItem(string description, decimal price)
    { 
        Description = description;
        Price = price;
    }  

    bool IsIncluded { get; set; }
    string Description { get; set; }
    decimal Price { get; set; }
}

You'd have a class like this for each item that's possible to select - notice how the type of Price doesn't need to change from int to decimal if you go on a 30% sale.. don't pick a type for its value, pick a type for its usage: currency should be decimal, regardless of whether the value is 200 or 199.99.

By using a class to encapsulate this concept, you're making sure that the Price is always going to be a decimal - that will avoid quite a few headaches when comes the time to tally up the bill.

Then you would make a "model" for your form, that would include a list of all possible items:

private PackageItem _excursions = new PackageItem("Excursion",399.9);
private PackageItem _fineDining = new PackageItem("Fine dining", 200);
private PackageItem _fitnessTraining = new PackageItem("Fitness trainer", 50);
private PackageItem _airportTransfer = new PackageItem("Airport transfer", 45.5);

Now, when a checkbox' Checked value changes, you can toggle the corresponding item's IsIncluded property:

private void diningCheckBox_Checked(object sender, EventArgs e)
{
    _fineDining.IsIncluded = diningCheckBox.Checked;
}

private void fitnessCheckBox_Checked(object sender, EventArgs e)
{
    _fitnessTraining.IsIncluded = fitnessCheckBox.Checked;
}

//...

Your approach for adding the descriptions into the titlesListBox suggests that a simple TextBox would have worked just as well, since you're only ever adding a single string to that list.

Consider simply iterating your PackageItem objects and adding an item whenever it's included - your click handler could be as simple as this:

var items = new[] { _excursions, _fineDining, _fitnessTraining, _airportTransfer };
foreach (var item in items)
{
    if (item.IsIncluded)
    {
        titlesListBox.Add(item.Description);
        valueListBox.Add(item.Price);
    }
}

From there, computing the total value should be a breeze.

Notice that the click handler doesn't care about the checkboxes - it's only looking at abstractions representing whatever these checkboxes stand for.

It's not Model-View-Presenter yet, but it would be a good step in the right direction I think.

I've left out the "No services" item for you to figure out.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.