2
\$\begingroup\$

I'm currently learning Java at home with YouTube tutorials. I took a short break from the course to make a small project for calculating costs of loan.

It's not economically accurate - I followed basic logic and not exact mathematical formulas.

I started 1-2 weeks ago but Im learning mainly in the evenings so theres not much experience.

I'm going back to the tutorials after this little experiment/break but I wanted to see if I can get something of my own working.

It consists of two classes - Main and Calculator. Did I separate classes correctly or should it be rearranged?

Other Rookie mistakes?

General Review?

Main

package MainPackage;
import java.util.Scanner;
public class InterestAndDiscount {

    public static double capital;
    public static double interest;
    public static double inflation;
    public static double duration;

    
    public static void main(String[] args) {
        
    System.out.println("Provide the data for calculations:");
    Scanner scanner = new Scanner(System.in);
    System.out.println("What is the starting capital: ");
    capital = scanner.nextDouble();
    System.out.println("What is the rate of interest in %: ");
    interest = scanner.nextDouble();
    System.out.println("What is the inflation rate in %: ");
    inflation = scanner.nextDouble();
    System.out.println("How long is the duration in years: ");
    duration = scanner.nextDouble();
    Calculator calculator1 = new Calculator(capital, interest, inflation, duration);
    
    calculator1.creditPayments();
    calculator1.capitalIncrease();
    calculator1.capitalDecrease();
    calculator1.getInfo();

    
    scanner.close();
        
    }
    

    }
    

Calculator

    package MainPackage;

public class Calculator {
    double capital;
    double interest;
    double duration;
    double inflation;
    double sumOfPayments=0;
    double newValue=0;
    double newCashValue=0;
    
    Calculator(double capital, double interest, double inflation, double duration){
        this.capital=capital;
        this.interest=interest;
        this.inflation=inflation;
        this.duration=duration;

    }
    void capitalIncrease() {
        double capital=this.capital;
        double inflation=this.inflation;
        double duration=this.duration;
        for(int i=0; i<duration;i++) {
            capital=capital*((100+inflation)/100);
            System.out.printf("%d year: %,.2f\n",i+1, capital);
        }
        this.newValue=capital;
        
    }
    
    void capitalDecrease() {
        double capital=this.sumOfPayments;
        double inflation=this.inflation;
        double duration=this.duration;
        for(int i=0; i<duration;i++) {
            capital=capital*((100-inflation)/100);
            System.out.printf("%d year: %,.2f\n",i+1, capital);
        }
        newCashValue=capital;
    
    }
    
    void creditPayments() {
        double capital=this.capital;
        double interest=this.interest/12;
        double duration=this.duration*12;
        double amountPaid=0;
        double monthlyPayment;
        double monthlyInterest;
        
        for(int i=0; i<duration;i++) {
            monthlyInterest=(capital*((100+interest)/100)-capital);
            capital=capital+monthlyInterest;
            if(i>duration-2) {
                monthlyPayment =(capital/(duration-i))+(monthlyInterest-(monthlyInterest/(duration/(i+1))));    //added this to avoid overpayment for last month    
            } else {
                monthlyPayment=(capital/(duration-i))+(monthlyInterest);
            }
            capital=capital-monthlyPayment;
            amountPaid=amountPaid+monthlyPayment;


            System.out.printf("monthly Interest: %,.2f\n",monthlyInterest);
            System.out.printf("monthly Payment: %,.2f\n",monthlyPayment);
            System.out.printf("%d month left capital: %,.2f\n",i+1, capital);
        }

        this.sumOfPayments=amountPaid;
    }
    
    void getInfo() {
        System.out.printf("Overall paid amount: %,.2f\n",this.sumOfPayments);
        System.out.printf("Overall paid interest: %,.2f\n",this.sumOfPayments-this.capital);
        System.out.printf("Property value increase: %,.2f\n",this.newValue-this.capital);
        System.out.printf("Property value after %.0f years %,.2f \n",this.duration, this.newValue);
        System.out.printf("Sum of Payments will be worhth: %,.2f in today`s money\n",this.newCashValue);
    }
    

    
}

\$\endgroup\$
7
  • 1
    \$\begingroup\$ I'm not really an expert on Java - but as a general principle, you want to have a really good reason for using global variables. Is "public static" in Java mutable? If so, then you should really avoid it. Since you only seem to be using these from inside your main method, why not make them local variables in there? \$\endgroup\$ May 6 at 6:44
  • 1
    \$\begingroup\$ Also, comparing your "main" method and your "getInfo" - the latter is much easier to read because you don't intermingle your computations with those "printf" statements. In your "main" method, I would do those 4 computations first - possibly storing their result in some structure / class - then insert a blank line and then your print statements. You already used descriptive variable names, which is great, and separating the computations from the output will not only make this more readable, but also help you in case you later want to refactor it to return results without printing them. \$\endgroup\$ May 6 at 6:51
  • 1
    \$\begingroup\$ In your "Calculator" class, it is generally a good idea to sepatate inputs from data. What I mean by that is that any variables that are only assigned by your class constructor should be marked "readonly" - to make it clear that it is an input to your class and not something that your class computed. \$\endgroup\$ May 6 at 7:00
  • 1
    \$\begingroup\$ This is also one of these things where I am not sure how it is generally handled in the Java world - I'm coming from a C / C++ / C# background - but as a general principle, you should absolutely avoid any "surprising side-effects"! Lexical scope is a computer thing - is not really a human thing. \$\endgroup\$ May 6 at 7:05
  • 2
    \$\begingroup\$ @MartinBaulig six long comments merit an answer post \$\endgroup\$ May 8 at 5:36

