4
\$\begingroup\$

I was failed at coding challenge for my interview with the following reasons given by the interviewer: deep use of mutation and a not that well thought up internal API.

I understand that my code and design have so many flaws and I genuinely want to get your opinion so I could improve my code.

The challenge is about asking me to build a quote processor to calculate a premium for different products, based on different rule sets.

Since the requirements are very lengthy, I try to summarize it below:

Data model

  • A user (with an id, an address, and a risk value)
  • An address (with an id and a location risk value)
  • A product (with an id and a price value)

(Risk values are in our patented Universal Risk Value Scale [tm])

For products we have modelled in our system the following types: - Houses (with address and size in square meters) - Bananas (with number of black spots) - Bicycles (with number of gears)

Asides from above, the premium quote would need to take in rules, base surcharge, universal risk value for each product and each user to quote a premium for user.

Rules

We want to build a quote processor that will receive a user and a set of products, and return whether we reject the user or not, and if not, for how much premium will we insure the products in euros per year.

To ensure this, we have a set of rules to follow.

Each product will have a base premium value, that multiplied by the product value will give a subtotal for the product's premium. To this subtotal, we must add a surcharge that depends on the product type and maybe on other rules. Each surcharge is multiplied to the base value.

Then, for all the products, if for one the rules say that it is un-insurable, then the final result given is that we won't offer an insurance. If no product is un-insurable, then we add all the premiums and that is the quote we return to the user.

EDIT - ADD MORE INFORMATION

Base Premium surcharge value per product

Banana: 1.15 House: 0.03

Surcharges

  • User

Depending on our estimation of the risk value of the user, we apply the following surcharges: enter image description here

  • Banana

    • We won't offer insurance for bananas unless their number of black spots is between 3 and 12 (inclusive).
    • We also won't offer banana insurance to users with a risk value of more than 200 (our investigations show they tend to lose them)

EXAMPLES 1:

  • 1,000,000 * // house value
  • 0.03 * // house base premium value
  • 0.7 * // house risk surcharge
  • 0.3 // user risk surcharge
  • = 6300 € per year as premium quote (=1000000 * 0.03 * 0.7 * 0.3)

EXAMPLES 2: BANANA

  • 10 * // banana value
  • 4 * // number of black spots (inclusive = no additional base premium value to be calculated)
  • 1 // user risk surcharge (user's risk value between 21 and 200)
  • = 10 € per year as premium quote (=10 * 1)

Below are my classes, I am putting here one or two model classes just to present my design.

Interfaces:

public interface Product {
    BigDecimal getProductValue();
    BigDecimal getBasePremium();
    BigDecimal getSubTotalPremium();
}

public interface Risk {

    void setRiskValue(int riskValue);
    BigDecimal getRiskSurcharge();

    static BigDecimal getUnIdentifiedRiskSurcharge()
    {
        return new BigDecimal(-1);
    }

    static BigDecimal getDefaultRiskSurcharge()
    {
        return new BigDecimal(0);
    }

    static boolean isWithinRange(int riskValue, int minValue, int maxValue)
    {
        return riskValue >= minValue && riskValue <= maxValue;
    }
}

Model classes: - Base class InsuredProduct implements above interfaces.

public abstract class InsuredProduct implements Product, Risk {
    private int id;
    private BigDecimal productValue;
    private int riskValue;

    public int getId() {
        return id;
    }
    public void setId(int id) {
        this.id = id;
    }

    public InsuredProduct(int id, BigDecimal productValue)
    {
        this.id = id;
        this.productValue =productValue;
    }

    public BigDecimal getProductValue() {
        return productValue;
    }

    public void setProductValue(BigDecimal productValue) {
        this.productValue = productValue;
    }

    @Override
    public BigDecimal getSubTotalPremium()
    {
        return getProductValue().multiply(getBasePremium());
    }

    @Override
    public void setRiskValue(int riskValue) {
        this.riskValue = riskValue;
    }

    public int getRiskValue() {
        return this.riskValue;
    }

}

Then Banana class as insured product extends from base class:

public class Banana extends InsuredProduct {
    private static BigDecimal BASE_PREMIUM = new BigDecimal(1.15);

