5
\$\begingroup\$

This does work and serves the purpose. But, being my first code, it's sloppy and I know there are somethings I could of done to make it easier. I would like everyone's opinion on what I could do to simplify this.

This takes user input from a Visual Studio Form and outputs data after calculating things. I would like to clean this up or know different routes that may be more useful. If you could explain why it works better that's a plus because I'm trying to learn!

private void buttonPayment_Click(object sender, EventArgs e)
    {

        double sellPrice = 0;
        double stateTax = 0;
        double totalFinanced = 0;
        double amtTax = 0;
        double docFee = 179;
        double tagFees = 0;
        double tradeAllowance = 0;
        double interestRate = 0;
        double taxableAmount = 0;
        double rebate = 0;
        double cashDown = 0;
        double term = 0;
        double monthlyRate = 0;
        double monthlyPayment = 0;
        double topNumber1 = 0;
        double topNumber = 0;
        double bottomNumber = 0;
        double topNumber2 = 0;
        double totalInterest = 0;
        double interestCharge = 0;

        sellPrice = Double.Parse(textBoxSellPrice.Text);
        tradeAllowance = Double.Parse(textBoxTradeAllowance.Text);
        tagFees = Double.Parse(textBoxTagFees.Text);
        rebate = Double.Parse(textBoxRebate.Text);
        interestRate = Double.Parse(textBoxRate.Text);
        term = Double.Parse(textBoxTerm.Text);
        cashDown = Double.Parse(textBoxCashDown.Text);


        if (comboBoxState.Text.Contains("NJ"))
        {
            stateTax = .06875;
        }

        if (comboBoxState.Text.Contains("DE"))
        {
            stateTax = .0425;
        }

        if (comboBoxState.Text.Contains("PA"))
        {
            stateTax = .06;
        }

        taxableAmount = sellPrice - tradeAllowance;
        amtTax = taxableAmount * stateTax;
        totalFinanced = (amtTax + taxableAmount + docFee + tagFees - rebate - cashDown);


        labelTax.Text = amtTax.ToString(format: "0.00");
        labelAmtFinanced.Text = totalFinanced.ToString(format: "0.00");
        labelDocFee.Text = docFee.ToString(format: "0.00");
        labelTagFees.Text = tagFees.ToString(format: "0.00");

        if (interestRate == 0)
        {

            monthlyPayment = totalFinanced / term;
            labelMonthlyPayment.Text = monthlyPayment.ToString(format: "0.00");
            labelInterestCharge.Text = 0.ToString();

        }

        else
        {
            //Calculatations
            interestRate = interestRate * .01;
            monthlyRate = interestRate / 12;

            //Calculates Payments

            topNumber1 = 1 + monthlyRate;
            topNumber2 = Math.Pow(topNumber1, term);
            topNumber = monthlyRate * topNumber2;
            bottomNumber = topNumber2 - 1;
            totalInterest = topNumber / bottomNumber;

            monthlyPayment = totalFinanced * totalInterest;
            interestCharge = (monthlyPayment * term) - totalFinanced;

            // Prints to the labels

            labelMonthlyPayment.Text = monthlyPayment.ToString(format: "0.00");
            labelInterestCharge.Text = interestCharge.ToString(format: "0.00");
        }

    }

    private void buttonClear_Click(object sender, EventArgs e)
    {
        ClearAllText(this);
        ClearAllCombo(this);
        ClearAllmasked(this);
        labelTax.Text = "---";
        labelDocFee.Text = "---";
        labelTagFees.Text = "---";
        labelAmtFinanced.Text = "---";
        labelMonthlyPayment.Text = "---";
        labelInterestCharge.Text = "---";
    }
    private void ClearAllText(Control con)
    {
        foreach (Control c in this.Controls)
        {
            if (c is TextBox && c != null)
            {
                c.Text = "0";
            }
        }
    }

    private void ClearAllCombo(Control con)
    {
        foreach (Control c in con.Controls)
        {
            if (c is ComboBox && c != null)
            {
                c.Text = "NJ";
            }
        }
    }
    private void ClearAllmasked(Control con)
    {
        foreach (Control c in con.Controls)
        {
            if (c is MaskedTextBox)
                ((MaskedTextBox)c).ResetText();
            else
                ClearAllText(c);
        }
    }

Here's a small picture of the program so you can reference to what I'm working with:

enter image description here

GREAT ADVICE! Thanks a million for the advice so far! I have been putting off learning c# for so long and I'm glad to see such a welcoming community.

