Any flaws on my Mortgage Calculator?

Hey there!

I’ve done the Mortgage Calculator from the Object-Oriented programming in the java course of code with Mosh.

As I did it from scratch and didn’t copy his solution, just arranged my result with his tips, I’d like you to tell me whether there’re flaws or any way to improve the following code.

Thank you!

package me.oriol;

public class Main {
    public static void main(String[] args) {

        var report = new MortgageReport();
        report.printMortgage();
        report.printPaymentSchedule();
    }
}
package me.oriol;

import java.util.Scanner;

public class Console {

    public int principal = (int) readNumber("Principal", 1000, 1_000_000);
    public float annualInterest = readNumber("Annual Interest Rate", 0.1F, 30);
    public byte years = (byte) readNumber("Period (Years)", 1, 30);

    public float readNumber(String prompt, float min, int max) {
        Scanner scanner = new Scanner(System.in);
        float value;
        while (true) {
            System.out.print(prompt + ": ");
            value = scanner.nextFloat();
            if (value >= min && value <= max)
                break;
            System.out.println("Enter a number between " + (short) min + " and " + max + ".");
        }
        return value;
    }
}
package me.oriol;

public class MortgageCalculator {

    private final Console console = new Console();
    private final byte MONTHS_IN_YEAR = 12;
    private final int principal = console.principal;
    private final float annualInterest = console.annualInterest;
    private final byte years = console.years;
    private final float monthlyInterest = getMonthlyInterest();
    private final short numberOfPayments = getNumberOfPayments();

    private float getMonthlyInterest() {
        byte PERCENT = 100;
        return annualInterest / MONTHS_IN_YEAR / PERCENT;
    }

    private short getNumberOfPayments() {
        return (short) (years * MONTHS_IN_YEAR);
    }

    public double calculateMortgage() {
        return principal
                * (monthlyInterest * Math.pow(1 + monthlyInterest, numberOfPayments)
                / (Math.pow(1 + monthlyInterest, numberOfPayments) - 1));
    }

    private double calculateBalance(short numberOfPaymentsMade) {
        return principal
                * (Math.pow(1 + monthlyInterest, numberOfPayments) - Math.pow(1 + monthlyInterest, numberOfPaymentsMade))
                / (Math.pow(1 + monthlyInterest, numberOfPayments) - 1);
    }

    public double[] getRemainingBalances() {
        var balances = new double[numberOfPayments];
        for (short month = 1; month <= balances.length; month++)
            balances[month -1] = calculateBalance(month);
        return balances;
    }
}
package me.oriol;

import java.text.NumberFormat;

public class MortgageReport {
    private final MortgageCalculator calculator = new MortgageCalculator();
    private final NumberFormat currency = NumberFormat.getCurrencyInstance();

    public void printMortgage() {
        double mortgage = calculator.calculateMortgage();
        String mortgageCurrency = currency.format(mortgage);
        System.out.println("\nMORTGAGE" + "\n--------" + "\nMonthly Payments: " + mortgageCurrency + "\n");
    }

    public void printPaymentSchedule() {
        System.out.println("PAYMENT SCHEDULE" + "\n----------------");
        for (double balance : calculator.getRemainingBalances())
            System.out.println(currency.format(balance));
        }
    }

Please contact me, do you have discord or skype?

Hey, could someone else help me out with that, please? The one who answered didnt help me…

Before I do, would you mind pasting the whole thing again in one code block like this:

```java
Your Java Code Here
```

Sure, I’ll do that in a sec, but i will paste 4 java codes as they’re 4 different classes.

done, do u think i can make any cleanups or just anything to improve this code?

Yes, there are a few things I would change.

First, you are doing too much at object construction time. You generally want to make your object construction be as simple as possible so that you can control when work is happening. One of the principal ways you are doing too much work at construction time is by initializing a ton of private fields - we should only initialize the ones we actually need and then use methods to get the rest of the stuff we need. In the places where we do need to initialize some members during construction, we should probably make that more explicit by actually having a constructor and doing it inside the constructor.

Second, there are a number of member variables that make no sense for the class’s responsibility. For example, all three of the Console classes fields make no sense at all and you immediately copy them into another class anyway. A slight variation on this could make sense if this Console class was only ever going to belong to this application and in that case you could have three additional methods called readPrincipal(), readAnnualInterestRate() and readPeriodInYears() which delegate to the readNumber method (which I believe can then be made private).