    private int noOfBlackSpots;
    private int userRiskValue;

    public Banana(int id, BigDecimal productValue) {
        super(id, productValue);
    }

    public int getNoOfBlackSpots() {
        return noOfBlackSpots;
    }

    public void setNoOfBlackSpots(int noOfBlackSpots) {
        this.noOfBlackSpots = noOfBlackSpots;
    }

    @Override
    public BigDecimal getBasePremium() {
        return BASE_PREMIUM;
    }

    @Override
    public BigDecimal getRiskSurcharge() {
        if(this.userRiskValue > 200)
            return Risk.getUnIdentifiedRiskSurcharge();

        final int minAcceptableBlackSpots = 3;
        final int maxAcceptableBlackSpots = 12;

        if(Risk.isWithinRange(this.noOfBlackSpots, minAcceptableBlackSpots, maxAcceptableBlackSpots))
            return Risk.getDefaultRiskSurcharge(); 

        return Risk.getUnIdentifiedRiskSurcharge();
    }

    @Override
    public void setRiskValue(int riskValue) {
        this.userRiskValue = riskValue;
    }
}

A User class (with Address) as an insured person. User's address is also part of risk surcharge to determine the quote calculation.

public class User implements Risk {
    private static final BigDecimal MAXIMUM_USER_RISK_SURCHARGE = new BigDecimal(3.0);
    private static final BigDecimal MEDIUM_USER_RISK_SURCHARGE = new BigDecimal(1.0);
    private static final BigDecimal MINIMUM_USER_RISK_SURCHARGE = new BigDecimal(0.3);

    private int id;
    private Address address;
    private int riskValue;

    public User(int id, Address address)
    {
        this.setId(id);
        this.setAddress(address);
    }

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }

    public Address getAddress() {
        return address;
    }

    public void setAddress(Address address) {
        this.address = address;
    }

    public int getRiskValue() {
        return riskValue;
    }

    @Override
    public void setRiskValue(int riskValue) {
        this.riskValue = riskValue;
    }

    @Override
    public BigDecimal getRiskSurcharge() {
        final int unsupportedRiskValue = 501;
        final int startMinimumRiskValue = 20;
        final int endMinimumRangeRiskValue = 200;
        final int startMaximumRiskValue = 201;
        final int endMaximumRangeRiskValue = 500;

        if(this.getRiskValue() >= unsupportedRiskValue)
            return Risk.getUnIdentifiedRiskSurcharge();

        if(this.getRiskValue() < startMinimumRiskValue)
            return MINIMUM_USER_RISK_SURCHARGE;
        if(Risk.isWithinRange(this.getRiskValue(), startMinimumRiskValue, endMinimumRangeRiskValue))
            return MEDIUM_USER_RISK_SURCHARGE;
        if(Risk.isWithinRange(this.getRiskValue(), startMaximumRiskValue, endMaximumRangeRiskValue))
            return MAXIMUM_USER_RISK_SURCHARGE;

        return Risk.getDefaultRiskSurcharge();  
    }

}

public class Address implements Risk{
    private static final BigDecimal MAXIMUM_LOCATION_RISK_SURCHARGE = new BigDecimal(2.5);
    private static final BigDecimal MEDIUM_LOCATION_RISK_SURCHARGE = new BigDecimal(1.0);
    private static final BigDecimal MINIMUM_LOCATION_RISK_SURCHARGE = new BigDecimal(0.7);

    private int id;
    private int locationRisk;

    public Address(int id)
    {
        this.id = id;
    }