1 Answer 1

2
\$\begingroup\$

First of all, I'm coming from a C / C++ / C# and Haskell background and am not too familiar with Java.

However, the main focus of this review are general programming techniques that will help you write robust code that's easily understandable by your peers.

Mutable Global Variables

Making a field public static means that code outside your class can access it, so it essentially becomes a global variable. Global variables should never be mutable - use final to prevent code outside of your class from modifying them.

Howerver, since you only seem to be using these from inside your main method, why not make them local variables in there?


Separate computations from output

Also, comparing your main method and your getInfo - the latter is much easier to read because you don't intermingle your computations with those printf statements.

In your main method, I would do those 4 computations first - possibly storing their result in some structure / class - then insert a blank line and then your print statements. You already used descriptive variable names, which is great, and separating the computations from the output will not only make this more readable, but also help you in case you later want to refactor it to return results without printing them.

/Edit: when I first commented, I didn't realize that scanner.nextDouble() was a Java utility function that reads from standard input.

I would move that code into a separate method and insert some blank lines to make it a bit more readable.

    Scanner scanner = new Scanner(System.in);

    System.out.println("Provide the data for calculations:");
    System.out.println("What is the starting capital: ");
    capital = scanner.nextDouble();

    System.out.println("What is the rate of interest in %: ");
    interest = scanner.nextDouble();

    System.out.println("What is the inflation rate in %: ");
    inflation = scanner.nextDouble();

    System.out.println("How long is the duration in years: ");
    duration = scanner.nextDouble();

Put the above into a separate method and return these 4 values in a custom data type.

This will make your code much more composable because you can then easily swap out the interactive console input with something else - for instance when writing tests.


In your Calculator class, it is generally a good idea to sepatate inputs from data. What I mean by that is that any variables that are only assigned by your class constructor should be marked final - to make it clear that it is an input to your class and not something that your class computed.

Even just separating them with a blank line and marking the inputs final already makes it a bit easier to read:

public class Calculator {
    // Inputs
    final double capital;
    final double interest;
    final double duration;
    final double inflation;

    // Computed
    double sumOfPayments=0;
    double newValue=0;
    double newCashValue=0;

Surprising Side-Effects

To make it easy for others to read and understand your code, never use two variables with the exact same same, but completely different meaning in different lexical scopes.

A computer program will understand that interest and this.interest are two distinct variables - but a human who has seen your outer definition may not realize that you're re-defining it it mean something completely different in an inner lexical scope.

This is also something that will really hurt you later on - imagine for instance that at some point, you need to add additional methods to your classes and also want to collaborate with other people on this. Well, it should be possible for somebody to just look at your class definition and one of it's methods to understand what that method does. For this to work, your other methods must not have any surprising side-effects - such as modifying class fields that look like read-only inputs.


Avoid copying class fiels into local variables:

void capitalIncrease() {
        double capital=this.capital;
        double inflation=this.inflation;
        double duration=this.duration;

There is no need for doing so - it only leads to the following anti-pattern:

            capital=capital*((100+inflation)/100);

As I explained above, this is a surprising side-effect as it is not immediately obvious to a reader, that capital no longer refers to the class field.


It is even worse in the creditPayments method, because first you have these assignments:

void creditPayments() {
        double capital=this.capital;
        double interest=this.interest/12;
        double duration=this.duration*12;

and then this code in a loop:

monthlyInterest=(capital*((100+interest)/100) - capital);
capital=capital+monthlyInterest;
if (i > duration-2) {
    monthlyPayment = (capital/(duration-i)) + (monthlyInterest - (monthlyInterest/(duration/(i+1))));    //added this to avoid overpayment for last month    
} else {
    monthlyPayment = (capital/(duration-i)) + (monthlyInterest);
}
capital = capital - monthlyPayment;
amountPaid = amountPaid + monthlyPayment;

A person reading this is likely going to focus on these computations to understand what your code does - the hard math behind them, and not so much the variable declarations at the beginning of the method.


"Time-Scoped" Data

In this particular algorithm, you have several parameters that are "scoped" to a particular time interval - for instance "interest" could be a monthly or yearly value.

You already recognized this in the "math" loop above, where you're using variables such as monthlyPayment. This is really great and shows an understanding of the fundamental concept.

Just try to do it consistently over that code.

You're only making it much harder on yourself when you have a yearly interest called interest, then divide that by 12 and have a variable called monthlyInterest in that same function that's used for something else.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.