\$\endgroup\$
6
  • \$\begingroup\$ @StephenRauch: Unless the formatting is due to copy/paste issues, it shouldn't be changed in the question and can instead be addressed in an answer. \$\endgroup\$
    – Jamal
    Feb 14 '17 at 17:41
  • \$\begingroup\$ @Jamal from what I can see he didn't change code. Just looks like he cleaned up the format from copy/paste \$\endgroup\$ Feb 14 '17 at 17:45
  • \$\begingroup\$ It was hard to tell since I see a lot of C# code formatted that way. It's not really a big deal anyway. \$\endgroup\$
    – Jamal
    Feb 14 '17 at 17:46
  • \$\begingroup\$ Since this looks like winforms, I would first suggest you flip all those text boxes to NumericUpDown controls instead. That eliminates the need to parse values, since NumericUpDown.Value is already a decimal type. \$\endgroup\$
    – Dan Lyons
    Feb 14 '17 at 18:52
  • \$\begingroup\$ I take this is Winforms. In WPF can bind to DisplayMember adn ValueMember for a slick solution to state and tax. \$\endgroup\$
    – paparazzo
    Feb 14 '17 at 19:35
5
\$\begingroup\$

My honest opinion would be to make an interest calculator class, but we can work on a single function.

If you do not already have some input validation elsewhere, I would consider the use of Double.TryParse() to ensure that the input can be converted. This way you can handle any input conversion errors.