Third, you have variables that store duplicate information. For example, in MortgageCalculator, you have an annualInterest and a monthlyInterest stored in the class. I think you can completely get rid of annualInterest and just store monthlyInterest since that is what you use in all of your other calculations. If you ever needed annualInterest again, you could have a method that calculates it from monthlyInterest. Same goes for years and numberOfPayments.

Fourth, you have constants that should be static class variables rather than creating a new variable per instance (eg. MONTHS_IN_YEAR). You also declared some of these constants inside your methods (PERCENT).

Fifth, you can return directly at your break line in the readNumber method since the whole point of this method is to return the value. Then you can move the float value inside the while loop so that the variable is declared right where it is used.

There are also some structural things about what classes should be responsible for what tasks. Does it make sense for the calculator to have a console variable or should we be passing the principal, interest and period into the constructor and just using them in our MortageCalculator class? Maybe the console should be a member variable of the MortgageReport class instead.

Hope that helps!

1 Like

About the first point. You mean specifically on the MortgageCalculator class, right? Or is that problem in other classes?

Also, with a constructor I force myself to use parameters in the future, isn’t that bad?

Edit: what’s the issue about having too many fields?

I’m going step by step… about the first point, would that be a good fix or I should add a constructor? If so, what should this constructor initialize?

package me.oriol;

public class MortgageCalculator {

    private final Console console = new Console();
    private final byte MONTHS_IN_YEAR = 12;

    private float getMonthlyInterest() {
        byte PERCENT = 100;
        return console.annualInterest / MONTHS_IN_YEAR / PERCENT;
    }

    private short getNumberOfPayments() {
        return (short) (console.years * MONTHS_IN_YEAR);
    }

    public double calculateMortgage() {
        return console.principal
                * (getMonthlyInterest() * Math.pow(1 + getMonthlyInterest(), getNumberOfPayments())
                / (Math.pow(1 + getMonthlyInterest(), getNumberOfPayments()) - 1));
    }

    private double calculateBalance(short numberOfPaymentsMade) {
        return console.principal
                * (Math.pow(1 + getMonthlyInterest(), getNumberOfPayments())
                - Math.pow(1 + getMonthlyInterest(), numberOfPaymentsMade))
                / (Math.pow(1 + getMonthlyInterest(), getNumberOfPayments()) - 1);
    }

    public double[] getRemainingBalances() {
        var balances = new double[getNumberOfPayments()];
        for (short month = 1; month <= balances.length; month++)
            balances[month -1] = calculateBalance(month);
        return balances;
    }
}

Okay, here I go:

1- I already asked ya about the first point.

2- I think I already did that on the version I just sent you of the MortgageCalculator, am I right?

3- I think that’s done on the new version as well, confirm me please.

4- I think MONTHS_IN_YEAR is only used in MortgageCalculator and PERCENT is only used on that method… why would I declare them public for all classes?

5- Got it, done!

6- I think the console is already a member variable of MortgageCalculator in the new version, correct me if I’m wrong…

Thanks for everything btw, although I still need to clarify a few things that hopefully you can answer me as soon as you can.

Okay, sorry for message spamming xd… You can ignore all what I sent before if you want.

Now, I have edited the whole code with the advice you’ve given to me. I’d like to know whether the corrections I did are well done or not and if you can correct me more things… I’m sure I missed something about all the advice you gave me so please, if you can, I’d be greatful if you revised again the following code:

package me.oriol;

public class Main {
    public static void main(String[] args) {

        var report = new MortgageReport();
        report.printMortgage();
        report.printPaymentSchedule();
    }
}
package me.oriol;

import java.util.Scanner;

public class Console {

    private final byte MONTHS_IN_YEAR = 12;
    private final byte PERCENT = 100;

    private float readNumber(String prompt, float min, int max) {
        var scanner = new Scanner(System.in);
        while (true) {
            float value;

            System.out.print(prompt + ": ");
            value = scanner.nextFloat();
            if (value >= min && value <= max)
                return value;
            System.out.println("Enter a number between " + (short) min + " and " + max + ".");
        }
    }
    public int principal = (int)
            readNumber("Principal", 1000, 1_000_000);
    public float monthlyInterest =
            readNumber("Annual Interest Rate", 0.1F, 30) / MONTHS_IN_YEAR / PERCENT;
    public int numberOfPayments = (short)
            readNumber("Period (Years)", 1, 30) * MONTHS_IN_YEAR;
}
package me.oriol;

public class MortgageCalculator {

    private final Console console = new Console();

    public double calculateMortgage() {
        return console.principal
                * console.monthlyInterest * Math.pow(1 + console.monthlyInterest, console.numberOfPayments)
                / (Math.pow(1 + console.monthlyInterest, console.numberOfPayments) - 1);
    }

