Rate calculation system for a code challenge

Question

This code challenge was part of a interview process I had applied for, and from which I was discarded after this test. I'd like to hear what other people have to say, hopefully in deeper detail.

The requirement was to build a rate calculation command-line application allowing borrowers to obtain a quote from a pool of lenders for 36 month loans.

A file containing a list of all the offers being made by the lenders within the system in CSV format will be provided. This file has three columns with header, 'Lender', 'Rate', 'Available', comma separated. The application should provide as low a rate to the borrower as is possible to ensure that the quotes are as competitive as they can be against our competitors.

It should also provide the borrower with the details of the monthly repayment amount and the total repayment amount. Repayment amounts should be displayed to 2 decimal places and the rate of the loan should be displayed to one decimal place. Borrowers should be able to request a loan of any £100 increment between £1000 and £15000 inclusive. If the market does not have sufficient offers from lenders to satisfy the loan then the system should inform the borrower that it is not possible to provide a quote at that time.

My code:

RateCalculatorApp.java

import domain.LenderOffer;
import interest.CompoundInterestCalculator;
import ratefinder.MultipleLendersRateFinder;
import ratefinder.RateFinder;
import result.ResultDisplay;

import java.math.BigDecimal;
import java.util.Collection;
import java.util.Optional;

import static csv.CsvReader.csvToLenderInfoList;
import static validation.InputValidator.validateArguments;

public class RateCalculatorApp {

    private static RateFinder rateFinder = new MultipleLendersRateFinder();

    public static void main(String[] args) {
        validateArguments(args);
        Collection<LenderOffer> lenderOffers = csvToLenderInfoList(args[0]);
        BigDecimal amount = new BigDecimal(args[1]);
        Optional<BigDecimal> rateOptional = rateFinder.findLowestRate(lenderOffers, amount);
        if (rateOptional.isPresent()) {
            CompoundInterestCalculator compoundInterestCalculator = new CompoundInterestCalculator(amount, rateOptional.get());
            ResultDisplay resultDisplay = new ResultDisplay(amount, rateOptional.get(),
                    compoundInterestCalculator.getMonthlyRepayment(), compoundInterestCalculator.getTotalRepayment());
            resultDisplay.display();
        } else {
            System.out.println("Is not possible to provide a quote at this time.");
        }
    }

}

MathConstants.java

package constants;
import java.math.BigDecimal;

public final class MathConstants {

    /**
     * Scale to be used on dividing BigDecimals when precision matters
     */
    public static final int DIVISION_SCALE = 10;

    /**
     * Rounding mode to be used on dividing BigDecimals when precision matters
     */
    public final static int ROUND_MODE = BigDecimal.ROUND_UP;

    private MathConstants() {
    }

}

CsvReader.java

package csv;
import domain.LenderOffer;

import java.io.IOException;
import java.math.BigDecimal;
import java.util.Collection;
import java.util.List;

import static java.nio.file.Files.readAllLines;
import static java.nio.file.Paths.get;
import static java.util.stream.Collectors.toList;

public class CsvReader {

    public static Collection<LenderOffer> csvToLenderInfoList(String marketFile) {
        try {
            List<String> marketDataEntriesAsStrings = readAllLines(get(marketFile));
            return parseStringsToLenderInfo(marketDataEntriesAsStrings);
        } catch (IOException e) {
            // Needs proper exception handling/logging, depending on how we want to deal with such cases
            throw new RuntimeException("Couldn't read market data file", e);
        }
    }

    private static Collection<LenderOffer> parseStringsToLenderInfo(List<String> marketDataEntriesAsStrings) {
        return marketDataEntriesAsStrings.stream().skip(1).map(s -> {
            String[] lenderInfoAsArray = s.split(",");
            BigDecimal rate = new BigDecimal(lenderInfoAsArray[1]);
            BigDecimal available = new BigDecimal(lenderInfoAsArray[2]);
            return new LenderOffer(rate, available);
        }).collect(toList());
    }
}

LenderOffer.java

package domain;
import java.math.BigDecimal;

public class LenderOffer {

    private final BigDecimal rate;
    private final BigDecimal available;

    public LenderOffer(BigDecimal rate, BigDecimal available) {
        this.rate = rate;
        this.available = available;
    }

    public BigDecimal getRate() {
        return rate;
    }

