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

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