feat: Improve types in `Mortgage` and `FairholdLandPurchase` #23

DafyddLlyr · 2024-07-26T20:39:22Z

What does this PR do

A pass through Mortgage and FairholdLandPurchase classes with the aim of removing properties marked as optional (?) which are actually always present.
Improvements to types
No logical changes, just tinkering around the edges!
I'll add comments alongside the code explaining my thoughts behind any changes

vercel · 2024-07-26T20:39:25Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
fairhold-dashboard	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 2, 2024 7:06am

DafyddLlyr · 2024-07-26T20:40:49Z

app/models/Mortgage.ts

+const DEFAULT_INTEREST_RATE = 0.06;
+const DEFAULT_MORTGAGE_TERM = 30;
+const DEFAULT_INITIAL_DEPOSIT = 0.15;


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 👍

DafyddLlyr · 2024-07-26T20:41:28Z

app/models/Mortgage.ts

+type MortgageBreakdown = {
+  yearlyPayment: number;
+  cumulativePaid: number;
+  remainingBalance: number;
+}[];


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

DafyddLlyr · 2024-07-26T20:43:37Z

app/models/Mortgage.ts

+   * This value is given as a percentage. For example, 0.05 represents a 5% rate
+   */


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

That's phenomenal! Super useful!

DafyddLlyr · 2024-07-26T20:45:02Z

app/models/Mortgage.ts

-    this.calculateMonthlyMortgagePayment(); // calculate the montly payment
-    this.calculateYearlyPaymentBreakdown(); // calculate the yearly breakdown;
+    this.termYears = mortgageTerm;
+    this.principal = this.calculateMortgagePrinciple();


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.

DafyddLlyr · 2024-07-26T20:46:07Z

app/models/constants.ts

@@ -0,0 +1 @@
+export const MONTHS_PER_YEAR = 12;


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.

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?

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

DafyddLlyr · 2024-07-26T20:48:15Z

app/models/tenure/FairholdLandPurchase.ts

-    newBuildPrice, // new build price of the property
-    depreciatedBuildPrice, // depreciated building price
-    constructionPriceGrowthPerYear, // construction price growth per year
-    yearsForecast, // years forecast
-    maintenanceCostPercentage, // maintenance cost percentage


There are a lot of similar comments removed in this PR - mainly as they add a bit of noise without much value, they're often a rehash of the variable name.

A good aim with comments is to try to explain "why", not "what". Hopefully the code and variable names should cover the "what" 👍

DafyddLlyr · 2024-07-28T10:54:20Z

app/models/Mortgage.ts

+interface MortgageParams {
+  propertyValue: number;
+  interestRate?: number;
+  mortgageTerm?: number;
+  initialDeposit?: number;
+}


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.

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?

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

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.

zz-hh-aa · 2024-07-29T08:39:52Z

app/models/tenure/FairholdLandPurchase.ts

