5
\$\begingroup\$

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?

Budgy / 2nd form

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)
        {
            bool isComplete = true;
            foreach (Control control in this.Controls)
            {
                if (control is TextBox)
                {
                    TextBox tb = control as TextBox;

                    if (string.IsNullOrEmpty(tb.Text))
                    {
                        isComplete = false;
                        tb.ForeColor = Color.White;

                        tb.Text = "0";
                        continue;
                    }
                }
            }
            if (isComplete)
            {
                //Parsing the variables (getting number from text)
                ContractDuration1 = int.Parse(tbContractDuration1.Text);
                ContractDuration2 = int.Parse(tbContractDuration2.Text);
                ContractDuration3 = int.Parse(tbContractDuration3.Text);
                ContractDuration4 = int.Parse(tbContractDuration4.Text);
                ContractDuration5 = int.Parse(tbContractDuration5.Text);
                ContractDuration6 = int.Parse(tbContractDuration6.Text);

                MonthlyFee1 = double.Parse(tbMonthlyFee1.Text);
                MonthlyFee2 = double.Parse(tbMonthlyFee2.Text);
                MonthlyFee3 = double.Parse(tbMonthlyFee3.Text);
                MonthlyFee4 = double.Parse(tbMonthlyFee4.Text);
                MonthlyFee5 = double.Parse(tbMonthlyFee5.Text);
                MonthlyFee6 = double.Parse(tbMonthlyFee6.Text);

                MonthlyFee1 = double.Parse(tbMonthlyFee1.Text);
                MonthlyFee2 = double.Parse(tbMonthlyFee2.Text);
                MonthlyFee3 = double.Parse(tbMonthlyFee3.Text);
                MonthlyFee4 = double.Parse(tbMonthlyFee4.Text);
                MonthlyFee5 = double.Parse(tbMonthlyFee5.Text);
                MonthlyFee6 = double.Parse(tbMonthlyFee6.Text);

                PhonePrice1 = double.Parse(tbPhonePrice1.Text);
                PhonePrice2 = double.Parse(tbPhonePrice2.Text);
                PhonePrice3 = double.Parse(tbPhonePrice3.Text);
                PhonePrice4 = double.Parse(tbPhonePrice4.Text);
                PhonePrice5 = double.Parse(tbPhonePrice5.Text);
                PhonePrice6 = double.Parse(tbPhonePrice6.Text);

                AdditionalCost1 = double.Parse(tbAdditionalCost1.Text);
                AdditionalCost2 = double.Parse(tbAdditionalCost2.Text);
                AdditionalCost3 = double.Parse(tbAdditionalCost3.Text);
                AdditionalCost4 = double.Parse(tbAdditionalCost4.Text);
                AdditionalCost5 = double.Parse(tbAdditionalCost5.Text);
                AdditionalCost6 = double.Parse(tbAdditionalCost6.Text);
            }


            OnCalculateButtonClick();
        }
        private void Reset_Click(object sender, EventArgs e)
        {
            BudgyMobilePlan NewForm = new BudgyMobilePlan();
            NewForm.Show();
            this.Dispose(false);
        }

        public BudgyMobilePlan()
        {
            InitializeComponent();
        }
        // Calculations when Calculate Button is clicked
        public void OnCalculateButtonClick()
        {

            double CalculatedCost1; double CalculatedCost2; double CalculatedCost3; double CalculatedCost4; double CalculatedCost5; double CalculatedCost6;

            CalculatedCost1 = (((ContractDuration1 * 12) * MonthlyFee1) + PhonePrice1 + AdditionalCost1) / (ContractDuration1 * 12); tbCalculatedCost1.Text = CalculatedCost1.ToString("F2");
            CalculatedCost2 = (((ContractDuration2 * 12) * MonthlyFee2) + PhonePrice2 + AdditionalCost2) / (ContractDuration2 * 12); tbCalculatedCost2.Text = CalculatedCost2.ToString("F2");
            CalculatedCost3 = (((ContractDuration3 * 12) * MonthlyFee3) + PhonePrice3 + AdditionalCost3) / (ContractDuration3 * 12); tbCalculatedCost3.Text = CalculatedCost3.ToString("F2");
            CalculatedCost4 = (((ContractDuration4 * 12) * MonthlyFee4) + PhonePrice4 + AdditionalCost4) / (ContractDuration4 * 12); tbCalculatedCost4.Text = CalculatedCost4.ToString("F2");
            CalculatedCost5 = (((ContractDuration5 * 12) * MonthlyFee5) + PhonePrice5 + AdditionalCost5) / (ContractDuration5 * 12); tbCalculatedCost5.Text = CalculatedCost5.ToString("F2");
            CalculatedCost6 = (((ContractDuration6 * 12) * MonthlyFee6) + PhonePrice6 + AdditionalCost6) / (ContractDuration6 * 12); tbCalculatedCost6.Text = CalculatedCost6.ToString("F2");
        }

    }
}
\$\endgroup\$
  • \$\begingroup\$ Have you considered double[] MonthlyFees = new double[6] \$\endgroup\$ – forsvarir Jul 15 '16 at 15:37
  • \$\begingroup\$ @ forsvarir. I did that a while ago, I also tried to put the CalculatedCost into an array. but it didnt loopt through correctly, resulting in the same answer in every "CalculaterdCost" textbox (Pay/Month on the designer) \$\endgroup\$ – Nick van Oijen Jul 15 '16 at 15:54
  • \$\begingroup\$ you have to use arrays. if the code doesnt work post it to stackoverflow not codereview \$\endgroup\$ – pm100 Jul 15 '16 at 16:01
  • \$\begingroup\$ and that is why i didn't post it here \$\endgroup\$ – Nick van Oijen Jul 15 '16 at 18:27