    private double calculateBalance(short numberOfPaymentsMade) {
        return console.principal
                * (Math.pow(1 + console.monthlyInterest, console.numberOfPayments)
                - Math.pow(1 + console.monthlyInterest, numberOfPaymentsMade))
                / (Math.pow(1 + console.monthlyInterest, console.numberOfPayments) - 1);
    }

    public double[] getRemainingBalances() {
        var balances = new double[console.numberOfPayments];
        for (short month = 1; month <= balances.length; month++)
            balances[month -1] = calculateBalance(month);
        return balances;
    }
}
package me.oriol;

import java.text.NumberFormat;

public class MortgageReport {
    private final MortgageCalculator calculate = new MortgageCalculator();
    private final NumberFormat currency = NumberFormat.getCurrencyInstance();

    public void printMortgage() {
        double mortgage = calculate.calculateMortgage();
        String mortgageCurrency = currency.format(mortgage);
        System.out.println("\nMORTGAGE" + "\n--------" + "\nMonthly Payments: " + mortgageCurrency + "\n");
    }

    public void printPaymentSchedule() {
        System.out.println("PAYMENT SCHEDULE" + "\n----------------");
        for (double balance : calculate.getRemainingBalances())
            System.out.println(currency.format(balance));
        }
    }

Again, thanks for everything man, you’re helping me a lot!

Sorry to take so long responding again (gotta do my day job). I’ll try to respond to everything in order as best as I can.

It is something you also did in the Console class. It makes the most sense for you to be able to initialize the Console class without actually triggering it to read anything from the console (yet). Only the MortgageReport managed to avoid this issue.


Setting things up in a constructor is not an issue and actually can help setup dependency injection to make your classes easier to test (where they have dependencies that you need to construct).


There is no real issue about having too many fields, but more about having fields that duplicate information as well as having fields that simply do not belong in the class because it violates the Single Responsibility Principle. For example, if the MortgageCalculator is only responsible for calculating the mortgage, why does that need a Console object?


Not exactly, you still have a Console inside the MortgageCalculator which (as I just said above) violates the SRP. I would instead pass the principal, interest rate, and period into the constructor for the MortgageCalculator like this:

public class MortgageCalculator {
  private static final int MONTH_PER_YEAR = 12;

  private final double principal;
  private final double monthlyInterest;
  private final int numberOfPayments;

  public MortgageCalculator(
    double principal, double annualInterest, int years) {
    this.principal = principal;
    this.monthlyInterest = annualInterest / MONTH_PER_YEAR;
    this.numberOfPayments = years * MONTHS_PER_YEAR;
  }
  // rest of the class
}

I think so, but I’ll double check in your latest iteration.


I did not say to make them public, just to store them as class variables rather than member variables so that there is exactly one copy for the class rather than one copy for each instance. For example, with MONTHS_IN_YEAR, with your existing setup this would allocate memory twice for the MONTHS_IN_YEAR whereas making it static means it is only allocated once:

var first = new MortgageCalculator(); // first.MONTHS_IN_YEAR allocated
var second = new MortgageCalculator(); // second.MONTHS_IN_YEAR allocated

If it were a static variable, it would be accessed from the class (MortgageCalculator.MONTHS_IN_YEAR) so that you do not allocate a new one every time you create a new instance of the class. As a general rule, any constant should basically be private static final in the class it is needed in. For the less common case where the constant is needed across multiple classes, you can make it public static final (but only expose it if needed).


These should both be static (as I mentioned above) since they are constants.

These should be converted to member functions so that you can control when they are read:

    public int readPrincipal() { return ... }
    // This one could also be readAnnualInterest() to be simpler
    // (since that's what you actually prompt for)
    public double readMonthlyInterest() { return ... }
    // This one could also be readPeriodInYears() for the same reason
    public int readNumberOfPayments() { return ... }

I also mentioned that the Console probably belongs in the MortgageReport class instead and then passing the stuff that MortgageCalculator needs through its constructor. Alternatively, you could put it into another class that coordinates reading from the console and then generating the report.


Since this is starting to get somewhat complicated in our discussions, would you rather move this to GitHub so that I can comment on individual sections of your code and then we can have the commentary be in-line with the code under discussion? I’d also be able to see your updates without you having to paste a ton of code over here on every iteration.

1 Like

sure, my GitHub is Oriol. But idk how I can start a topic in there, i have never used it…

1 Like

Well, coz you need to get the monthlyInterest, principal and period values from there, right?

Just annotating this topic to mention that we followed up directly with each other and were able to come up with a reasonable refactoring of the code.