    public BigDecimal getAvailable() {
        return available;
    }

}

CompoundInterestCalculator.java

package interest;
import java.math.BigDecimal;

import static constants.MathConstants.DIVISION_SCALE;
import static constants.MathConstants.ROUND_MODE;
import static java.math.BigDecimal.ONE;
import static java.math.BigDecimal.valueOf;

public class CompoundInterestCalculator {

    private static final Integer DEFAULT_NUMBER_MONTHS = 36;

    private final BigDecimal baseAmount;
    private final BigDecimal annualInterestRatePerOne;
    private final Integer months;

    public CompoundInterestCalculator(BigDecimal baseAmount, BigDecimal annualInterestRatePerOne) {
        this(baseAmount, annualInterestRatePerOne, DEFAULT_NUMBER_MONTHS);
    }

    public CompoundInterestCalculator(BigDecimal baseAmount, BigDecimal annualInterestRatePerOne, Integer months) {
        this.baseAmount = baseAmount;
        this.annualInterestRatePerOne = annualInterestRatePerOne;
        this.months = months;
    }

    private BigDecimal calculateTotalRepayment() {
        return ONE.add(annualInterestRatePerOne.divide(valueOf(12), DIVISION_SCALE, ROUND_MODE)).pow(months).multiply(baseAmount);
    }

    public BigDecimal getMonthlyRepayment() {
        return calculateTotalRepayment().divide(valueOf(months), DIVISION_SCALE, ROUND_MODE);
    }

    public BigDecimal getTotalRepayment() {
        return calculateTotalRepayment();
    }
}

RateFinder.java

package ratefinder;
import domain.LenderOffer;    
import java.math.BigDecimal;
import java.util.Collection;
import java.util.Optional;

public interface RateFinder {

    /**
     * Find the lowest rate within the given offers that satisfies the specified amount
     *
     * @param lenderOffers the offer list containing rates and available amount
     * @param amount       the amount to be lent
     * @return an optional with the found rate, empty if no matching quote can be provided
     */
    Optional<BigDecimal> findLowestRate(Collection<LenderOffer> lenderOffers, BigDecimal amount);
}

MultipleLendersRateFinder.java

package ratefinder;
import domain.LenderOffer;

import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Optional;

import static constants.MathConstants.DIVISION_SCALE;
import static constants.MathConstants.ROUND_MODE;
import static java.math.BigDecimal.ZERO;

public class MultipleLendersRateFinder implements RateFinder {

    @Override
    public Optional<BigDecimal> findLowestRate(Collection<LenderOffer> lenderOffers, BigDecimal amount) {
        List<LenderOffer> lenderOfferList = getOffersAsSortedList(lenderOffers);
        BigDecimal resultRate = ZERO;
        BigDecimal cumulativeAmount = ZERO;
        for (int i = 0; i < lenderOfferList.size() && cumulativeAmount.compareTo(amount) < 0; i++) {
            LenderOffer lenderOffer = lenderOfferList.get(i);
            BigDecimal amountToBeLentThisOffer = lenderOffer.getAvailable().min(amount.subtract(cumulativeAmount));
            BigDecimal factorThisOffer = amountToBeLentThisOffer.divide(amount, DIVISION_SCALE, ROUND_MODE);
            resultRate = resultRate.add(lenderOffer.getRate().multiply(factorThisOffer));
            cumulativeAmount = cumulativeAmount.add(amountToBeLentThisOffer);
        }
        if (cumulativeAmount.compareTo(amount) < 0) {
            return Optional.empty();
        } else {
            return Optional.of(resultRate);
        }
    }

    private List<LenderOffer> getOffersAsSortedList(Collection<LenderOffer> lenderOffers) {
        List<LenderOffer> lenderOfferList = new ArrayList<>(lenderOffers);
        lenderOfferList.sort((o1, o2) -> o1.getRate().compareTo(o2.getRate()));
        return lenderOfferList;
    }
}

SingleLenderRateFinder.java

package ratefinder;
import domain.LenderOffer;    
import java.math.BigDecimal;
import java.util.Collection;
import java.util.Optional;

public class SingleLenderRateFinder implements RateFinder {