8
\$\begingroup\$

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<T> to the grid. However, in the more likely case you want to allow adding new providers, I suggest instantiating and binding to a BindingList<T>.

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-updating Pay):

public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
        var availableTypes = Enum.GetValues(typeof(PlanType));
        planTypeDataGridViewTextBoxColumn.DataSource = availableTypes;
        _providers = new BindingList<Provider>();
        providerBinding.DataSource = _providers;
    }

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

    private readonly BindingList<Provider> _providers;
}
\$\endgroup\$
  • \$\begingroup\$ my friend, you are a wizard and a scholar. Thank you for the detailed answer. scince i signed up today, my up-vote won't be displayed, Upvoted anyway. \$\endgroup\$ – Nick van Oijen Jul 15 '16 at 18:38
  • 1
    \$\begingroup\$ _pay is a bug source. The class has the option of calling CalculatePay() or using _pay and if these ever get out of synch... More than once I've fixed bugs that were caused by alternate use of a field and its Property (or method). Get rid of _pay. And, one should not be saving calculated values as a general rule; so another reason to not have this apple in the garden of OO Eden. \$\endgroup\$ – radarbob Jul 16 '16 at 0:01
0
\$\begingroup\$

Two quick observations from your code - The double.Parse and int.Parse . What I would do is embed them in a try ... Catch

 try {
        ContractDuration1 = int.Parse(tbContractDuration1.Text);
        // others
     }
     catch (FormatException) {
       // do something
     }   
     catch (OverflowException) {
        // do something
     }  

Alternatively, you could use TryParse-(credit @BCdotWEB)

 bool result = Int32.TryParse(tbContractDuration1.Text, out ContractDuration1);

Secondly, It will be neater if you grouped the int.Parse and double.Parse checks into two different methods.

\$\endgroup\$
  • 4
    \$\begingroup\$ Why do that when there's TryParse? \$\endgroup\$ – BCdotWEB Jul 15 '16 at 15:44
  • \$\begingroup\$ that's an alternative as well @BCdotWEB \$\endgroup\$ – Siobhan Jul 15 '16 at 15:45
  • \$\begingroup\$ @ Tolani Jaiye-Tikolo Thanks, i will group them in different methods @BCdotWEB i´ve read about the TryParse method. It seems a great alternative to the try catch block \$\endgroup\$ – Nick van Oijen Jul 15 '16 at 15:50
  • \$\begingroup\$ @BCdotWEB I already made amends based on your suggestion \$\endgroup\$ – Siobhan Jul 15 '16 at 16:00

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.