Small shopping cart application - Solid Principles and Clean Code

Question

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.

Discountwould 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

• Setris: I would also suggest editing your post to include a complete list of business rules (involving prices/discounts/loyalty points/etc) or assumptions that the interviewers provided to you over the course of the interview. Otherwise, any potential reviewer here will have to extrapolate/guess at what those rules are based on the original pre-refactor code which could have a big influence on the suggested design.
• Laila Campos: I've edited the question to include business rules and the definition for Discount and DiscountPercentage classes. Please check whether this is better or if there's still any missing information. And thank you for the suggestion.
• Laila Campos: Yes, there's a main function. I forgot to add it to the question, sorry. I've since edited my question to include it.
• Reinderien: If not from the same interview, then at least very very similar to this question

Answer 1

I see no need for abstract classes here - there are no alternate implementations.

This string:

" Name: %s \n Price: %s \n" % (self.name, self.price)

has too much internal whitespace, and would be better as an interpolated f-string.

There is no need for the Customer class - that can just be a name string.

"Will receive %s" is a little confusing, and would be clearer as "Will receive %s points".

Your discount selection code is repeated and should be moved to a single place.

Add PEP484 type hints.

I see no need for the Handler and Calculator classes. The current application's business logic complexity level (not a lot) does not call for them. These can be demoted to functions or methods on other classes.

Consider using locale.currency for currency formatting.

You would benefit from making more of your classes immutable - NamedTuple is one easy way. The only class in your current implementation that needs to mutate is the ShoppingCart.

Is it a bad practice to have so many static methods?

It's a smell, and in this case yes, it calls for refactoring and collapse of many of your classes down to simpler imperative code.

Don't use [] as an argument default - it's mutable, and since all defaults are referentially equal that has nasty consequences. One common way around this is to make an Optional that's filled in during the method.

Suggested

One way of simplifying while retaining the same logic.

from locale import currency, LC_ALL, setlocale
from typing import Optional, NamedTuple
setlocale(LC_ALL, '')
class Product(NamedTuple):
 price: float
 product_code: str
 name: str
 def __str__(self) -> str:
 return f"Name: {self.name}\nPrice: {currency(self.price)}"
class Order(NamedTuple):
 points_earned: int
 total: float
 def __str__(self) -> str:
 return (
 f"Total price: {currency(self.total)}"
 f"\nWill receive {self.points_earned} points"
 )
class Discount(NamedTuple):
 product: Product
 point_factor: float
 discount_factor: float
 @classmethod
 def for_product(cls, product: Product) -> 'Discount':
 for discount in (15, 10):
 if product.product_code.startswith(f"DIS_{discount}"):
 break
 else:
 discount = 5
 if discount > 5:
 points = 1/discount
 else:
 points = 0
 return cls(
 product=product,
 point_factor=points,
 discount_factor=1/discount,
 )
 @property
 def points(self) -> float:
 return self.product.price * self.point_factor
 @property
 def price(self) -> float:
 return self.product.price * (1 - self.discount_factor)
class ShoppingCart:
 def __init__(self, customer_name: str, products: Optional[list[Product]] = None) -> None:
 if products is None:
 products = []
 self.products = products
 self.customer_name = customer_name
 def add_product(self, product: Product) -> None:
 self.products.append(product)
 def checkout(self) -> Order:
 total_price = 0
 points = 0
 for product in self.products:
 discount = Discount.for_product(product)
 total_price += discount.price
 points += discount.points
 return Order(round(points), total_price)
 def __str__(self) -> str:
 product_list = "\n".join(str(product) for product in self.products)
 return f"Customer: {self.customer_name}\nBought:\n{product_list}"
def main() -> None:
 products = [
 Product(10, "DIS_10_PRODUCT1", "product 1"),
 Product(20, "DIS_10_PRODUCT2", "product 2"),
 ]
 customer = "A Customer"
 shopping_cart = ShoppingCart(customer, products)
 product3 = Product(30, "DIS_10_PRODUCT3", "product 3")
 shopping_cart.add_product(product3)
 print(shopping_cart)
 order = shopping_cart.checkout()
 print(order)
if __name__ == "__main__":
 main()

Output

Customer: A Customer
Bought:
Name: product 1
Price: 10ドル.00
Name: product 2
Price: 20ドル.00
Name: product 3
Price: 30ドル.00
Total price: 54ドル.00
Will receive 6 points