    @Override
    public Optional<BigDecimal> findLowestRate(Collection<LenderOffer> lenderOffers, BigDecimal amount) {
        return lenderOffers.stream()
                .filter(lenderOffer -> lenderOffer.getAvailable().compareTo(amount) >= 0)
                .sorted((o1, o2) -> o1.getRate().compareTo(o2.getRate()))
                .map(LenderOffer::getRate)
                .findFirst();
    }
}

ResultDisplay.java

package result;
import java.math.BigDecimal;
import static constants.MathConstants.ROUND_MODE;

public class ResultDisplay {

    private final BigDecimal amount;
    private final BigDecimal rate;
    private final BigDecimal monthlyRepayment;
    private final BigDecimal totalRepayment;

    public ResultDisplay(BigDecimal amount, BigDecimal rate, BigDecimal monthlyRepayment, BigDecimal totalRepayment) {
        this.amount = amount;
        this.rate = rate;
        this.monthlyRepayment = monthlyRepayment;
        this.totalRepayment = totalRepayment;
    }

    public void display() {
        System.out.println("Requested amount: £" + amount.setScale(0, ROUND_MODE));
        System.out.println("Rate: " + rate.multiply(BigDecimal.valueOf(100)).setScale(1, ROUND_MODE) + "%");
        System.out.println("Monthly repayment: £" + monthlyRepayment.setScale(2, ROUND_MODE));
        System.out.println("Total repayment: £" + totalRepayment.setScale(2, ROUND_MODE));
    }
}

InputValidator.java

package validation;    
import java.util.Arrays;    
import static java.nio.file.Files.exists;
import static java.nio.file.Paths.get;

public class InputValidator {

    private static final int MINIMUM_AMOUNT = 1000;
    private static final int MAXIMUM_AMOUNT = 15000;
    private static final int FACTOR = 100;

    public static void validateArguments(String[] args) {
        validateGotTwoArgs(args);
        validateFileExists(args[0]);
        validateAmount(args[1]);
    }

    private static void validateGotTwoArgs(String[] args) {
        if (args.length != 2) {
            throw new IllegalArgumentException("App needs two arguments. Given was: " + Arrays.toString(args));
        }
    }

    private static void validateFileExists(String marketFileName) {
        if (!exists(get(marketFileName))) {
            throw new IllegalArgumentException("Couldn't find market data file in given path: " + marketFileName);
        }
    }

    private static void validateAmount(String amountAsString) {
        validateAmountIsANumber(amountAsString);
        Integer amount = Integer.valueOf(amountAsString);
        validateAmountIsInRange(amount);
        validateAmountIsMultiple(amount);
    }

    private static void validateAmountIsANumber(String amountAsString) {
        try {
            Integer.valueOf(amountAsString);
        } catch (NumberFormatException e) {
            throw new IllegalArgumentException("Given amount is not a number: " + amountAsString);
        }
    }

    private static void validateAmountIsInRange(Integer amount) {
        if (amount < MINIMUM_AMOUNT || amount > MAXIMUM_AMOUNT) {
            throw new IllegalArgumentException("Amount must be within [" + MINIMUM_AMOUNT + ", " + MAXIMUM_AMOUNT + "], was: " + amount);
        }
    }

    private static void validateAmountIsMultiple(Integer amount) {
        if (amount % FACTOR != 0) {
            throw new IllegalArgumentException("Amount must be a multiple of " + FACTOR + ", was: " + amount);
        }
    }
}

I've omitted the test classes because that's already long enough, but I had 21 tests covering 70% of the code. I have two implementations of RateFinder because I wasn't sure of whether the offers could needed to come from a single lender or could come from multiple, and didn't get an explanation before the deadline. The feedback I got from the company was:

• SoC is not good. RateFinder is returning the rate, then the calculator is doing other calculations, but they should be together. RateFinder should return a set of lenders or something similar

• overuse of static import, so much that it becomes a code smell

• Results are far from expected

I don't know what they mean by "results are far from expected", and haven't get that answered. I am positive that the results are functionally right, so I assume it's about code quality. These two online calculators give the same result as my program: https://www.thecalculatorsite.com/finance/calculators/compoundinterestcalculator.php and https://www.paisabazaar.com/compound-interest-calculator/

Answer 1

commenting the review

RateFinder is returning the rate, then the calculator is doing other calculations, but they should be together.

If you look at the loop in MultipleLendersRateFinder.java 3 of the 5 lines access properties of LenderOffer. This is a clear sign that these 3 lines should be in LenderOffer.

