6
\$\begingroup\$

Any comment on my code?

Data model

public class LoanRepaymentPlan
{
    public decimal TotalRepaymentAmount { get; private set; }
    public decimal CurrentOutstandingAmount { get; private set; }
    public decimal MonthlyRepaymentInterestAmount { get; private set; }
    public decimal MonthlyRepaymentAmount { get; private set; }
    public DateTime RepaymentDate { get; private set; }

    private LoanRepaymentPlan(decimal TotalRepaymentAmount, decimal CurrentOutstandingAmount, decimal MonthlyRepaymentAmount)
    {
        this.TotalRepaymentAmount = TotalRepaymentAmount;
        this.CurrentOutstandingAmount = CurrentOutstandingAmount;
        this.MonthlyRepaymentAmount = MonthlyRepaymentAmount;
    }

    public static LoanRepaymentPlan Create(decimal TotalRepaymentAmount, decimal CurrentOutstandingAmount, decimal MonthlyRepaymentAmount)
    {
        return new LoanRepaymentPlan(TotalRepaymentAmount, CurrentOutstandingAmount, MonthlyRepaymentAmount);
    }
}

Main calculation

static void Main(string[] args)
{
    Console.WriteLine("Equated Monthly Installment (EMI) for Home Loan");

    Console.WriteLine("Total Loan Amount: ");
    var principal = Convert.ToDouble(Console.ReadLine());

    Console.WriteLine("Interest Rate Per Annum (%): ");
    var interestRatePerAnnum = Convert.ToDouble(Console.ReadLine()) / 100;

    Console.WriteLine("Loan Period (in years): ");
    var years = Convert.ToDouble(Console.ReadLine());

    Console.WriteLine("------------------------------------------------");

    Console.WriteLine("Monthly installment: " + HousingLoanInterest(principal, interestRatePerAnnum, years).ToString("C2"));

    decimal totalRepaymentAmount = 0;
    var monthlyRepaymentAmount = HousingLoanInterest(principal, interestRatePerAnnum, years);
    var tenure = years * 12;
    var principalAndInterest = CompoundInterest(principal, interestRatePerAnnum, 12, years);
    List<LoanRepaymentPlan> repaymentPlans = new List<LoanRepaymentPlan>();

    while (tenure >= 0)
    {
        totalRepaymentAmount = totalRepaymentAmount + (decimal)monthlyRepaymentAmount;
        tenure = tenure - 1;

        repaymentPlans.Add(LoanRepaymentPlan.Create(totalRepaymentAmount,
            (decimal)principalAndInterest - totalRepaymentAmount, (decimal)monthlyRepaymentAmount));
    }

    var table = new ConsoleTable("Current Outstanding Amount", "Monthly Repayment Amount", "Total Repayment Amount");
    foreach (var item in repaymentPlans)
    {
        table.AddRow(item.CurrentOutstandingAmount, item.MonthlyRepaymentAmount, item.TotalRepaymentAmount);
    }
    table.Write();
}

Helper methods

static double CompoundInterest(double principal, double interestRate, int timesPerYear, double years)
{
    // (1 + r/n)
    double body = 1 + (interestRate / timesPerYear);

    // nt
    double exponent = timesPerYear * years;

    // P(1 + r/n)^nt
    return principal * Math.Pow(body, exponent);
}

static double HousingLoanInterest(double loanAmount, double interestRate, double years)
{
    return (loanAmount * Math.Pow((interestRate / 12) + 1,
         (years * 12)) * interestRate / 12) / (Math.Pow
         (interestRate / 12 + 1, (years * 12)) - 1);
}

There are some inaccuracy of the calculation but I will try to figure out later. One thing bothers me is the Math.Pow which uses double. I have tried to convert the calculation to decimal but unable to do so at the moment.

\$\endgroup\$
  • 1
    \$\begingroup\$ Great first question! \$\endgroup\$ – Reinderien Jul 14 at 2:55
6
\$\begingroup\$

var

Particularly given that you have accuracy issues, it's important for your own understanding of what the code does to be explicit in the following declarations:

    var monthlyRepaymentAmount = HousingLoanInterest(principal, interestRatePerAnnum, years);
    var tenure = years * 12;
    var principalAndInterest = CompoundInterest(principal, interestRatePerAnnum, 12, years);

From these lines alone, I have no idea if these are supposed to be doubles, integers, decimals, etc. Just spell out the actual type.

In-place operations

        totalRepaymentAmount = totalRepaymentAmount + (decimal)monthlyRepaymentAmount;
        tenure = tenure - 1;
     

can be

totalRepaymentAmount += monthlyRepaymentAmount;
tenure--;

Formatting

This could benefit from some temporary variables, but even without them,

    return (loanAmount * Math.Pow((interestRate / 12) + 1,
         (years * 12)) * interestRate / 12) / (Math.Pow
         (interestRate / 12 + 1, (years * 12)) - 1);