    public int getRiskValue() {
        return locationRisk;
    }

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }

    @Override
    public void setRiskValue(int riskValue) {
        this.locationRisk = riskValue;
    }

    @Override
    public BigDecimal getRiskSurcharge() {
        final int unsupportedLocationRisk = 502;
        final int startMinimumLocationRisk = 100;
        final int endMinimumRangeLocationRisk = 299;
        final int startMaximumLocationRisk = 300;
        final int endMaximumRangeLocationRisk = 501;

        if(this.getRiskValue() >= unsupportedLocationRisk)
            return Risk.getUnIdentifiedRiskSurcharge();

        if(this.getRiskValue() < startMinimumLocationRisk)
            return MINIMUM_LOCATION_RISK_SURCHARGE;
        if(Risk.isWithinRange(this.getRiskValue(), startMinimumLocationRisk, endMinimumRangeLocationRisk))
            return MEDIUM_LOCATION_RISK_SURCHARGE;
        if(Risk.isWithinRange(this.getRiskValue(), startMaximumLocationRisk, endMaximumRangeLocationRisk))
            return MAXIMUM_LOCATION_RISK_SURCHARGE;

        return Risk.getDefaultRiskSurcharge();
    }
}

Processor: To calculate the premium quote or to determine if it's is insurable.

public class UserPremiumBuilder<T extends InsuredProduct> {

    private User user;
    T product;

    private UserPremiumBuilder(User user){
        this.user = user;
    }

    public static <T extends InsuredProduct> UserPremiumBuilder<T> createProfile(User user){
        UserPremiumBuilder<T> builder = new UserPremiumBuilder<T>(user);
        return builder;
    }

    public UserPremiumBuilder <T> setProduct(T product) {
        this.product = product;
        return this; 
    }

    public BigDecimal calculateSingleProductPremiumForUser() {
        BigDecimal userRiskSurcharge = user.getRiskSurcharge();
        if(Risk.getUnIdentifiedRiskSurcharge().compareTo(userRiskSurcharge)==0)
            return Risk.getUnIdentifiedRiskSurcharge();

        this.product.setRiskValue(this.user.getRiskValue());
        BigDecimal productRiskSurcharge =this.product.getRiskSurcharge();
        if(Risk.getUnIdentifiedRiskSurcharge().compareTo(productRiskSurcharge)==0)
            return Risk.getUnIdentifiedRiskSurcharge();

        if(Risk.getDefaultRiskSurcharge().compareTo(productRiskSurcharge)==0)
            return this.product.getSubTotalPremium();

        BigDecimal totalPremium = this.product.getSubTotalPremium().multiply(this.product.getRiskSurcharge()).multiply(user.getRiskSurcharge());
        return totalPremium;
    }

}

A sample unit test class:

public class UserPremiumBuilderBananaTest {
    Banana banana; 

    @Before
    public void initBanana()
    {
        banana = new Banana(1, new BigDecimal(10));
    }

    @Test
    public void testUnInsurableBananaRiskSurchargeWithUserRiskValueLargerThan200() {
        User user = initUser(10);
        user.setRiskValue(201);

        banana.setRiskValue(user.getRiskValue());
        banana.setNoOfBlackSpots(4);

        BigDecimal totalPremium = createProfile(user).setProduct(banana).calculateSingleProductPremiumForUser();
        assertEquals(Risk.getUnIdentifiedRiskSurcharge(), totalPremium);
    }

    @Test
    public void testInsurableBananaRiskSurchargeWithUserRiskNotGreaterThan200() {
        User user = initUser(10);
        user.setRiskValue(200);

        banana.setRiskValue(user.getRiskValue());
        banana.setNoOfBlackSpots(3);

        UserPremiumBuilder<Banana> builder = createProfile(user);
        BigDecimal totalPremium = builder.setProduct(banana).calculateSingleProductPremiumForUser();
        totalPremium = totalPremium.setScale(2, BigDecimal.ROUND_HALF_EVEN);
        BigDecimal productPremium = banana.getSubTotalPremium().setScale(2, BigDecimal.ROUND_HALF_EVEN);
        assertEquals(productPremium, totalPremium);

        user.setRiskValue(199);
        totalPremium = builder.setProduct(banana).calculateSingleProductPremiumForUser();
        totalPremium = totalPremium.setScale(2, BigDecimal.ROUND_HALF_EVEN);
        assertEquals(productPremium, totalPremium);

    }

    private static UserPremiumBuilder<Banana> createProfile(User user)
    {
        return UserPremiumBuilder.createProfile(user);
    }

