I've recently had a technical interview where I had to identify code smells and refactor an existing code by applying the SOLID principles and implementing unit tests.
It is a small shopping cart application with a small set of eCommerce/shopping cart rules. Rules include calculation of total price, discount and loyalty points calculation.
Existing Business Rules:
Calculates total price and total loyalty points earned by the customer.
Products with product code starting with DIS_10 have a 10% discount applied.
Products with product code starting with DIS_15 have a 15% discount applied.
Loyalty points are earned more when the product is not under any offer.
- Customer earns 1 point on every $5 purchase.
- Customer earns 1 point on every $10 spent on a product with 10% discount.
- Customer earns 1 point on every $15 spent on a product with 15% discount.
Most of the business logic was implemented in single method ShoppingCart.checkout
.
Here's the whole application before refactoring:
Models:
class Product:
def __init__(self, price, product_code, name):
self.price = price
self.product_code = product_code
self.name = name
def __str__(self):
return " Name: %s \n Price: %s \n" % (self.name, self.price)
class Customer:
def __init__(self, name):
self.name = name
def __str__(self):
return self.name
class Order:
def __init__(self, loyalty_points_earned=0, total_price=0):
self.loyalty_points = loyalty_points_earned
self.total = total_price
def __str__(self):
return "Total price: %s \nWill receive %s" % (self.total, self.loyalty_points)
ShoppingCart:
class ShoppingCart:
def __init__(self, customer=Customer, products=[]):
self.products = products
self.customer = customer
def add_product(self, product):
self.products.append(product)
def checkout(self):
total_price = 0.00
loyalty_points_earned = 0.00
for product in self.products:
discount = 0.00
if product.product_code.startswith("DIS_10"):
loyalty_points_earned += (product.price / 10)
discount = product.price * 0.1
elif product.product_code.startswith("DIS_15"):
loyalty_points_earned += (product.price / 15)
discount = product.price * 0.15
else:
loyalty_points_earned += (product.price / 5)
discount = 0.00
total_price += product.price - discount
return Order(int(loyalty_points_earned), total_price)
def __str__(self):
product_list = "".join('%s'%product for product in self.products)
return "Customer: %s \nBought: \n%s" % (self.customer, product_list)
Running the code:
def main():
product1 = Product(10.0, "DIS_10_PRODUCT1", "product 1")
product2 = Product(20.0, "DIS_10_PRODUCT2", "product 2")
products = [product1, product2]
customer = Customer("A Customer")
shopping_cart = ShoppingCart(customer, products)
product3 = Product(30.0, "DIS_10_PRODUCT3", "product 3")
shopping_cart.add_product(product3)
print(shopping_cart)
order = shopping_cart.checkout()
print(order)
if __name__ == "__main__":
main()
Based on the code above, I've decided to create three new classes: DiscountHandler
, LoyaltyPointsHandler
and PriceCalculator
.
Here's my code (the models remain the same):
LoyaltyPointsHandler:
class LoyaltyPointsHandler:
@staticmethod
def calculate_loyalty_points_earned(product):
if product.product_code.startswith("DIS_10"):
return LoyaltyPointsHandler.earn_loyalty_points_with_DIS_10_code(product)
elif product.product_code.startswith("DIS_15"):
return LoyaltyPointsHandler.earn_loyalty_points_with_DIS_15_code(product)
else:
return LoyaltyPointsHandler.earn_loyalty_points_with_no_discount(product)
@staticmethod
def earn_loyalty_points_with_no_discount(product):
loyalty_points_earned = product.price / 5
return loyalty_points_earned
@staticmethod
def earn_loyalty_points_with_DIS_10_code(product):
loyalty_points_earned = product.price / 10
return loyalty_points_earned
@staticmethod
def earn_loyalty_points_with_DIS_15_code(product):
loyalty_points_earned = product.price / 15
return loyalty_points_earned
DiscountHandler:
class DiscountHandler(Discount):
@staticmethod
def calculate_discount(product):
if product.product_code.startswith("DIS_10"):
return DiscountHandler.calculate_10_per_cent_discount(product)
elif product.product_code.startswith("DIS_15"):
return DiscountHandler.calculate_15_per_cent_discount(product)
else:
return 0.00
@staticmethod
def calculate_10_per_cent_discount(product):
discount = product.price * 0.1
return discount
@staticmethod
def calculate_15_per_cent_discount(product):
discount = product.price * 0.15
return discount
PriceCalculator:
class PriceCalculator:
@staticmethod
def calculate_price_single_product_percentage_discount(product):
final_price = product.price - DiscountHandler.calculate_discount(product)
return final_price
ShoppingCart:
class ShoppingCart:
def __init__(self, customer=Customer, products=[]):
self.products = products
self.customer = customer
def add_product(self, product):
self.products.append(product)
def checkout(self):
total_price = 0.00
loyalty_points_earned = 0.00
for product in self.products:
loyalty_points_earned += LoyaltyPointsHandler.calculate_loyalty_points_earned(product)
total_price += PriceCalculator.calculate_price_single_product_percentage_discount(product)
return Order(int(loyalty_points_earned), total_price)
def __str__(self):
product_list = "".join('%s'%product for product in self.products)
return "Customer: %s \nBought: \n%s" % (self.customer, product_list)
There was no time to show my full implementation to the person who was interviewing me and at the end of the interview I was asked what approach would I take if I had to implement a new type of discount, like the buy X and take Y home for example.
We talked a bit and I told them that maybe I'd create an interface called Discount
with an abstract method calculate_discount
and have the classes DiscountPercentage
and DiscountBuyXTakeY
implement it.
Discount
would be like such:
class Discount (ABC):
@staticmethod
@abstractmethod
def calculate_discount(product):
pass
DiscountPercentage would be like such:
class DiscountPercentage(Discount):
@staticmethod
def calculate_discount(product):
if product.product_code.startswith("DIS_10"):
return DiscountPercentage.calculate_10_per_cent_discount(product)
elif product.product_code.startswith("DIS_15"):
return DiscountPercentage.calculate_15_per_cent_discount(product)
else:
return 0.00
@staticmethod
def calculate_10_per_cent_discount(product):
discount = product.price * 0.1
return discount
@staticmethod
def calculate_15_per_cent_discount(product):
discount = product.price * 0.15
return discount
But then how would I implement DiscountBuyXTakeY
?
Because its calculate_discount
method would need the number of products being purchased. But if I pass it as a parameter then the method signature would differ than that of the parent class and wouldn't it violate Liskov's Substitution principle then?
I also have the following questions:
- What could I have done better in the way of refactoring the code so it conforms to the SOLID principles? Was my approach good? Could I have gone further?
- And what about my approach at implementing this new discount type? Was I in the right track at the very least?
- Is it a bad practice to have so many static methods?
Thank you in advance
Discount
andDiscountPercentage
classes. Please check whether this is better or if there's still any missing information. And thank you for the suggestion. \$\endgroup\$main
function. I forgot to add it to the question, sorry. I've since edited my question to include it. \$\endgroup\$