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.

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.

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.