    private User initUser(int addressRiskValue)
    {
        Address address = new Address(1);
        address.setRiskValue(addressRiskValue);
        User user = new User(1, address);   
        return user;
    }
}
\$\endgroup\$
  • \$\begingroup\$ At a first glance, try to avoid instantiating BigDecimals when you can reuse existing instances. Thus, instead of new BigDecimal(0) use BigDecimal.ZERO. Similar instances exists for ONE and TEN as well. Also, don't use the double constructor of BigDecimal, because double can be imprecise and your 3.0 could become 3.0000000001. Use either new BigDecimal("3.0") or BigDecimal.valueOf(3.0). \$\endgroup\$ – Tom Dec 11 '18 at 15:22
  • \$\begingroup\$ @Tom: Thank you, definitely very useful advice! If you could also shed some light on the interviewer's feedback to my code: "deep use of mutation and a not that well thought up internal API.", I really appreciate that! \$\endgroup\$ – Anna Dec 11 '18 at 15:45
  • \$\begingroup\$ I'm sorry, I don't have the time for further inspections of the code, but I'm sure someone else will gladly help you. \$\endgroup\$ – Tom Dec 11 '18 at 15:50
  • 1
    \$\begingroup\$ What is risk value? It's making the question nearly unanswerable \$\endgroup\$ – Pimgd Dec 12 '18 at 14:30
  • \$\begingroup\$ @Pimgd: Risk value is given differently per product and user, as part of premium quote calculation. More information has been added. \$\endgroup\$ – Anna Dec 12 '18 at 15:05
1
\$\begingroup\$

Your model classes are also doing the calculating and that hurts.

I've been trying to understand the code for a while now and that's the main point I keep coming up with.

@Override
public BigDecimal getRiskSurcharge() {
    if(this.userRiskValue > 200)
        return Risk.getUnIdentifiedRiskSurcharge();

    final int minAcceptableBlackSpots = 3;
    final int maxAcceptableBlackSpots = 12;

    if(Risk.isWithinRange(this.noOfBlackSpots, minAcceptableBlackSpots, maxAcceptableBlackSpots))
        return Risk.getDefaultRiskSurcharge(); 

    return Risk.getUnIdentifiedRiskSurcharge();
}

This does not belong in a Banana.

This is your business logic. It goes in the processor or parts thereof.

When I look at the Processor itself, then I also see something strange:

public BigDecimal calculateSingleProductPremiumForUser() {
    BigDecimal userRiskSurcharge = user.getRiskSurcharge();
    if(Risk.getUnIdentifiedRiskSurcharge().compareTo(userRiskSurcharge)==0)
        return Risk.getUnIdentifiedRiskSurcharge();

    this.product.setRiskValue(this.user.getRiskValue());
    BigDecimal productRiskSurcharge =this.product.getRiskSurcharge();
    if(Risk.getUnIdentifiedRiskSurcharge().compareTo(productRiskSurcharge)==0)
        return Risk.getUnIdentifiedRiskSurcharge();

    if(Risk.getDefaultRiskSurcharge().compareTo(productRiskSurcharge)==0)
        return this.product.getSubTotalPremium();

    BigDecimal totalPremium = this.product.getSubTotalPremium().multiply(this.product.getRiskSurcharge()).multiply(user.getRiskSurcharge());
    return totalPremium;
}

Given a User (which was provided via the constructor) and a Product (which may be null)...

Actually, let's stop there for a moment.

Why can product be null?

Here's one of your tests, with the code of functions inlined.

@Test
public void testUnInsurableBananaRiskSurchargeWithUserRiskValueLargerThan200() {
    Banana banana = new Banana(1, new BigDecimal(10));
    Address address = new Address(1);
    address.setRiskValue(10);
    User user = new User(1, address);
    user.setRiskValue(201);

    banana.setRiskValue(user.getRiskValue());
    banana.setNoOfBlackSpots(4);

    BigDecimal totalPremium = UserPremiumBuilder
        .createProfile(user)
        .setProduct(banana)
        .calculateSingleProductPremiumForUser();
    assertEquals(Risk.getUnIdentifiedRiskSurcharge(), totalPremium);
}

Let's look at it from this perspective.

The Banana constructor takes an id and a product value.

We need a User with an id and address, and the address also has an id and a risk value.

