Geektrust coding challenge: simulating a marketplace for loans

Question

This is the my solution to a coding challenge in Geektrust. The question is linked here.

A shortened version would be as follows.

The aim is to simulate a marketplace for banks to lend money to borrowers and receive payments for the loans. It takes as input:

1. The bank name, borrower name, principal, interest and term.
2. Lump sum payments if any.
3. Given the bank name, borrower name, and EMI number, the program should print the total amount paid by the user (including the EMI number mentioned) and the remaining number of EMIs.

The output should be

1. Amount paid so far, and number of EMIs remaining for the user with the bank

These are the commands the program takes as input

1. LOAN BANK_NAME BORROWER_NAME PRINCIPAL NO_OF_YEARS RATE_OF_INTEREST
2. PAYMENT BANK_NAME BORROWER_NAME LUMP_SUM_AMOUNT EMI_NO
3. BALANCE BANK_NAME BORROWER_NAME AMOUNT_PAID EMI_NO

This is my solution to the problem

import math
from sys import argv
LOAN = "LOAN"
PAYMENT = "PAYMENT"
BALANCE = "BALANCE"
MONTHS_IN_A_YEAR = 12
HUNDRED = 100
def round_to_whole_no(n):
 return math.ceil(n)
class Bank:
 def __init__(self, name):
 self.name = name
class BankLoan:
 def __init__(self, **kwargs):
 self.bank = kwargs['bank']
 self.principal = kwargs['principal']
 self.rate_of_interest = kwargs['rate_of_interest']
 self.no_of_years = kwargs['no_of_years']
 self.borrower = kwargs['borrower']
 def interest(self):
 return self.principal * self.no_of_years * (self.rate_of_interest / HUNDRED)
 def total_amount(self):
 return self.interest() + self.principal
 def emi_amount(self):
 return round_to_whole_no(self.total_amount() / (self.no_of_years * MONTHS_IN_A_YEAR))
 def no_of_emi(self):
 return self.no_of_years * MONTHS_IN_A_YEAR
class Borrower:
 def __init__(self, name):
 self.name = name
class LedgerEntry:
 def __init__(self, bank_loan=None, payment=None):
 self.bank_loan = bank_loan
 self.payment = payment
 def add_payment(self, payment):
 self.payment = payment
 def balance(self):
 if self.payment is None:
 return self.bank_loan.total_amount()
 else:
 return self.bank_loan.total_amount() - self.payment.lump_sum_amount
 def amount_paid(self, emi_no):
 if self.payment is None:
 return emi_no * self.bank_loan.emi_amount()
 else:
 if emi_no >= self.payment.emi_no:
 return self.payment.lump_sum_amount +\
 (self.balance()
 if ((emi_no * self.bank_loan.emi_amount()) + self.payment.lump_sum_amount) >
 self.bank_loan.total_amount()
 else emi_no * self.bank_loan.emi_amount())
 else:
 return emi_no * self.bank_loan.emi_amount()
 def no_of_emi_left(self, emi_no):
 if self.payment is None:
 return self.bank_loan.no_of_emi() - emi_no
 else:
 remaining_principal = self.bank_loan.total_amount() - self.amount_paid(emi_no=emi_no)
 remaining_emi = round_to_whole_no(remaining_principal / self.bank_loan.emi_amount())
 return remaining_emi
class Ledger(dict):
 def __init__(self):
 super().__init__()
 def add_entry_to_ledger(self, bank_name, borrower_name, ledger_entry):
 self[bank_name] = {}
 self[bank_name][borrower_name] = ledger_entry
class Payment:
 def __init__(self):
 pass
 def __init__(self, **kwargs):
 self.bank_loan = kwargs['bank_loan']
 self.lump_sum_amount = kwargs['lump_sum_amount']
 self.emi_no = kwargs['emi_no']
def main():
 ledger = Ledger()
 if len(argv) != 2:
 raise Exception("File path not entered")
 file_path = argv[1]
 f = open(file_path, 'r')
 lines = f.readlines()
 for line in lines:
 if line.startswith(LOAN):
 bank_name, borrower_name, principal, no_of_years, rate_of_interest = line.split()[1:]
 bank = Bank(name=bank_name)
 borrower = Borrower(name=borrower_name)
 bank_loan = BankLoan(bank=bank,
 borrower=borrower,
 principal=float(principal),
 no_of_years=float(no_of_years),
 rate_of_interest=float(rate_of_interest))
 ledger_entry = LedgerEntry(bank_loan=bank_loan)
 ledger.add_entry_to_ledger(bank_name=bank_name, borrower_name=borrower_name,
 ledger_entry=ledger_entry)
 if line.startswith(PAYMENT):
 bank_name, borrower_name, lump_sum_amount, emi_no = line.split()[1:]
 ledger_entry = ledger[bank_name][borrower_name]
 payment = Payment(bank_loan=ledger_entry.bank_loan,
 lump_sum_amount=float(lump_sum_amount),
 emi_no=float(emi_no))
 ledger_entry.add_payment(payment)
 ledger[bank_name][borrower_name] = ledger_entry
 if line.startswith(BALANCE):
 bank_name, borrower_name, emi_no = line.split()[1:]
 ledger_entry = ledger[bank_name][borrower_name]
 amount_paid = int(ledger_entry.amount_paid(emi_no=float(emi_no)))
 no_of_emis_left = int(ledger_entry.no_of_emi_left(emi_no=float(emi_no)))
 print(bank_name, borrower_name, str(amount_paid), str(no_of_emis_left))