is more legible as

return (
    loanAmount * Math.Pow(
        interestRate / 12 + 1,
        years * 12
    ) * interestRate / 12
) / (
    Math.Pow(
        interestRate / 12 + 1,
        years * 12
    ) - 1
);

which in turn is equivalent to

double annual = interestRate / 12;
double pow = Math.Pow(
    annual + 1,
    years * 12
);
return loanAmount * pow * annual / (pow - 1);

Simpler yet:

double annual = interestRate / 12,
       pow = Math.Pow(
           annual + 1,
           -12 * years
       );
return loanAmount * annual / (1 - pow);

Please test this for equivalence.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks @Reinderien. I have refactored my code :) \$\endgroup\$ – John Jul 14 at 9:43
4
\$\begingroup\$

There are great improvement points that are mentioned by @Reinderien, so I won't iterate on them.
There are also other areas that can be fine-tuned.

LoanRepaymentPlan - Properties

You do not need to explicitly say private set for properties in this particular case. If you specify only getters that would be perfectly fine, because ctors can set properties even if they don't expose getters.

public class LoanRepaymentPlan
{
    public decimal TotalRepaymentAmount { get; }
    public decimal CurrentOutstandingAmount { get; }
    public decimal MonthlyRepaymentInterestAmount { get; }
    public decimal MonthlyRepaymentAmount { get; }
    public DateTime RepaymentDate { get; }

    public LoanRepaymentPlan(decimal TotalRepaymentAmount, decimal CurrentOutstandingAmount, decimal MonthlyRepaymentAmount)
    {
        this.TotalRepaymentAmount = TotalRepaymentAmount;
        this.CurrentOutstandingAmount = CurrentOutstandingAmount;
        this.MonthlyRepaymentAmount = MonthlyRepaymentAmount;
    }
}

With this you have created an immutable class (can't be modified after it has been initialized). From memory optimization perspective it make sense to change class to struct. That way it is a great chance that it will be allocated on the stack (or even in CPU registers) so no heap allocation is needed and no gargabe collection for this object.

There is another interesting topic, which is currently not available, but worth mentioning C# 9 records. In C# 9 the same could be achieved via the following code:

public data class LoanRepaymentPlan
{
    public decimal TotalRepaymentAmount { get; init; }
    public decimal CurrentOutstandingAmount { get; init; }
    public decimal MonthlyRepaymentInterestAmount { get; }
    public decimal MonthlyRepaymentAmount { get; init; }
    public DateTime RepaymentDate { get; }    
}

LoanRepaymentPlan - Static constructor

In my opinion it does not make too much sense. You have to write 2 more characters at each initialization and you have to maintain 2 object creator functions. I don't see the whole point of this static ctor.

Input handling

Your code bouncing between double and decimal types. If precision matters then use decimal everywhere otherwise use double.

The Convert.ToDouble(Console.ReadLine()) is really error-prone. What if the user enters "some string"? It will throw a FormatException. Use double.TryParse instead:

var userInput = Console.ReadLine();
if(!double.TryParse(userInput, out var principal))
{
  //TODO: handle that case
}

You should also consider to validate user input against valid ranges. For example interest rate should be between 0 and 100.

while vs for loop

Even though your while loop works fine in my opinion the for loop might be way more expressive and concise.

for (var tenure = years * 12; tenure >= 0; tenure--)
{
    totalRepaymentAmount += (decimal)monthlyRepaymentAmount;

    repaymentPlans.Add(LoanRepaymentPlan.Create(totalRepaymentAmount,
        (decimal)principalAndInterest - totalRepaymentAmount, (decimal)monthlyRepaymentAmount));
}

The iterator variable is declared at the same place where it is modified.
It improves readability and maintainability.

Functional decomposition

I would highly encourage you to split your main function into smaller chunks with dedicated responsibilities:

  1. Gather user inputs for calculation
  2. Perform calculation
  3. Dump calculation result

It would improve testability, maintainability and readability as well.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the code review and comments.Really appreciate it. Regarding the Static constructor, previously I learned from another programmer which I think a kind of design pattern. About the switching between double and decimal, it is all because of Math.Pow which accept boolean parameter. Not to sure how to use decimal for Math.Pow. \$\endgroup\$ – John Jul 14 at 9:42
  • 1
    \$\begingroup\$ @John Indeed Math.Pow can't accept decimals, but you can do the same in other way:** X^n = e^(n * ln x)**. Please check the following SO thread for more detail. \$\endgroup\$ – Peter Csala Jul 14 at 10:00
  • 1
    \$\begingroup\$ @John The static constructor + private constructor combo sometimes considered as a Factory method pattern wrongly. If the Product knows how to create itself then why do you need a Creator? The Product and Creator have clear responsibilities and scope. Merging them into a single object violates the whole concept. \$\endgroup\$ – Peter Csala Jul 14 at 10:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.