Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Improve types in Mortgage and FairholdLandPurchase #23

Merged
merged 2 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app/models/Mortgage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ it("can be instantiated", () => {
const mortgage = new Mortgage({
propertyValue: 100,
interestRate: 0.05,
termOfTheMortgage: 25,
mortgageTerm: 25,
initialDeposit: 0.1,
});
expect(mortgage).toBeDefined();
Expand All @@ -14,7 +14,7 @@ it("correctly calculates the amount of the mortgage ", () => {
const mortgage = new Mortgage({
propertyValue: 100,
interestRate: 0.05,
termOfTheMortgage: 25,
mortgageTerm: 25,
initialDeposit: 0.1,
});

Expand All @@ -25,7 +25,7 @@ it("correctly calculates the amount of monthly payment ", () => {
const mortgage = new Mortgage({
propertyValue: 100,
interestRate: 0.05,
termOfTheMortgage: 25,
mortgageTerm: 25,
initialDeposit: 0.1,
});
expect(mortgage.monthlyPayment).toBeCloseTo(0.53);
Expand Down
157 changes: 88 additions & 69 deletions app/models/Mortgage.ts
Original file line number Diff line number Diff line change
@@ -1,95 +1,114 @@
import { MONTHS_PER_YEAR } from "./constants";

const DEFAULT_INTEREST_RATE = 0.06;
const DEFAULT_MORTGAGE_TERM = 30;
const DEFAULT_INITIAL_DEPOSIT = 0.15;
Comment on lines +3 to +5
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining constants like this make it clearer that these are constants and not magic numbers, it should also make them easier to find and update 👍


interface MortgageParams {
propertyValue: number;
interestRate?: number;
mortgageTerm?: number;
initialDeposit?: number;
}
Comment on lines +7 to +12
Copy link
Contributor Author

@DafyddLlyr DafyddLlyr Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting up a type for the constructor params allows us to avoid a fair amount of type repetition / boilerplate. Still a bit verbose but a little better I think?

Likely required until a solution for microsoft/TypeScript#5326 is in place.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I don't quite understand the ? in the interface. I thought that all these parameters need to exist (can't be undefined) to instantiate the mortgage class?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a much nice constructor, thanks for suggesting it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I can explain this!

These variables are genuinely optional in the constructor. If I don't provide them, then we'll fall back to the default values within the constructor (e.g. this.initialDeposit = params.initialDeposit || DEFAULT_INITIAL_DEPOSIT).

This means we can easily construct a Mortgage instance with only the required variable propertyValue, and always have a class which has the required properties initialDepost, interestRate and termYears.


type MortgageBreakdown = {
yearlyPayment: number;
cumulativePaid: number;
remainingBalance: number;
}[];
Comment on lines +14 to +18
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Types and interfaces can be defined once, applied to their respective classes, and then cascade down to where needed.


export class Mortgage {
propertyValue: number; //value of the property for the mortgage
interestRate: number; // interest rate of the mortgage in percentage e.r, 0.05=5%
termYears: number; // number of years of the mortgage
initialDeposit: number; // initial deposit of the value of the mortgage in percentage e.g. 0.15 =15% deposit
principal?: number; // amount of the morgage requested
monthlyPayment?: number; // monthly rate of the mortgage
totalMortgageCost?: number; // total cost of the mortgage
yearlyPaymentBreakdown?: {
yearlyPayment: number;
cumulativePaid: number;
remainingBalance: number;
}[]; // yearly breakdown of the mortgage
constructor({
propertyValue,
interestRate = 0.06,
termOfTheMortgage = 30,
initialDeposit = 0.15,
}: {
propertyValue: number;
interestRate?: number;
termOfTheMortgage?: number;
initialDeposit?: number;
}) {
this.propertyValue = propertyValue;
this.initialDeposit = initialDeposit;
this.interestRate = interestRate;
this.termYears = termOfTheMortgage;
this.calculateAmountOfTheMortgage(); // calculate the amount of the mortgage
this.calculateMonthlyMortgagePayment(); // calculate the montly payment
this.calculateYearlyPaymentBreakdown(); // calculate the yearly breakdown;
}
propertyValue: number;
/**
* This value is given as a percentage. For example, 0.05 represents a 5% rate
*/
Comment on lines +23 to +24
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using JSDoc for meaningful comments leads to better DX as these are then accessible through most IDEs -

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's phenomenal! Super useful!

interestRate: number;
termYears: number;
/**
* This value is given as a percentage. For example, 0.15 represents a 15% deposit
*/
initialDeposit: number;
/**
* The principle is the value of the property, minus the deposit
*/
principal: number;
monthlyPayment: number;
totalMortgageCost: number;
yearlyPaymentBreakdown: MortgageBreakdown;

constructor(params: MortgageParams) {
this.propertyValue = params.propertyValue;
this.initialDeposit = params.initialDeposit || DEFAULT_INITIAL_DEPOSIT;
this.interestRate = params.interestRate || DEFAULT_INTEREST_RATE;
this.termYears = params.mortgageTerm || DEFAULT_MORTGAGE_TERM;

// Computed properties, order is significant
this.principal = this.calculateMortgagePrinciple();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I was forgetting from our call earlier - as opposed to marking properties with ! which isn't ideal, as long as we define them in the constructor they'll get picked up as required properties.


const { monthlyPayment, totalMortgageCost } =
this.calculateMonthlyMortgagePayment();
this.monthlyPayment = monthlyPayment;
this.totalMortgageCost = totalMortgageCost;

calculateAmountOfTheMortgage() {
this.principal = this.propertyValue * (1 - this.initialDeposit); // calculate the amount of the mortgage by removing the deposit
return this.principal;
this.yearlyPaymentBreakdown = this.calculateYearlyPaymentBreakdown();
}

calculateMonthlyMortgagePayment() {
const monthlyInterestRate = this.interestRate / 12; // Convert annual interest rate to monthly rate
const numberOfPayments = this.termYears * 12; // Convert term in years to total number of payments
if (this.principal !== undefined) {
const monthlyPayment =
(this.principal *
monthlyInterestRate *
Math.pow(1 + monthlyInterestRate, numberOfPayments)) /
(Math.pow(1 + monthlyInterestRate, numberOfPayments) - 1); // Calculate the monthly payment
this.monthlyPayment = parseFloat(monthlyPayment.toFixed(2)); // Store monthly payment rounded to 2 decimal places in class property
this.totalMortgageCost = this.monthlyPayment * numberOfPayments; // total cost of the mortgage
return this.monthlyPayment;
} else {
throw new Error("amountOfTheMortgage is undefined");
}
private calculateMortgagePrinciple() {
const principal = this.propertyValue * (1 - this.initialDeposit);
return principal;
}
calculateYearlyPaymentBreakdown() {
if (this.monthlyPayment == undefined || this.totalMortgageCost == undefined)
throw new Error("monthlyPayment or totalMortgageCost is undefined");

private calculateMonthlyMortgagePayment() {
const monthlyInterestRate = this.interestRate / MONTHS_PER_YEAR;
const numberOfPayments = this.termYears * MONTHS_PER_YEAR;

let monthlyPayment =
(this.principal *
monthlyInterestRate *
Math.pow(1 + monthlyInterestRate, numberOfPayments)) /
(Math.pow(1 + monthlyInterestRate, numberOfPayments) - 1);
monthlyPayment = parseFloat(monthlyPayment.toFixed(2));

const totalMortgageCost = monthlyPayment * numberOfPayments;

return { monthlyPayment, totalMortgageCost };
}
private calculateYearlyPaymentBreakdown() {
let yearlyPayment =
this.initialDeposit * this.propertyValue + this.monthlyPayment * 12;
this.initialDeposit * this.propertyValue +
this.monthlyPayment * MONTHS_PER_YEAR;
let cumulativePaid =
this.initialDeposit * this.propertyValue + this.monthlyPayment * 12;
let remainingBalance = this.totalMortgageCost - this.monthlyPayment * 12;
this.initialDeposit * this.propertyValue +
this.monthlyPayment * MONTHS_PER_YEAR;
let remainingBalance =
this.totalMortgageCost - this.monthlyPayment * MONTHS_PER_YEAR;

interface mortgageBreakdownTypes {
yearlyPayment: number;
cumulativePaid: number;
remainingBalance: number;
}
let yearlyPaymentBreakdown: mortgageBreakdownTypes[] = [
let yearlyPaymentBreakdown: MortgageBreakdown = [
{
yearlyPayment: yearlyPayment,
cumulativePaid: cumulativePaid,
remainingBalance: remainingBalance,
},
]; // initialize the yearlyPaymentBreakdown
];

for (let i = 0; i < this.termYears - 1; i++) {
if (i != this.termYears - 1) {
yearlyPayment = this.monthlyPayment * 12; // calculate the yearly payment
yearlyPayment = this.monthlyPayment * MONTHS_PER_YEAR;
} else {
yearlyPayment = remainingBalance; // last year just pay the remaining balance
// last year just pay the remaining balance
yearlyPayment = remainingBalance;
}

cumulativePaid = cumulativePaid + yearlyPayment; // calculate the updated cumulative paid
remainingBalance = remainingBalance - yearlyPayment; // calculate the updated remaining balance
cumulativePaid = cumulativePaid + yearlyPayment;
remainingBalance = remainingBalance - yearlyPayment;

yearlyPaymentBreakdown.push({
yearlyPayment: yearlyPayment,
cumulativePaid: cumulativePaid,
remainingBalance: remainingBalance,
}); // add the current yearly payment to the yearlyPaymentBreakdown
});
}
this.yearlyPaymentBreakdown = yearlyPaymentBreakdown; // set the yearlyPaymentBreakdown

return yearlyPaymentBreakdown;
}
}
}
1 change: 1 addition & 0 deletions app/models/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const MONTHS_PER_YEAR = 12;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is a bit redundant, discuss! For me it's a little easier to read and reason with than a mixture of numbers and variable names. If this isn't helpful we can revert to 12.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense! I imagine that saving this as a constant might help contextualise a * or / 12 more quickly. One less jump to make while reading over some code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - I think that parsing word * word is generally easier than word * number 👍

Loading