if __name__ == "__main__":
 main()

The automated review of the code by Geektrust has reported the code to have poor Object Oriented modelling and that it breaks encapsulation.

• SATW: Sorry about that, I have updated both the code and the link.

Answer 1

Out of all of your constants, only one is worth keeping, MONTHS_IN_A_YEAR; the others are less useful than having no declared constant at all.

round_to_whole_no first of all obscures what actually happens: I'd naively assume that this rounds up or down, but it only rounds up. But it's also less useful than having no function at all; just call ceil directly.

Add PEP484 type hints.

You have some kwarg abuse: for instance, in BankLoan's constructor, you know exactly what the arguments need to be: so why not write them out?

Bank as it stands is not useful. You could reframe it as containing loans, in which case it would be useful.

Your BankLoan.interest etc. would be better-modelled with @property.

Broadly, this code seems... not right? I don't read anything in the problem description guaranteeing that there will be at most one lump sum payment. If there is more than one lump sum payment on a loan, I doubt that your code will do the right thing.

It's not a good idea to inherit from dict in this case: you should has-a rather than is-a, for your database lookup pattern.

Use argparse.

For important financials, usually prefer Decimal instead of float.

main does too much. You need to delegate to routines in the classes.

Suggested

import argparse
import math
from bisect import bisect
from decimal import Decimal
from itertools import islice
from locale import setlocale, currency, LC_ALL
from typing import TextIO, NamedTuple, Iterable, Iterator
MONTHS_PER_YEAR = 12
class LoanRecord(NamedTuple):
 bank_name: str
 borrower_name: str
 principal: Decimal # P
 tenure_years: Decimal # N
 interest_rate: Decimal # R
 @classmethod
 def parse(
 cls,
 bank_name: str, borrower_name: str,
 principal: str, no_of_years: str, interest_rate: str,
 ) -> 'LoanRecord':
 return cls(
 bank_name=bank_name, borrower_name=borrower_name,
 principal=Decimal(principal),
 tenure_years=Decimal(no_of_years),
 interest_rate=Decimal(interest_rate),
 )
class PaymentRecord(NamedTuple):
 # in sortation priority
 emi_no: int
 bank_name: str
 borrower_name: str
 lump_sum: Decimal
 @classmethod
 def parse(
 cls,
 bank_name: str, borrower_name: str,
 lump_sum: str, emi_no: str,
 ) -> 'PaymentRecord':
 return cls(
 bank_name=bank_name, borrower_name=borrower_name,
 lump_sum=Decimal(lump_sum), emi_no=int(emi_no),
 )
class BalanceRecord(NamedTuple):
 bank_name: str
 borrower_name: str
 emi_no: int
 @classmethod
 def parse(
 cls, bank_name: str, borrower_name: str, emi_no: str,
 ) -> 'BalanceRecord':
 return cls(
 bank_name=bank_name, borrower_name=borrower_name, emi_no=int(emi_no),
 )
class Loan:
 def __init__(self, record: LoanRecord) -> None:
 self.interest_rate = record.interest_rate
 self.principal = record.principal
 self.tenure_years = record.tenure_years
 self.lump_payments: list[PaymentRecord] = []
 def process_payment(self, payment: PaymentRecord) -> None:
 i = bisect(a=self.lump_payments, x=payment)
 self.lump_payments.insert(i, payment)
 def iter_payments(self) -> Iterator[int]:
 i_start = 0
 emi_amount = self.emi_amount
 for lump_payment in self.lump_payments:
 for i in range(i_start, lump_payment.emi_no):
 yield emi_amount
 yield emi_amount + lump_payment.lump_sum
 i_start = lump_payment.emi_no
 while True:
 yield emi_amount
 def iter_payments_to_end(self) -> Iterator[int | Decimal]:
 owing = self.total_amount
 for payment in self.iter_payments():
 new_owing = owing - payment
 if new_owing < 0:
 yield owing
 return
 yield payment
 if new_owing == 0:
 return
 owing = new_owing
 def replay(self, emi_no: int) -> tuple[
 Decimal, # amount paid
 int, # payment periods left
 ]:
 payments = self.iter_payments_to_end()
 paid = sum(islice(payments, emi_no))
 payments_remaining = sum(1 for _ in payments)
 return paid, payments_remaining
 def describe(self, emi_no: int) -> str:
 paid, payments_left = self.replay(emi_no)
 return f'{currency(paid)} {payments_left}'
 @property
 def interest(self) -> Decimal: # I
 return self.principal * self.tenure_years * self.interest_rate/100
 @property
 def total_amount(self) -> Decimal: # A
 return self.principal + self.interest
 @property
 def emi_amount(self) -> int:
 return math.ceil(
 self.total_amount / self.tenure_years / MONTHS_PER_YEAR
 )
