Mortgage calculator

Question

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 a = 200000.00;
                 double r = (1+i/12);

                 DecimalFormat df = new java.text.DecimalFormat("$,###.00"); 

    //Method to calculate the payment.
    void DoWork() throws InterruptedException 
    {

                //Payment calculation 
                monthlyPayment = (float) (principal * Math.pow(1 + interestRate / period, term * period) * (interestRate / period) / (Math.pow(1 + interestRate / period, term * period) - 1));


    }//Method to calculate Amortization 

    void DoWorkAmortization() throws InterruptedException
        {



        int number = 1;
        double monthlyPayments  = a * ( r - 1 ) / ( 1 - Math.pow(r,-n));
        System.out.println ("Month \t\t Payment \t\0\0\0\0 Interest Paid \t\0\0\0\0 Principal Paid \t\0\0\0\0 Remaining Balance");
        System.out.println("__________________________________________________________________________________________________________");

        for ( number = 1; number <= 360; number++)
        {   

        double interest = a * ( i/12 );
        double principal = monthlyPayments - interest ;
        double balance = a - principal;
        a = balance;

            System.out.println ("\0\0"+ number + "\t\t" + df.format(monthlyPayments)+"\t\t" + df.format(interest)+"\t\t\t"+ df.format(principal)+ "\t\t\t" +df.format(balance));  


             if (number % 12 == 0){

                Thread.sleep(600);

                }


            }

        }
    }

Answer 1

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

2. 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.

3. 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.

4. 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.

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

6. 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.

Answer 2

1. Standard naming for Java functions uses a lower case for the first letter - for example instead of DoWorkAmortization it would be doWorkAmortization.

2. In DoWorkAmortization, the declaration of the variable number should occur at the top of the for loop:

   for (int number = 1; number <= 360; number++)
   

   rather than four statements above the loop. It's one thing to use a vague variable name as a loop index, but separating the declaration from the use for no reason only invites confusion. A more informative name wouldn't be out of line either - moNumber, or even moNr.

3. In methods that write output (such as DoWorkAmortization), it isn't a bad idea to pass in a PrintStream parameter rather than hard coding System.out.

Another 2 cents.