Code Review, Refactoring Calculations through a series of textboxes.

Question

The winforms program (gloryfied excelsheet) 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 righthand 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;

   namespace Budgy
   {
    public 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");
       }



   }
 }

Answer 1 · 2016-07-15T18:33:46Z

July 15, 2016 6:33pm

Since only partial code is shown here (and none of the HTML), some of this is based on guesswork.

Rather than looking towards inheritance, I would probably start refactoring by removing all the "variables used in the calculation", and replacing the OnCalculatedButtonClick function (which I'm betting is not actually an event handler) with something like this:

        private TextBox FindTextBox(string basename, int number)
        {
            return (TextBox)Page.FindControl(basename + number.ToString());
        }

        public void CalculateCost(n, int ContractDuration,
            double MonthlyFee, double PhonePrice, double AdditionalCost)
        {
            FindTextBox("tbCalculatedCost", n).Text =
                ((((ContractDuration * 12) * MonthlyFee)
                    + PhonePrice + AdditionalCost)
                  / (ContractDuration * 12)).ToString("F2");
        }

And then the "if (isComplete)" section in CalculatedCostButton_Click with something like this:

            if (isComplete)
            {
                //Parsing the variables (getting number from text)
                for (int i = 1; i <= 6; i++)
                {
                    CalculateCost(i,
                        int.Parse(FindTextBox("tbContractDuration", i).Text),
                        double.Parse(FindTextBox("tbMonthlyFee", i).Text),
                        double.Parse(FindTextBox("tbPhonePrice", i).Text),
                        double.Parse(FindTextBox("tbAdditionalCost", i).Text));
                }
            }

The 41% size reduction you get from these changes should go a long way towards improved readability and ease of maintenance.

Welcome to the Treehouse Community

Looking to learn something new?

Chanel van Oijen

Chanel van Oijen

Code Review, Refactoring Calculations through a series of textboxes.

1 Answer

Steven Parker

Steven Parker