Krdytkyu

I've got 17 currency fields on a form and need to do some calculations with their values. I wrote this method some time ago and have come back to try and refactor it to make it more efficient and/or more readable. Could I have some pointers please?

Note: The variables appended with d are the decimals used to hold the field values, their counterparts without the d are the fields themselves. I am aware that variables could do with renaming to make more sense. Hopefully it's fairly clear what this is doing by looking at the comments.

private void getTotals() {



    //declarations of decimal variables

    decimal curPurc1d; decimal curPurc2d; decimal curPurc3d; decimal curPurc4d; //purItemxCost

    decimal curItem1Totd; decimal curItem2Totd; decimal curItem3Totd; decimal curItem4Totd; //curItemxTot

    decimal LessItem1Costd; decimal LessItem2Costd; decimal LessItem3Costd; decimal LessItem4Costd; decimal LessItem5Costd; //LessItemxCost

    decimal ditem1Cost = 0; decimal ditem2Cost = 0; decimal ditem3Cost = 0; decimal ditem4Cost = 0; //Full cost of items



    //Check if we have some valid numbers, stop if we don't

    if (   !decimal.TryParse(curPurc1.Value.ToString(), out curPurc1d)

        || !decimal.TryParse(curPurc2.Value.ToString(), out curPurc2d)

        || !decimal.TryParse(curPurc3.Value.ToString(), out curPurc3d)

        || !decimal.TryParse(curPurc4.Value.ToString(), out curPurc4d)

        || !decimal.TryParse(curItem1Tot.Value.ToString(), out curItem1Totd)

        || !decimal.TryParse(curItem2Tot.Value.ToString(), out curItem2Totd)

        || !decimal.TryParse(curItem3Tot.Value.ToString(), out curItem3Totd)

        || !decimal.TryParse(curItem4Tot.Value.ToString(), out curItem4Totd)

        || !decimal.TryParse(LessItem1Cost.Value.ToString(), out LessItem1Costd)

        || !decimal.TryParse(LessItem2Cost.Value.ToString(), out LessItem2Costd)

        || !decimal.TryParse(LessItem3Cost.Value.ToString(), out LessItem3Costd)

        || !decimal.TryParse(LessItem4Cost.Value.ToString(), out LessItem4Costd)

        || !decimal.TryParse(LessItem5Cost.Value.ToString(), out LessItem5Costd)            

        )

        return;



        //For each of the 4 'items', try to get the value as a decimal, but only if the related checkbox is checked.

        if (Item1RateYN.Checked) 

        {

            decimal.TryParse(curItem1Cost.Value.ToString(), out ditem1Cost);

        }

        if (Item2RateYN.Checked) 

        {

            decimal.TryParse(curItem2Cost.Value.ToString(), out ditem2Cost);

        }

        if (Item3RateYN.Checked) 

        {

            decimal.TryParse(curItem3Cost.Value.ToString(), out ditem3Cost);

        }

        if (Item4RateYN.Checked) 

        {

            decimal.TryParse(curItem4Cost.Value.ToString(), out ditem3Cost);

        }



    //Add up some values which are always part of the subtotal and then the ditemx ones, which will be 0 or set to a numerical value depending if the checkbox is checked

    decimal subtotals = curPurc1d + curPurc2d + curPurc3d + curPurc4d + curItem1Totd + curItem2Totd + curItem3Totd + curItem4Totd 

                      + ditem1Cost + ditem2Cost + ditem3Cost + ditem4Cost;

    subtotal.Value = subtotals;



    //Get total minus the cost of the property (curPurc1d)

    costPlusSubTotal.Value = subtotals - curPurc1d;



    //add up all the "less" items to know how much to reduce by 

    decimal lessTotals = LessItem1Costd + LessItem2Costd + LessItem3Costd + LessItem4Costd + LessItem5Costd;

    totalLess.Value = lessTotals;



    //Total Balance due

    //subtotal minus the 'less' values total

    decimal total = (subtotals - lessTotals);

    //set the final figure into the relevant field

    balanceDueTot.Value = total;    

}

Two words: loops and arrays.

There a bunch of variable names that differ only by a number. Whenever you see code like this, you can probably clean it up by creating a collection of those things and looping over them. Basically, you have a checkbox and text field repeated multiple times, and one field where you display the total of all checked fields.

1. Create a user control to encapsulate the checkbox and text field


