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

Mortgage calculator

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

Problem

I feel that my app could be more efficient but I am not sure what else I can do. I feel like I could be reusing more. If you could give me some tips on how I could improve this, I would really appreciate it.

public class Main {

   /**
    * @param args
 * @throws InterruptedException 
    */
   public static void main(String[] args) throws InterruptedException {

        //creating a new instance of my Calc class 
        Calc calc = new Calc();

       // setting the variables 
        calc.term = 30;
        calc.interestRate = (float) 0.0575;
        calc.principal = (float) 200000.00;
        calc.monthlyPayment = 0;

      //format currency correctly
            DecimalFormat df = new java.text.DecimalFormat("$,###.00");  

       //Method call to calc class
        calc.DoWork();

        //out put 
        System.out.println("\n__________________________________________________________________________________________________________");
        System.out.println("\nPrincipal Amount: " + df.format (calc.principal ));
        System.out.println("interest rate: " + calc.interestRate);
        System.out.println("Term of loan(number of years): " + calc.term);
        System.out.println("Monthly payment is: " + df.format (calc.monthlyPayment));
        System.out.println("__________________________________________________________________________________________________________\n");
        Thread.sleep(600);

        calc.DoWorkAmortization(); 

      }
   }


```
package com.mortgagecalc;

import java.text.DecimalFormat;

public class Calc {

int term; //how long the loan is
float interestRate; // loan interest rate
float principal; // loan amount
float monthlyPayment; // monthly payment
private int period = 12; // 12 months in a year

//Amortization Variables

int n = 360;
double i = 0.0575;
double

Solution

-
I believe you'd be better off with having term, interestRate,... as privates, and provide those in the Calc constructor.

-
The name DoWork does not convey the task the function performs; a better name could be CalculatePayment. Also, the "do" in "DoWork" suggests that either the function has a side effect or takes a lot of time to compute. The same for DoWorkAmortization, it prints stuff and sleeps; could be PrintAmortizations. Calc class name should also be mor
telling.

-
But the act of conveying information to the user, using a specific way (System.out) and sleeping does not belong to the Calc class. It is better to leave only the calculation responsibility to Calc and the rest to the program.

-
But while you have that function print to the user, then at least catch the InterruptedException and act on it in the function and not pass it further.

-
DoWork does not have to declare InterruptedException as it will not be thrown there.

-
Use intermediate values for subexpressions in DoWork to make the code more readable.

Those are my 2¢.

EDIT: Removed 4. and 5., as InterruptedException should be propagated in this case.

Context

StackExchange Code Review Q#9680, answer score: 11

Revisions (0)

No revisions yet.