class Bank:
 def __init__(self, name: str) -> None:
 self.name = name
 self.loans: dict[str, Loan] = {}
 def __str__(self) -> str:
 return self.name
 def process_loan(self, loan: LoanRecord) -> None:
 self.loans[loan.borrower_name] = Loan(loan)
 def process_payment(self, payment: PaymentRecord) -> None:
 self.loans[payment.borrower_name].process_payment(payment)
 def describe_balance(self, balance: BalanceRecord) -> str:
 loan = self.loans[balance.borrower_name]
 return f'{self.name} {balance.borrower_name} {loan.describe(balance.emi_no)}'
class Database:
 def __init__(self) -> None:
 self.banks: dict[str, Bank] = {}
 def _get_bank(self, record: LoanRecord | PaymentRecord | BalanceRecord) -> Bank:
 bank = self.banks.get(record.bank_name)
 if bank is None:
 bank = self.banks[record.bank_name] = Bank(record.bank_name)
 return bank
 def process_loan(self, loan: LoanRecord) -> None:
 self._get_bank(loan).process_loan(loan)
 def process_payment(self, payment: PaymentRecord) -> None:
 self._get_bank(payment).process_payment(payment)
 def describe_balance(self, balance: BalanceRecord) -> str:
 return self._get_bank(balance).describe_balance(balance)
def parse_args() -> TextIO:
 parser = argparse.ArgumentParser(description='Ledger Co. marketplace processor')
 parser.add_argument(
 'filename', type=open,
 help='Name of the transaction file containing LOAN, PAYMENT and BALANCE commands',
 )
 return parser.parse_args().filename
def process_lines(database: Database, lines: Iterable[str]) -> Iterator[str]:
 for line in lines:
 record_type, *fields = line.split()
 match record_type:
 case 'LOAN':
 loan = LoanRecord.parse(*fields)
 database.process_loan(loan)
 case 'PAYMENT':
 payment = PaymentRecord.parse(*fields)
 database.process_payment(payment)
 case 'BALANCE':
 balance = BalanceRecord.parse(*fields)
 yield database.describe_balance(balance)
 case _:
 raise ValueError(f'Invalid record type {record_type}')
def main() -> None:
 setlocale(LC_ALL, '')
 database = Database()
 with parse_args() as f:
 for balance_desc in process_lines(database=database, lines=f):
 print(balance_desc)
if __name__ == "__main__":
 main()

For this sample input:

LOAN MBI Dale 10000 5.5 3.9
PAYMENT MBI Dale 1270 5
BALANCE MBI Dale 12

The output is:

MBI Dale 3490ドル.00 47

Further optimisation is possible by calculating segments between lump sum payments in one shot instead of iterating over every single payment period.

• Grajdeanu Alex: Extremely helpful & nice answer as always. One question though: is there any reason / advantage of inheriting from NamedTuple instead of using a dataclass? ^_^
• Reinderien: @GrajdeanuAlex Yes, the former is much simpler internally, and potentially more efficient.

Answer 2

A few tips.

One is to be aware of the anaemic domain antipattern (https://en.wikipedia.org/wiki/Anemic_domain_model). Classes with only attributes and no methods aren't really in the spirit of OO, so things like 'Bank' and 'Borrower' should probably just be strings (unless you want to add more functionality).

Another is to consider the information expert principle (https://en.wikipedia.org/wiki/GRASP_(object-oriented_design)#Information_expert). Your LedgerEntry class (for instance) is heavily dependent on your BankLoan class, and needs to ask it for a large amount of information in order to work. You could instead have a 'repay' method on your BankLoan class, and drop big parts of the remaining interface. As is, the two classes aren't 'really' separate, as they rely on each other too much.

If you'd like to separate things out into lots of classes, that's fine. But it's important to provide minimal high-level facades for your end users. As is, they have to reach deep into your object model, and interact with several classes to make a transaction, which leads to a big surface area between you and the end user. Find the minimal interface the user needs to fulfil their requirements and make everything else private.