Problem with adding totals from user selected CheckBoxes

Question

I am creating a form that allows the user to select from a group of checkboxes for automotive services. In the form, the user selects from a list of priced services and a final total is calculated based on what is selected.

The logic of the selected services being added up is placed within a method that returns the total.

.

Once the user clicks on the calculate button, all selected prices will be added up and displayed by the total fees label.

    public partial class Automotive_Shop : Form
    {
        const int salesTax = (6 / 100);
        // prices for services
        const int
            oilChange = 26,
            lubeJob = 18,
            radiatorFlush = 30,
            transissionFlush = 80,
            inspection = 15,
            mufflerReplacement = 100,
            tireRotation = 20;

        int total = 0;

        public Automotive_Shop()
        {
            InitializeComponent();
        }




        private int OilLubeCharges()
        {
            if (oilChangeCheckBox.Checked == true)
            {
                total += oilChange;
            }                 
            if (lubeJobCheckBox.Checked == true)
            {
                total += lubeJob;
            }
          
            return total;
        }
           
      

      


        private void calculateButton_Click(object sender, EventArgs e)
        {
            totalFeesOutput.Text = OilLubeCharges().ToString("C");
                
        }

        private void exitButton_Click(object sender, EventArgs e)
        {
            // close application
            this.Close();
        }
    }

The total should only be added once.

For instance: if the "oil change" check box is selected, then the total should be $26.

if the "lube job" check box is selected, then the total should be $18.

And if both check boxes are selected, then the total should be $44.

What ends up happening is that after the first check box is selected and the calculate button is clicked, the "total" variable value continues to be added up.

So if i select "oil change" then click calculate, I get $26. if I deselect it and select "lube job" the total doesn't equal $18, but $44.

Answer 1

You mention "the logic of the selected services..." and this phrase is insightful! Consider that one approach is to separate that logic from the view (i.e. the Form that allows interaction with that logic) because it's clear enough that when any property changes it causes ripple effects in certain other properties and the behavior is well-defined. If this "business logic" is placed in a non-UI class that models the desired behavior, the properties can be made smart enough to send notification events whenever they change.

For example, if the dollar amount for Parts is changed, it triggers a recalculation of Tax and the new value for Tax triggers a recalculation of the TotalFees property in turn.

class AutoShopModel : INotifyPropertyChanged
{
    protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
    {
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
        switch (propertyName)
        {
            case nameof(Parts):
                recalcTax();
                break;
            case nameof(StandardServiceCharges):
            case nameof(Tax):
            case nameof(Labor):
                recalcTotalFees();
                break;
            default:
                recalcStandardServiceCharges();
                break;
        }
    }
    public event PropertyChangedEventHandler PropertyChanged;        

    private void recalcTotalFees() =>
        TotalFees =
            StandardServiceCharges +
            Parts +
            Labor +
            Tax;
    .
    .
    .
}

This demonstrates that the model is smart enough to maintain the relative values in a consistent internal state regardless of which property changes. Then, synchronizing the changes to the UI is a simple matter of binding controls like CheckBox and TextBox to properties in the model that have been set up to be bindable.

For example, the OilChange property is just a bool and making it bindable simply means firing an event when its value changes:

partial class AutoShopModel
{
    public bool OilChange
    {
        get => _oilChange;
        set
        {
            if (!Equals(_oilChange, value))
            {
                _oilChange = value;
                OnPropertyChanged();
            }
        }
    }
    bool _oilChange = false;
    .
    .
    .
}

Finally, the glue that holds it all together takes place in the Load method of the MainForm where the checkBoxOilChange gets bound to changes of the AutoShopModel.OilChange boolean:

public partial class MainForm : Form
{
    public MainForm() => InitializeComponent();
    protected override void OnLoad(EventArgs e)
    {
        base.OnLoad(e);

        Binding binding = new Binding(
            nameof(CheckBox.Checked), 
            AutoShopModel, 
            nameof(AutoShopModel.OilChange), 
            false, 
            DataSourceUpdateMode.OnPropertyChanged);
        checkBoxOilChange.DataBindings.Add(binding);
        .
        .
        .
    }
    AutoShopModel AutoShopModel { get; } = new AutoShopModel();
}

As a bonus, when you go to make the Android or iOS version of your app, the AutoShopModel is portable and reusable because it doesn't reference any platform-specific UI elements.

If you're inclined to play around with this View-Model idea, I put together a short demo you can clone.

Answer 2

A function should be responsible for whatever it does. So everything it does should be there, nothing more and nothing less.

This means when a member function is calculating a total, and in the comments you refer to it as a subtotal, this is what the function should do.

Therefore you declare in that function int subtotal = 0; and return that.

Then you could store this to a member variable if you like.

---

As an example regarding your comments, I have added the member function int ApplyDiscount(...).

The only thing it does is applying a discount to 'a' subtotal you pass in. It should be improved for a working application.

---

At the button click the OilLubeCharges are calcultated, then that is passed into ApplyDiscount.

This return value you could store at a global variable. Both you could to event specify the full cost, the discount in price and the total.

        // Added a functionality due to the comments
        // When `discount` is 0.2f for 20%
        private int ApplyDiscount(int subtotal, float discount)
        {
            return (int)( subtotal - ( subtotal * discount ) );
        }

        private int OilLubeCharges()
        {

            int subtotal = 0;

            if (oilChangeCheckBox.Checked == true)
            {
                subtotal += oilChange;
            }                 
            if (lubeJobCheckBox.Checked == true)
            {
                subtotal += lubeJob;
            }
          
            return subtotal;

        }

        ...

        private void calculateButton_Click(object sender, EventArgs e)
        {

            int subtotal = OilLubeCharges();
            int total = ApplyDiscount(subtotal, 0.2f);

            totalFeesOutput.Text = total.ToString("C");
                
        }

Answer 3

To fix this problem, the total variable needs to be reset to 0 before the calculation happens.

The calculate button click event should be updated to look like this:

private void calculateButton_Click(object sender, EventArgs e)
{
    total = 0;
    totalFeesOutput.Text = OilLubeCharges().ToString("C");
}