-    yearsForecast: number,
-    constructionPriceGrowthPerYear: number
-  ) {
+  private calculateLifetime({


I don't quite understand why private is used here--I get that it means its properties aren't accessible elsewhere, but why is that useful? Is that to prevent them from being modified or something like that?

Yep happy to explain this!

I'm using private here just to annotate how the class is currently being used in the codebase. These functions are just internal to the class and being used to construct it. They don't have a use case (currently) outside the scope of the constructor.

This just means that when we're using an instance of the Mortgage class downstream somewhere it won't be possible to call this function directly (mortgage.calculateLifetime(), but we will be able to access the property mortgage.lifetime.

gabrielegranello · 2024-07-29T09:41:40Z

app/models/Property.ts

+/**
+ * Number of decimal places to use when rounding numerical values
+ */
+const PRECISION = 2


Should these be moved into constants.ts ?

Good question! If we're consistently using two decimal places across the codebase as a standard, yes we should move it. If this is particular to this class, it can stay here.

gabrielegranello · 2024-07-29T09:55:10Z

app/models/Property.ts

-      this.landToTotalRatio = this.landPrice / this.bedWeightedAveragePrice; // define the land to total ratio
-    }
-  }
+// TODO: Reuse HouseType enum


Let me see if got the logic right here. Since you previously defined the calculationSchema, can we add something like this?

import { calculationSchema } from './calculationSchema'; type HouseType = typeof calculationSchema.shape.houseType._type;

Not sure of the exact syntax, but that's essentially the idea yes. We would like to define the enum once, so that we don't need to try and synchronise various variables and make sure they always match.

We might define this in Zod first (as I did in another PR), we we might define it here in the class and pull this into our Zod validation schema.

Once both PRs are merged I'll come back and pick up this TODO 👍

zz-hh-aa

Thanks for the super clear explanations! Changes make a lot of sense and really tidy things up.

DafyddLlyr commented Jul 26, 2024

View reviewed changes

DafyddLlyr force-pushed the dp/tidy-up-mortgage-and-fairholdlandpurchase branch from 441c868 to 4798881 Compare July 26, 2024 20:50

vercel bot deployed to Preview July 26, 2024 20:51 View deployment

DafyddLlyr mentioned this pull request Jul 26, 2024

feat: Tighten types on Property class #24

Merged

DafyddLlyr force-pushed the dp/tidy-up-mortgage-and-fairholdlandpurchase branch from 4798881 to 15b606c Compare July 28, 2024 10:50

vercel bot deployed to Preview July 28, 2024 10:51 View deployment

DafyddLlyr commented Jul 28, 2024

View reviewed changes

zz-hh-aa reviewed Jul 29, 2024

View reviewed changes

gabrielegranello reviewed Jul 29, 2024

View reviewed changes

Base automatically changed from dp/split-up-models to main July 29, 2024 10:02

DafyddLlyr force-pushed the dp/tidy-up-mortgage-and-fairholdlandpurchase branch from 15b606c to 82705f9 Compare July 29, 2024 10:04

vercel bot deployed to Preview July 29, 2024 10:05 View deployment

This was referenced Jul 29, 2024

feat: tidy up types on FairholdLandRent #27

Merged

Tidy up of the SocialRent class #28

Merged

zz-hh-aa approved these changes Jul 31, 2024

View reviewed changes

vercel bot deployed to Preview July 31, 2024 11:02 View deployment

vercel bot deployed to Preview August 2, 2024 07:01 View deployment

DafyddLlyr added 2 commits August 2, 2024 08:04

feat: Improve types in Mortgage and FairholdLandPurchase

8bede03

feat: Setup constructor param types

5386b54

DafyddLlyr force-pushed the dp/tidy-up-mortgage-and-fairholdlandpurchase branch from 57ee5da to 5386b54 Compare August 2, 2024 07:05

vercel bot deployed to Preview August 2, 2024 07:06 View deployment

DafyddLlyr merged commit b0a4ef4 into main Aug 2, 2024
2 checks passed

DafyddLlyr deleted the dp/tidy-up-mortgage-and-fairholdlandpurchase branch August 2, 2024 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Improve types in `Mortgage` and `FairholdLandPurchase` #23

feat: Improve types in `Mortgage` and `FairholdLandPurchase` #23

DafyddLlyr commented Jul 26, 2024 •

edited

Loading

vercel bot commented Jul 26, 2024 •

edited

Loading

DafyddLlyr Jul 26, 2024

DafyddLlyr Jul 26, 2024

DafyddLlyr Jul 26, 2024

gabrielegranello Jul 29, 2024

DafyddLlyr Jul 26, 2024

DafyddLlyr Jul 26, 2024

zz-hh-aa Jul 29, 2024

DafyddLlyr Jul 29, 2024

DafyddLlyr Jul 26, 2024

DafyddLlyr Jul 28, 2024 •

edited

Loading

gabrielegranello Jul 29, 2024

gabrielegranello Jul 29, 2024

DafyddLlyr Jul 31, 2024

zz-hh-aa Jul 29, 2024

DafyddLlyr Jul 31, 2024

gabrielegranello Jul 29, 2024 •

edited

Loading

DafyddLlyr Jul 31, 2024

gabrielegranello Jul 29, 2024

DafyddLlyr Jul 31, 2024

zz-hh-aa left a comment

		* This value is given as a percentage. For example, 0.05 represents a 5% rate
		*/

feat: Improve types in Mortgage and FairholdLandPurchase #23

feat: Improve types in Mortgage and FairholdLandPurchase #23

Conversation

DafyddLlyr commented Jul 26, 2024 • edited Loading

What does this PR do

vercel bot commented Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DafyddLlyr Jul 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielegranello Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-hh-aa left a comment

Choose a reason for hiding this comment

feat: Improve types in `Mortgage` and `FairholdLandPurchase` #23

feat: Improve types in `Mortgage` and `FairholdLandPurchase` #23

DafyddLlyr commented Jul 26, 2024 •

edited

Loading

vercel bot commented Jul 26, 2024 •

edited

Loading

DafyddLlyr Jul 28, 2024 •

edited

Loading

gabrielegranello Jul 29, 2024 •

edited

Loading