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

App that calculates interest into a monthly payment

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

Problem

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 + tag

Solution

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 stateTaxes = new Dictionary()
{
    { "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 stateTaxes = new Dictionary()
    {
        { "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....
}

Code Snippets

if(!Double.TryParse(textBoxSellPrice.Text, out sellPrice)
{
    // Error Handling
}
Dictionary<string, double> stateTaxes = new Dictionary<string, int>()
{
    { "NJ", .06875 },
    { "DE", .0425},
    { "PA", .06 }
};

if(stateTaxes.ContainsKey(comboBoxState.Text)
{
    stateTax = stateTaxes[comboBoxState.Text];
}
totalInterest = interestRate == 0 ? 0 : topNumber / bottomNumber;
private void ToLabel(this double value, Label lbl)
{
    lbl.Text = value.ToString(format: "0.00")
}
amtTax.ToLabel(labelTax);
totalFinanced.ToLabel(labelAmtFinanced);
// etc....

Context

StackExchange Code Review Q#155349, answer score: 5

Revisions (0)

No revisions yet.