Then we set the user's risk value, which is something like a credit score but for insurances.

Next, we place the risk value of the user in the banana.

That part makes no sense to me.

Next, set amount of black spots for the banana (which is just part of test setup again).

Then, using the UserPremiumBuilder, create a profile, set a product, and calculate the product premium.


I'll describe the issues I have with the ideas presented in the unit test, and then draft my own interface. Afterward, I'll explain to you why I think they are a better fit for the problem domain.

I can imagine there's an insurance company that has statistics for things. Given an address, it can find how much "bonus" it should apply. Furthermore, given some kind of history, people have scores, and these scores also define some sort of bonus. Lastly, depending on the type, different restrictions and calculations may apply. For instance, refusing to insure any car older than 40 years, or any house which has not been inspected.

It's with that imagination that I'll build my model classes and interfaces.

A customer rings up the company. They provide their customer id. Their score is retrieved from some database. Also retrieved from the database is their address, which has been given a score by our automatic scoring system.

Address address = new Address(1, 10); //ID, Risk value
User user = new User(1, address, 201); //ID, address, risk value

Since we can't service customers properly if we don't know their scores or where they live (or what the score is for where they live), all of this is required in the constructor.

The customer says they have a banana they wish to insure (it's very important to them). We ask them for some of the details and register the banana in our database. However, if that's the logic we follow, I'd expect a product to have a reference to a user (namely the "owner" of the product). We'll ignore that for now, I guess a Banana requires an id for SOME reason, if only due to business requirements.

Banana banana = new Banana(1, new BigDecimal(10), 4); //ID, value, black spots

The customer says they'd only want to insure this one banana and if we could provide them with a quote.

So, we enter into the calculator - our user (with their address) and our banana.

BigDecimal totalPremium = PremiumCalculator.calculateFor(user, banana); //User user, Product[] products...

We now have a total premium which we can use.

The full code for this section:

Address address = new Address(1, 10); //ID, Risk value
User user = new User(1, address, 201); //ID, address, risk value
Banana banana = new Banana(1, new BigDecimal(10), 4); //ID, value, black spots

BigDecimal totalPremium = PremiumCalculator.calculateFor(user, banana); //User user, InsurableProduct[] products...

I did hide some of the details, though.

I'm sure it's fine if I skip over the part where I redefined constructors for Address, User and Banana.

I think I should explain at least part of the interfaces:

public interface InsurableProduct {
    BigDecimal getProductValue();
    default BigDecimal getBasePremiumMultiplier() {
        return BigDecimal.ONE;
    }
    default BigDecimal getRiskSurchargeMultiplier() {
        return BigDecimal.ONE;
    }
    boolean canBeInsuredFor(User user);
}

This is an insurable product. It has a base value, which you can multiply by the premium multiplier to get a sub total premium, which you can multiply by the risk surcharge multiplier to get the final price.

It also has a "canBeInsuredFor" method; this is to specify logic like "don't insure bananas that have a million black spots". It also takes a user, since for some products, we might say "If the user is living in a high risk area, we'll refuse to insure your $100k car".

Okay, so how does this "PremiumCalculator" work?

public class PremiumCalculator {
    private PremiumCalculator() { 
        //No instantiation required/allowed
    }
    public static BigDecimal calculateFor(User user, InsurableProduct[] products...) {
        BigDecimal total = BigDecimal.ZERO;
        for (InsurableProduct product : products) {
              if (!product.canBeInsuredFor(user)) { 
                  return Risk.getUnIdentifiedRiskSurcharge(); //Left this in for now
              }
              BigDecimal productValue = product.getProductValue();
              BigDecimal productSubtotal = productValue.multiply(product.getBasePremiumMultiplier());
              BigDecimal productPremium = productSubtotal.multiply(product.getRiskSurchargeMultiplier());
              total = total.add(productPremium);
        }
        return total.multiply(user.getRiskSurcharge());
    }
}

It takes the user and products, and if any of them in uninsurable, it returns the unidentified risk surcharge value - your "error" value. I don't like how it's handled, but I don't have a better solution yet.

Start the total off at zero, and then, for each product, calculate the productPremium via value * basePremiumMultiplier * riskSurchargeMultiplier. Then add to the total.

Finally, multiply the total by the users's risk surcharge.

This places your calculations in a single class.

As for the implementation of your Banana:

public class Banana implements InsurableProduct {
    private static BigDecimal BASE_PREMIUM = new BigDecimal(1.15);

    private final int id;
    private final BigDecimal productValue;
    private final int noOfBlackSpots;

    public Banana(int id, BigDecimal productValue, int noOfBlackSpots) {
        this.id = id;
        this.productValue = productValue;
        this.noOfBlackSpots = noOfBlackSpots;
    }

    public int getId() {
        return id;
    }

    public int getNoOfBlackSpots() {
        return noOfBlackSpots;
    }

    @Override
    public BigDecimal getProductValue() {
        return productValue;
    }

    @Override
    public BigDecimal getBasePremiumMultiplier() {
        return BASE_PREMIUM;
    }

    @Override
    public boolean canBeInsuredFor(User user) {
        final int minAcceptableBlackSpots = 3;
        final int maxAcceptableBlackSpots = 12;

        return Risk.isWithinRange(noOfBlackSpots, minAcceptableBlackSpots, maxAcceptableBlackSpots) && user.getRiskValue() <= 200;
    }

    @Override
    public BigDecimal getRiskSurchargeMultiplier() {
        return Risk.getDefaultRiskSurcharge(); 
    }
}

Some of this might be extractable to a base class; I don't care about that right now.


What I have done is decouple "can be insured" and "can be insured for", for products. I have also migrated the User's risk value to be part of the user, and not individually set, first.

By relocating the calculations to the calculator, the only thing the products still contain are the actual rules. If we remove the getters and setters from the Banana, we're left with this:

public class Banana implements InsurableProduct {
    public final int id;
    public final BigDecimal productValue;
    public final int noOfBlackSpots;

    public Banana(int id, BigDecimal productValue, int noOfBlackSpots) {
        this.id = id;
        this.productValue = productValue;
        this.noOfBlackSpots = noOfBlackSpots;
    }

    @Override
    public BigDecimal getProductValue() {
        return productValue;
    }

    @Override
    public BigDecimal getBasePremiumMultiplier() {
        return new BigDecimal(1.15);
    }

    @Override
    public boolean canBeInsuredFor(User user) {
        final int minAcceptableBlackSpots = 3;
        final int maxAcceptableBlackSpots = 12;

        return Risk.isWithinRange(noOfBlackSpots, minAcceptableBlackSpots, maxAcceptableBlackSpots) && user.getRiskValue() <= 200;
    }

    @Override
    public BigDecimal getRiskSurchargeMultiplier() {
        return Risk.getDefaultRiskSurcharge(); 
    }
}

(I've made the variables public so you could still retrieve them).

In essence, this is all the real coding required. Three of these methods are simple getters still, the last one contains your business logic, and even then it's small and to the point.

Further more, you'll notice that the whole use of "mutation" is gone. There are no setters. A Banana is immutable. You want to change something? Get a new Banana. Whether that meets the business needs remains to be seen. But for a quote-creating application, if you don't need to be able to modify it, then you can't modify it.

I believe this should showcase enough on how to create a better API (you must provide all values, you can't forget something) and how to reduce mutation.

\$\endgroup\$
  • \$\begingroup\$ Thank you! Your design looks much better than mine. One question: you said it’s not good to put logic in model; so where should I put the calculation logic for each type of product in this example that you could suggest? And do we always avoid putting (business) logic in the models? If so, could you share the reason why? \$\endgroup\$ – Anna Dec 18 '18 at 8:48
  • \$\begingroup\$ @Anna It's not good to put logic in the model because if you have another model that you want to apply the same (or mostly the same) logic too, you now have to duplicate the logic, which is not ideal. Basically, you're trying to separate concerns (google separation of concerns). If you later have to deal with persistence to a database, or transformation of entities to a DTO, or other manipulations, you want the smallest models you can get away with, because it reduces your complexity and test setup. \$\endgroup\$ – Pimgd Dec 18 '18 at 9:47

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.