4
\$\begingroup\$

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);

                }


            }

        }
    }
\$\endgroup\$
  • \$\begingroup\$ Note that you don't need to write (float) 0.0575, just 0.0575F. \$\endgroup\$ – Landei Mar 4 '12 at 12:54
11
\$\begingroup\$
  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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your tips :) I was having trouble with just putting the calculations for the Amortization in the Calc class... I am not sure how to pass this. Again thank you for your review. \$\endgroup\$ – Ryan Summey Mar 4 '12 at 1:51
  • \$\begingroup\$ You could create a class having getInterest(), getPrincipal() and getBalance() and return it's instances from a CalculateStateOnDay(int day). \$\endgroup\$ – Adrian Panasiuk Mar 4 '12 at 1:55
  • \$\begingroup\$ ahh so its more of a getter / setter ? can i do this with in Calc class ? Im going to rename that class btw lol \$\endgroup\$ – Ryan Summey Mar 4 '12 at 2:03
  • \$\begingroup\$ interest, principal and balance are all related to a specific day, and as such should not be inside Calc - which is not bound to a specific day. What I'm suggesting is to return them in instances of some class InterestData, which has getters for the three. Those are not really getters from the Calc class. \$\endgroup\$ – Adrian Panasiuk Mar 4 '12 at 2:07
  • \$\begingroup\$ But in this simple Calc exercise, it would be easier to have functions getInterest(day), getPrincipal(day) and getBalance(day) in Calc, so you don't have a InterestData class with all properties and no operations \$\endgroup\$ – Adrian Panasiuk Mar 4 '12 at 2:11
7
\$\begingroup\$
  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.

\$\endgroup\$

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.