This "Responsibility Distribution" also occurs with the InputValidator and the CsvReader. IMHO the check for the existence of the file belongs to the CsvReader rather then to the InputValidator. After all javas own FileReader thows a FileNotFoundException too...

An another place is your main method where you do a single System.out.println() although you (could) have you ResultDisplay handling that case too.

RateFinder should return a set of lenders or something similar

Just guessing, but I think they wanted a list of lenders alomg with the ammount to lend and a summery line with the overall result.

overuse of static import, so much that it becomes a code smell

I strongly support that.

I use static key word only if I have a good reason and the ability to call something from main does not count here.

In you code the static import addiction tricked me to think validateArguments(args); was a method in RateCalculatorApp.

other findings

odd ball solution

All over your code you are using Java8 streams but in MultipleLendersRateFinder.java you fall back to loop with index _{(although foreach loop might have been less "old fashioned"...)}.

unnessessary state

Your ResultDisplay has only a single method but you enforce state by passing the values to display as constructor parameters. At least it is immutable which is the good part about it.

---

On the other hand I can imagin an approach where that might have been usefull:
ResultDisplay could have been an interface with one Implementation as you actually did and another NoResultDisplay that only outputs the requested prase as anonymous class instance or even a lambda assigned to a constant in that interface. Then your main could look like this:

    ResulpDisplay resultDisplay = ResulpDisplay.NO_RESULT;
    if (rateOptional.isPresent()) {
        CompoundInterestCalculator compoundInterestCalculator = 
              new CompoundInterestCalculator(
                      amount, 
                      rateOptional.get());
        resultDisplay =
              new LowestRateDisplay(
                      amount, 
                      rateOptional.get(),
                      compoundInterestCalculator.getMonthlyRepayment(), 
                      compoundInterestCalculator.getTotalRepayment());
    }
    resultDisplay.display();

and according to the same level of abstraction pattern I'd even move that to a separate method:

  private /*static*/ ResultDisplay createDisplay(
                      Optional<BigDecimal> rateOptional,
                      BigDecimal amount
  ) {
    if (rateOptional.isPresent()) {
        CompoundInterestCalculator compoundInterestCalculator = 
              new CompoundInterestCalculator(
                      amount, 
                      rateOptional.get());
        return new LowestRateDisplay(
                      amount, 
                      rateOptional.get(),
                      compoundInterestCalculator.getMonthlyRepayment(), 
                      compoundInterestCalculator.getTotalRepayment());
     } else {
        return ResultDisplay.NO_RESULT;
     }
  }

// main
    ResultDisplay resultDisplay  = createDisplay(
                      rateOptional,
                      amount);
    resultDisplay.display(); 

naming

Choose your names from the problem domain (only). E.g. you named a variable rateOptional but what value does the technical aspect that thsi variable holds an Optional ad to someone hwho wants ti understand your algorithm?
None...

So get rid of that implementation related suffixes like *App, *Finder or *Calculator.

Constant container class

You created a Constant container class to provide system with constants. But there is only one constant you use in more then one class. And even this is quiestionable if that should be a compile time constant of it that might rather be something that we want to configure at program start.

Also you created another odd ball solution by having the static import to the Constant container class and a class constant in the same file.

So don't do this.

Usually classes using the same constants also tend to implement the same interface which would be a much better place for such constants. And values that are runtime constants but may change on proram start should be passed around as constructor parameters.

---

Regarding the static imports, what would you use instead of, let's say, the import of InputValidator.validateArguments()? Would you make the methods non-static and then just use it as new InputValidator().validateArguments(args)? – antonro

Yes.

I'd even go one step further and pass those instances as Constructor parameters to the RateCalculatorApp instancs:

public static void main(
           String[] args
) {
   RateCalculatorApp rateCalculator =
       new RateCalculatorApp(
           new InputValidator(),
           new CsvReader());
   rateCalculator.process(
           args);
}
private void process(
           String[] args
) {
   inputValidator.validateArguments(
           args);
   Collection<LenderOffer> lenderOffers =
       csvReader.csvToLenderInfoList(
           args[0]);
   //...
}

This opens an easy way to replace this rather expensive dependencies with test doubles (e.g. mocks created with a mocking framework) for unit testing.