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

Refactoring Calculations through a series of textboxes

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

Problem

Alright guys, I know this isn't the proper way of problem solving, But I need some guidance here. Since I am just starting in programming, the code is not really readable.

The Winforms program (essentially a glorified Excel sheet) I created is made up from different textboxes (as shown in designer picture below). The textboxes are used for input of numbers. All the input is extracted, converted to int or double and run through the calculation. The textboxes on the right hand side (orange boxes) are used to show the calculated price to the users.

The questions are:

How do I make the code more readable by encapsulating and inheritance?

What do I use to make a list of every variable and how do I make a standard calculation and loop every pair of textboxes through the same calculation?

```
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Drawing.Printing;
using System.Drawing.Imaging;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

partial class BudgyMobilePlan : Form
{
//declaring the variables used in the calculation

int ContractDuration1;
int ContractDuration2;
int ContractDuration3;
int ContractDuration4;
int ContractDuration5;
int ContractDuration6;

double MonthlyFee1;
double MonthlyFee2;
double MonthlyFee3;
double MonthlyFee4;
double MonthlyFee5;
double MonthlyFee6;

double PhonePrice1;
double PhonePrice2;
double PhonePrice3;
double PhonePrice4;
double PhonePrice5;
double PhonePrice6;

double AdditionalCost1;
double AdditionalCost2;
double AdditionalCost3;
double AdditionalCost4;
double AdditionalCost5;
double AdditionalCost6;

private void CalculatedCostButton_Click(object sender, EventArgs e)
{
boo

Solution

Honestly, it looks like you want to use a domain object to model your providers and perform the pay calculations. Rather than using a bunch of text fields and drop-down controls, you can then data bind your domain object to a DataGridView.

Domain Object

You want an object which has properties for all your display values. Since data binding is involved, you need to ensure the object implements the INotifyPropertyChanged interface, like so:

public sealed class Provider : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    public string Name
    {
        get { return _name; }
        set
        {
            _name = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Name)));
        }
    }

    public PlanType PlanType
    {
        get { return _planType; }
        set
        {
            _planType = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(PlanType)));
        }
    }

    public double Duration
    {
        get { return _duration; }
        set
        {
            _duration = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Duration)));
        }
    }

    public double MonthlyFee
    {
        get { return _monthlyFee; }
        set
        {
            _monthlyFee = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(MonthlyFee)));
        }
    }

    public double Price
    {
        get { return _price; }
        set
        {
            _price = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Price)));
        }
    }

    public double AdditionalCost
    {
        get { return _additionalCost; }
        set
        {
            _additionalCost = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(AdditionalCost)));
        }
    }

    public double Pay { get { return _pay; } }

    public void CalculatePay()
    {
        var months = Duration * 12;
        _pay = months * MonthlyFee + Price + AdditionalCost / months;
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Pay)));
    }

    private string _name;
    private PlanType _planType;
    private double _duration;
    private double _monthlyFee;
    private double _price;
    private double _additionalCost;
    private double _pay;
}


For the plan type, you can simply use an enumeration:

public enum PlanType
{
    CoolPlan,
    BadPlan,
    GoodPlan,
}


Without knowing your domain, I just threw in placeholders for illustration purposes.

Data Binding

If your list of providers is fixed, you can bind any IEnumerable to the grid. However, in the more likely case you want to allow adding new providers, I suggest instantiating and binding to a BindingList.

I would then add a BindingSource to your form which points to the Provider type, then set the DataSource property of the grid to the binding source, and tweak your columns to look how you like. In your code-behind, you will set the DataSource property on this BindingSource to your collection of providers.

The PlanType field required a little extra care. In this case, you want to set up the column as a DataGridViewComboBoxColumn. Once you do that, you want to supply the possible list of values for PlanType to the column so the drop-down works as expected. This can be done easily with Enum.GetValues:

var availableTypes = Enum.GetValues(typeof(PlanType));
planTypeDataGridViewTextBoxColumn.DataSource = availableTypes;


Pay Calculation

The calculation step is now a simple matter of enumerating your providers and calling CalculatePay:

private void calculateButton_Click(object sender, EventArgs e)
{
    foreach(var provider in _providers)
    {
        provider.CalculatePay();
    }
}


That's assuming, of course, that you even want to wait to calculate pay.

Another alternative which might be more usable would be to calculate on-the-fly as users enter values. To do this, you add a call to CalculatePay in Provider's property setters:

public double Duration
{
    get { return _duration; }
    set
    {
        _duration = value;
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Duration)));
        CalculatePay();
    }
}


For this to work out, you have to tweak the CalculatePay method to handle zero values:

public void CalculatePay()
{
    if (Duration == 0)
    {
        _pay = 0;
    }
    else
    {
        var months = Duration * 12;
        _pay = months * MonthlyFee + Price + AdditionalCost / months;
    }
    PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Pay)));
}


As a result, you could eliminate the Calculate button.

The end result is that your code-behind for the form looks something more like the following (minus the click handler if you decided to go with auto-updati

Code Snippets

public sealed class Provider : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    public string Name
    {
        get { return _name; }
        set
        {
            _name = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Name)));
        }
    }

    public PlanType PlanType
    {
        get { return _planType; }
        set
        {
            _planType = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(PlanType)));
        }
    }

    public double Duration
    {
        get { return _duration; }
        set
        {
            _duration = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Duration)));
        }
    }

    public double MonthlyFee
    {
        get { return _monthlyFee; }
        set
        {
            _monthlyFee = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(MonthlyFee)));
        }
    }

    public double Price
    {
        get { return _price; }
        set
        {
            _price = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Price)));
        }
    }

    public double AdditionalCost
    {
        get { return _additionalCost; }
        set
        {
            _additionalCost = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(AdditionalCost)));
        }
    }

    public double Pay { get { return _pay; } }


    public void CalculatePay()
    {
        var months = Duration * 12;
        _pay = months * MonthlyFee + Price + AdditionalCost / months;
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Pay)));
    }

    private string _name;
    private PlanType _planType;
    private double _duration;
    private double _monthlyFee;
    private double _price;
    private double _additionalCost;
    private double _pay;
}
public enum PlanType
{
    CoolPlan,
    BadPlan,
    GoodPlan,
}
var availableTypes = Enum.GetValues(typeof(PlanType));
planTypeDataGridViewTextBoxColumn.DataSource = availableTypes;
private void calculateButton_Click(object sender, EventArgs e)
{
    foreach(var provider in _providers)
    {
        provider.CalculatePay();
    }
}
public double Duration
{
    get { return _duration; }
    set
    {
        _duration = value;
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Duration)));
        CalculatePay();
    }
}

Context

StackExchange Code Review Q#134995, answer score: 8

Revisions (0)

No revisions yet.