if(!Double.TryParse(textBoxSellPrice.Text, out sellPrice)
{
    // Error Handling
}

Can the user input text into the combobox? This presents an issue since other values are dependent on the value. I would lock that down and change the if statements to a dictionary so that it can be easier to maintain if it were to change.

Dictionary<string, double> stateTaxes = new Dictionary<string, int>()
{
    { "NJ", .06875 },
    { "DE", .0425},
    { "PA", .06 }
};

if(stateTaxes.ContainsKey(comboBoxState.Text)
{
    stateTax = stateTaxes[comboBoxState.Text];
}

Just out of my own preference, I would not declare every variable at the beginning. That wall of declarations is a bit daunting and misleading when trying to review this. I would remove any values that are not derived from the form and functionalize other other calculations with localized variables. Doing this could also remove the need for the interestRate == 0 conditional block.

totalInterest = interestRate == 0 ? 0 : topNumber / bottomNumber;

And now that you have all variables populated for all situations, I would group all of the control text assignments so that you have them all together. Since these all need the same formatting, you could make an extension for double that accepts the Label as a parameter.

private void ToLabel(this double value, Label lbl)
{
    lbl.Text = value.ToString(format: "0.00")
}

So then...

amtTax.ToLabel(labelTax);
totalFinanced.ToLabel(labelAmtFinanced);
// etc....

Here is how it could look as a class

class InterestCalculator
{
    private const double docFee = 179;

    private double sellPrice = 0;
    private double rebate= 0;
    private double cashDown = 0;
    private double tradeAllowance = 0;
    private string state;
    private double tagFees = 0;
    private double interestRate = 0;
    private double term = 0;
    private Dictionary<string, double> stateTaxes = new Dictionary<string, int>()
    {
        { "NJ", .06875 },
        { "DE", .0425},
        { "PA", .06 }
    };

    public double StateTax
    {
        get
        {
            if(stateTaxes.ContainsKey(state)
            {
                return stateTaxes[state];
            }
            return 0;
        }
    }

    public double TaxableAmount
    {
        get { return sellPrice - tradeAllowance; }
    }

    public double Tax
    {
        get { return TaxableAmount * StateTax; }
    }

    public double Deductions
    {
        get { return rebate + tradeAllowance; }
    }

    public double Fees
    {
        get { return docFee + tagFees; }
    }

    public double TotalFinanced
    {
        get { return TaxableAmount + Tax + Fees - Deductions; }
    }

    public double MonthlyRate
    {
        get { return interestRate / 12; }
    }

    public double CompoundedInterestRate
    {
        get { return Math.Pow(MonthlyRate + 1, term); }
    }

    public double TotalInterest
    {
        get 
        { 
            if(interestRate == 0)
            {
                return 0;
            }
            return ((MonthlyRate + 1) * CompoundedInterestRate) / (CompoundedInterestRate - 1);
        }
    }

    public double MonthlyPayment
    {
        get { return TotalFinanced * TotalInterest; }
    }

    public double InterestCharge
    {
        get { (MonthlyPayment * term) - TotalFinanced; }
    }

    public InterestCalculator(string state, double sellPrice, double rebate, 
        double cashDown, double tradeAllowance, double tagFees, double interestRate, double term)
    {
        this.state = state;
        this.sellPrice = sellPrice;
        this.rebate = rebate;
        this.cashDown = cashDown;
        this.tradeAllowance = tradeAllowance;
        this.tagFees = tagFees;
        this.interestRate = interestRate * .01;
        this.term = term;
    }
}

And it could be used like this

private void buttonPayment_Click(object sender, EventArgs e)
{
    // This is without validation
    double sellPrice = Double.Parse(textBoxSellPrice.Text);
    double tradeAllowance = Double.Parse(textBoxTradeAllowance.Text);
    double tagFees = Double.Parse(textBoxTagFees.Text);
    double rebate = Double.Parse(textBoxRebate.Text);
    double interestRate = Double.Parse(textBoxRate.Text);
    double term = Double.Parse(textBoxTerm.Text);
    double cashDown = Double.Parse(textBoxCashDown.Text);
    string state = comboBoxState.Text;

    InterestCalculator calculator = new InterestCalculator(state, sellPrice, tradeAllowance, tagFees, rebate, interestRate, term, cashDown);

    calculator.Tax.ToLabel(labelTax);
    calculator.TotalFinanced.ToLabel(labelAmtFinanced);
    // etc....
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ great advice! I'm working on this now! I'm not familiar with separating this into separate classes though. Still a lot to learn xD \$\endgroup\$ Feb 14 '17 at 20:18
  • 2
    \$\begingroup\$ WOW! Great edit! This is the exact thing I wanted to see. I look forward to using this. Thanks a million! \$\endgroup\$ Feb 14 '17 at 20:51
4
\$\begingroup\$

Switch Statements

You have some logic about state tax, and right now you have three cases where the value is not 0. What about if you need to edit the code to add more and more in? If you find yourself in a case where you have 3 or more if statements to set one value, then you should look at switch statements. If you only have two you could use a ternary operator. Example:

double stateTax = 0;
switch (comboBoxState.Text)
{
    case "NJ":
        stateTax = .06875;
        break;
    case "DE":
        stateTax = .0425;
        break;
    case "PA":
        stateTax = .06;
        break;
    default:
        stateTax = 0;
}

Logical grouping of mathematical operators

If we take a look at the following line: totalFinanced = (amtTax + taxableAmount + docFee + tagFees - rebate - cashDown); Whilst this has a more obvious calculation order, it isn't that obvious to every developer. When you add division and multiplication into the mix it gets really difficult to follow. There's a really simple way of doing this, use brackets:

var percentageDifference = (23 / 65) * 100;

I would also avoid the number of arguments you're calculating in one statement. You've already reduced this to some extent by introducing taxableAmount and amtTax. Can you extract any more calculations into variables that make sense?

Consistent naming of variables

You start off pretty strongly with the naming of your variables, and they all make sense to me until, and do excuse me if this makes sense from a financial point of view:

topNumber1 = 1 + monthlyRate;
topNumber2 = Math.Pow(topNumber1, term);
topNumber = monthlyRate * topNumber2;

How do topNumber, topNumber1 and topNumber2 differ? Of course they differ in their calculation, but if I'm further down in your method and I see topNumber + topNumber2, I'll have to scroll up to garner their meaning. Is there a more suitable name you can give these?

Furthermore, and this is a bit nitpickery but the variables taxableAmount and amtTax. I'm assuming you've shortened the amt in amtTax from the word amount. Firstly, I'd say be consistent, either make them both use amt or both use amount. Secondly, you have intellisense so you only have to add in the extra characters when you declare the variable and after that you're not really saving any time by shortening the word, but you might make another developer in the code feel like they need to double check that you do mean amount.

Separating your code logically

I think you need to think how you can better isolate parts of your code. Currently all of your logic is within the same button click handler.

On first glance, it looks like most of the code down to:

labelTagFees.Text = tagFees.ToString(format: "0.00");

Is based on the same group of logic. You might be able to refactor this by doing something like:

private int CalculateTotalFinanced()
{
    double stateTax = 0;
    switch (comboBoxState.Text)
    {
        case "NJ":
            stateTax = .06875;
            break;
        case "DE":
            stateTax = .0425;
            break;
        case "PA":
            stateTax = .06;
            break;
        default:
            stateTax = 0;
    }

    taxableAmount = sellPrice - tradeAllowance;
    amtTax = taxableAmount * stateTax;
    totalFinanced = (amtTax + taxableAmount + docFee + tagFees - rebate - cashDown);
}

When you can break your logic into smaller functions like this, its easier for other developers to follow when they read it. You've grouped a bunch of logic into one description there and it reduces the complexity of your main click button handler. See what else you can isolate...

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thanks for this! I hated looking at the stateTax if statements and that definitely looks a lot better! The topNumber's is for calculating the formula for interest on a car loan with a designated term. I was trying to put it all into one but for the life of me could not figure out how to consolidate it to be accurate without breaking down the formula. \$\endgroup\$ Feb 14 '17 at 19:16
  • 1
    \$\begingroup\$ now to ask further. Since this has always confused me. Can i call to a double that I used in a previous class? Cause I want to refactor this and I tried before and failed miserably. Now that this code is functioning I might try more and see how it goes. \$\endgroup\$ Feb 14 '17 at 19:18
  • 1
    \$\begingroup\$ @NickMesser no you will need to pass that reference, but that's simple enough, eg. YourNewClass.YourNewMethod(doubleYouNeedToPass) \$\endgroup\$
    – ediblecode
    Feb 14 '17 at 19:26
1
\$\begingroup\$

A couple of things:

Here:

if (comboBoxState.Text.Contains("NJ"))
{
    stateTax = .06875;
}

if (comboBoxState.Text.Contains("DE"))
{
    stateTax = .0425;
}

if (comboBoxState.Text.Contains("PA"))
{
    stateTax = .06;
}

A Dictionary<string,double>(taxes) will simplify that code to:

stateTax = taxes[comboBoxState.Text];

Instead of clearing every control individually, create a custom class(MyPanel) that inherits from a container like Panel. You populate the panel with the controls you need and their default values. Once you create a global instance(newPanel) of this container and add it to your form, you can re-initialize it by assiging the instance to a new constructor:

newPanel = new MyPanel();
\$\endgroup\$
2
  • 1
    \$\begingroup\$ thanks for the advice! I will look into this. I found the "Clear" on another forum and never really dived into how it operates. I will see what I can do here. \$\endgroup\$ Feb 14 '17 at 19:19
  • 1
    \$\begingroup\$ Make sure that if you make the dictionary you check if the key exists first. if(taxes.ContainsKey(comboBoxState.Text)){ stateTax = taxes[comboBoxState.Text];} \$\endgroup\$ Feb 14 '17 at 19:36
1
\$\begingroup\$

Here are some more tricks:

labelTax.Text = "---";
labelDocFee.Text = "---";
labelTagFees.Text = "---";
labelAmtFinanced.Text = "---";
labelMonthlyPayment.Text = "---";
labelInterestCharge.Text = "---";

You can turn these lines into:

labelTax.Text =
labelDocFee.Text =
labelTagFees.Text =
labelAmtFinanced.Text =
labelMonthlyPayment.Text =
labelInterestCharge.Text = "---";

or you can just create an extension

 public static Label ResetText(this Label label)
 {
      label.Text = "---";
      return label;
 }

and use it like:

labelTax.ResetText();

labelTax.Text = amtTax.ToString(format: "0.00");

With C# 6 you can now do:

labelTax.Text = $"{amtTax:0.00}";
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Nice tricks! Since label is a reference type, isn't the return unnecessary on ResetText()? I'm new to extension methods and I'm wondering if there is a purpose for this. \$\endgroup\$ Feb 16 '17 at 15:19
  • 2
    \$\begingroup\$ @ChandlerTurner the only purpose of the return value is to be able to chain other extensions if you had any like myLabel.ResetText().ResetColor(); etc. \$\endgroup\$
    – t3chb0t
    Feb 16 '17 at 15:26
1
\$\begingroup\$

My two cents, Regarding variables life span.

When you are declaring variables, you should to do that just before the place that the variable will be used. And try to keep at minimum the span where the variable is going to live.

For example instead of declaring this variables at the beginning of your method you should declare/instance them where you are going to use them.

Instead of:

    double topNumber1 = 0;
    double topNumber = 0;
    double bottomNumber = 0;
    double topNumber2 = 0;
    double totalInterest = 0;
    double interestCharge = 0;
    ....(many, many lines of code before variables being used)..

you should do:

    ....
        //Calculates Payments
        double topNumber1 = 1 + monthlyRate;
        double topNumber2 = Math.Pow(topNumber1, term);
        double topNumber = monthlyRate * topNumber2;
        ... etc..

Basically if variables should live as less as possible. And they should be declared in their respective scope (those variables only exists inside the if/else block).

\$\endgroup\$
1
  • 1
    \$\begingroup\$ thanks for the advice. I will remember this in the future. \$\endgroup\$ Feb 14 '17 at 21:36

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.