2. Make sure this user control has a public property decimal TotalCost { get } that will:
   
   
   
   
   • Return the decimal-parsed values of total - cost for the fields when checked, and zero when unchecked.
   
   
   • Throws an exception if the decimal cannot be parsed
   
   
   
   


3. Expose a boolean property bool IsValid that returns true when the user as entered a valid decimals


4. Expose a public property bool IsChecked that returns whether or not the checkbox is checked


5. Create a collection of these user controls, one for each checkbox/text field combo on screen

Now loop and process:

decimal purchaseTotal = 0m;

decimal totalAmount = 0m;



foreach (var control in PurchasedItems.Where(p => p.IsChecked && p.IsValid))

{

    purchaseTotal += control.TotalCost;

}

I left some details out, but your code isn't really clear on what the UI looks like, or what the business use case is, but it really just boils down to:

• Create an abstraction for what each combo of controls represents (e.g. create a new user control)


• Create a collection of these controls


• Loop and calculate

Assuming the fields have definitions like the following

public class TextboxControl

{

    public object Value { get; set; }

}



public class CheckboxControl

{

    public bool Checked { get; set; }

}

I would create some extensions for them in order to reduce code duplication and increase readability, and pretty much this is the core idea.

public static class ControlsExtensions

{

    public static bool HasValue(this TextboxControl source) 

            => decimal.TryParse(source.Value.ToString(), out _);



    public static decimal GetValue(this TextboxControl source) 

            => decimal.Parse(source.Value.ToString());



    public static decimal GetOptionalValue(this TextboxControl source, CheckboxControl checkbox) 

    {

        if (checkbox.Checked)

        {

            decimal value = decimal.TryParse(source.Value.ToString(), out value) ? value : 0;

            return value;

        }

        return default(decimal);

    }

}

Then my next step in refactoring would to use these extensions and rearrange a bit the lines to increase cohesion

private void getTotals()

{

  //Check if we have some valid numbers, stop if we don't

    if (   !curPurc1.HasValue()

        || !curPurc2.HasValue()

        || !curPurc3.HasValue()

        || !curPurc4.HasValue()

        || !curItem1Tot.HasValue()

        || !curItem2Tot.HasValue()

        || !curItem3Tot.HasValue()

        || !curItem4Tot.HasValue()

        || !LessItem1Cost.HasValue()

        || !LessItem2Cost.HasValue()

        || !LessItem3Cost.HasValue()

        || !LessItem4Cost.HasValue()

        || !LessItem5Cost.HasValue()           

        )

        return;



    //Add up some values which are always part of the subtotal and then the ditemx ones, which will be 0 or set to a numerical value depending if the checkbox is checked

    decimal subtotals = curPurc1.GetValue() + curPurc2.GetValue() + curPurc3.GetValue() 

                      + curPurc4.GetValue() + curItem1Tot.GetValue() + curItem2Tot.GetValue() 

                      + curItem3Tot.GetValue() + curItem4Tot.GetValue() 

                      + Item1RateYN.GetOptionalValue() + Item2RateYN.GetOptionalValue() + Item3RateYN.GetOptionalValue() + Item4RateYN.GetOptionalValue();



    //Get total minus the cost of the property (curPurc1d)

    decimal plusSubTotal = subtotals - curPurc1.GetValue();





    //add up all the "less" items to know how much to reduce by 

    decimal lessTotals = LessItem1Cost.GetValue() + LessItem2Cost.GetValue() + LessItem3Cost.GetValue() + LessItem4Cost.GetValue() + LessItem5Cost.GetValue();



    //Total Balance due

    //subtotal minus the 'less' values total

    decimal total = (subtotals - lessTotals);



    //update the relevant UI field

    costPlusSubTotal.Value = plusSubTotal;

    subtotal.Value = subtotals;

    balanceDueTot.Value = total;

    totalLess.Value = lessTotals;

}

Then to make the code more C#-like, I would

• use var instead of decimal




• rename getTotals to GetTotals



• use_camelCase for private fileds (so Less becomes _less)


• reduce redundant parenthesis from (subtotals - lessTotals)
  


• use brackets {} for the return statement

Notice that I also grouped the update of the UI at the end of the method.

I would have some comments also for the name of the method itself as GetTotals implies that the method returns something, but the return signature is void. One idea is to use something like CalculateTotals.