How can I refactor this code?

[Paolo Cartella]

Here is what I am trying to do:

In this code there are products and discounts.

The discounts can be of various types.

Two specific types are:

-Buy X, get Y free
-Amount A off when buying quantity Q

What we know:

-List of products
-List of discounts
-Quantity and type of product the customer wants to buy

What we are interested in finding out:

-Which discount provides the biggest savings taking all of the products into account, in aggregate?
-What is the total saving with that discount?
-What is the total price of the customers' chosen products (taking quantities into account)?
-What is the discounted total price?

I have a feeling my code smells, but I can't pinpoint why exactly. I seem to be using a lot of static methods. Also, I don't like how the Buy X, get Y free discount needs the price of the product applied because that couples it to the specific product, while Amount A off when buying Quantity Q can be more generic.

Here is the source code:

Here are the classes:

Controller

package discounts;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

import helper.*;
import model.Info;
import model.Product;

public class Controller {

	public static void main(String[] args) {
		
		int[] quantities = {4, 5, 6, 7};
		Info info = calculate(quantities);
		
		System.out.println(info);
		
	}

	protected static Info calculate(int[] quantities) {

		List<Product> products = ProductsHolder.getProducts();

		Products.updateProductsWithQuantities(products, quantities);
		
		Map<String, Double> totalSavingsByDiscountName = new HashMap<>();
		
		totalSavingsByDiscountName = Discounts.calculateTotalSavingsByDiscountName(products);
		
		String chosenDiscount = Discounts.chooseBestDiscount(totalSavingsByDiscountName);
		
		double totalPrice = 0;
		
		for (Product product : products) {
			totalPrice += product.getPrice() * product.getQuantity();
		}

		double priceWithDiscount = totalPrice - totalSavingsByDiscountName.get(chosenDiscount);

		Info info = new Info(totalPrice, priceWithDiscount, chosenDiscount);
		
		return info;

	}
}

DiscountsHolder

package discounts;

import java.util.Arrays;
import java.util.List;

import model.Discount;
import model.Product;
import model.AmountPerQuantityDiscount;
import model.QuantityPerQuantityDiscount;

public class DiscountsHolder {

	public static List<Discount> getDiscounts(Product product) {

		return Arrays.asList(new QuantityPerQuantityDiscount(1, 3, product.getPrice(), "2=3"),
							new AmountPerQuantityDiscount(6000, 12, "-6000 for 12"));
	}

}

ProductsHolder

package discounts;

import java.util.Arrays;
import java.util.List;

import model.Product;

public class ProductsHolder {
	
	public static List<Product> getProducts() {
		return Arrays.asList(new Product("Fabergé Egg", 2000),
							new Product("Rubber Duck", 3000),
							new Product("-6000 Star Destroyer", 2800),
							new Product("-6000 Smartwatch Du Jour", 1000));
	}
	
}

Discounts

package helper;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import discounts.DiscountsHolder;
import model.AmountPerQuantityDiscount;
import model.Discount;
import model.Product;
import model.QuantityPerQuantityDiscount;

public class Discounts {

	public static Map<Discount, Double> calculateAmounts(Product product) {
		
		Map<Discount, Double> aggregate = new HashMap<>();
		
		List<Discount> discounts = DiscountsHolder.getDiscounts(product);
		List<Discount> discountsForThisProduct = assign(discounts, product);

		for (Discount discount : discountsForThisProduct) {
			
			int discountQty = 0;
			
			if (discount instanceof QuantityPerQuantityDiscount) {
				discountQty = product.getQuantity() / discount.getDivisor() * ((QuantityPerQuantityDiscount)discount).getDividend();
			} else if (discount instanceof AmountPerQuantityDiscount) {
				discountQty = product.getQuantity() / discount.getDivisor();
			}
			
			double amount = discountQty * discount.getValue();
			
			aggregate.put(discount, amount);
		}
		
		return aggregate;
	}
	
	public static String chooseBestDiscount(Map<String, Double> totalSavingsByDiscountName) {
		
		String picked = "";
		double chosenDiscountAmount = 0;
		
		for (Map.Entry<String, Double> totalSavingByDiscountName : totalSavingsByDiscountName.entrySet()) {
			if (totalSavingByDiscountName.getValue() > chosenDiscountAmount) {
				picked = totalSavingByDiscountName.getKey();
				chosenDiscountAmount = totalSavingByDiscountName.getValue();
			}
		}
		
		//If no discounts
		if (chosenDiscountAmount == 0) {
			totalSavingsByDiscountName.put("None", 0.0);
			picked = "None";
		}
		
		return picked;
	}
	
	public static Map<String, Double> calculateTotalSavingsByDiscountName(List<Product> productsWithQuantities) {
		
		Map<String, Double> totalSavingsByDiscountName = new HashMap<>();
		
		for (Product p : productsWithQuantities) {
			
			Map<Discount, Double> savingsByDiscount = calculateAmounts(p);
			
			for (Map.Entry<Discount, Double> savingByDiscount : savingsByDiscount.entrySet()) {
			
				double value = 0.0;
				Discount currentDiscount = savingByDiscount.getKey();
				
				if (totalSavingsByDiscountName.containsKey(currentDiscount.getName())) {
					value = totalSavingsByDiscountName.get(currentDiscount.getName());
				}
				
				value += savingByDiscount.getValue();
				totalSavingsByDiscountName.put(currentDiscount.getName(), value);
			}
		}
		
		return totalSavingsByDiscountName;
	}
	
	private static List<Discount> assign(List<Discount> discounts, Product product) {
		
		List<Discount> assignedDiscounts = new ArrayList<>();
		
		for (Discount discount : discounts) {
			if (discount.getName().startsWith("-6000") && product.getName().startsWith("-6000")) {
				assignedDiscounts.add(discount);
			} else if (!discount.getName().startsWith("-6000") && !product.getName().startsWith("-6000")) {
				assignedDiscounts.add(discount);
			}
		}
		
		return assignedDiscounts;
		
	}
	
}

Products

package helper;

import java.util.List;
import model.Product;

/**
 * Helper class for @link{Product}-related operations
 *
 */
public class Products {

	/**
	 * Updates a list of products with the requested quantities.
	 */
	public static void updateProductsWithQuantities(List<Product> modifiedProducts, int[] quantities) {
		for (int i = 0; i < modifiedProducts.size(); i++) {
			Integer quantity = 0;
			try {
				quantity = Integer.valueOf(quantities[i]);
			} catch (NumberFormatException nfe) {
				//Treat as 0
			} finally {
				modifiedProducts.get(i).setQuantity(quantity);
			}
		}
	}
}

Discount

package model;

public abstract class Discount {

    private int divisor;
    private double value;
    String name;
    
	public String getName() {
		return name;
	}
	public void setName(String newName) {
		this.name = newName;
	}
	Discount(int divisor, double value, String newName) {
        this.divisor = divisor;
        this.value = value;
        this.name = newName;
    }
	public int getDivisor() {
		return divisor;
	}
	public double getValue() {
		return value;
	}
	
	public void setDivisor(int newDivisor) {
		this.divisor = newDivisor;
	}
	
	public void setValue(double newValue) {
		this.value = newValue;
	}
	
}

AmountPerQuantityDiscount

package model;

/*
 * A class for an amount-per-quantity discount, such as 6000 USD/12 pieces.
 */
public class AmountPerQuantityDiscount extends Discount {

	public AmountPerQuantityDiscount(int newValue, int newDivisor, String newName) {
		super(newDivisor, newValue, newName);
	}
	
	public String toString() {
		return String.format("Name: %s, Divisor: %d, Value: %d", this.getName(), this.getDivisor(), this.getValue());
	}
	
}

QuantityPerQuantityDiscount

package model;

/*
 * A class for a quantity-per-quantity discount, such as 1 per 3.
 */
public class QuantityPerQuantityDiscount extends Discount {
	private int dividend;
	
	public QuantityPerQuantityDiscount(int newDividend, int newDivisor, double price, String newName) {
		super(newDivisor, price, newName);
		this.dividend = newDividend;
	}
	
	public int getDividend() {
		return this.dividend;
	}
	
	public String toString() {
		return String.format("Name: %s, Dividend: %d, Divisor: %d, Value: %.2f", this.getName(), this.getDividend(), this.getDivisor(), this.getValue());
	}

}

Info

package model;

public class Info {

	private double totalPrice;
	private double discountedPrice;
	private String chosenType;
	
	public Info(double newTotalPrice, double newDiscountedPrice, String newChosenType) {
		this.totalPrice = newTotalPrice;
		this.discountedPrice = newDiscountedPrice;
		this.chosenType = newChosenType;
	}
	
	public double getTotalPrice() {
		return totalPrice;
	}
	
	public void setTotalPrice(double totalPrice) {
		this.totalPrice = totalPrice;
	}
	
	public double getDiscountedPrice() {
		return discountedPrice;
	}
	
	public void setDiscountedPrice(double discountedPrice) {
		this.discountedPrice = discountedPrice;
	}
	
	public String getChosenType() {
		return chosenType;
	}
	
	public void setChosenType(String chosenType) {
		this.chosenType = chosenType;
	}
	
	public String toString() {
		return String.format("Total price:%20.2f\tDiscounted price:%20.2f\tChosen type:%20s", totalPrice, discountedPrice, chosenType);
	}
	
}

Product

package model;

public class Product {

	private String name;
	private double price;
	private int quantity;
	
	public int getQuantity() {
		return quantity;
	}

	public void setQuantity(int newQuantity) {
		this.quantity = newQuantity;
	}

	public Product(String newName, double newPrice) {
		this.name = newName;
		this.price = newPrice;
	}
	
	public Product(Product otherProduct) {
			this.name = otherProduct.getName();
			this.price = otherProduct.getPrice();
	}

	public String getName() {
		return name;
	}
	public double getPrice() {
		return price;
	}

	@Override
	public String toString() {
		return String.format("Name: %s, Price: %.2f, Quantity: %d", this.getName(), this.getPrice(), this.getQuantity());
	}
	
}

[Paolo Cartella]

Here is the source code: https://nofile.io/f/WKDb1pX1zcS/src.zip

[Junilu Lacar]

You're right, your code smells from top to bottom. The first problem I see is that it's not object-oriented. That's why you're using a lot of static methods, because the perspective you have when you wrote the code is that you, the programmer, are in control of everything that's happening. I like to call it the "Wizard of Oz" perspective where you're the man behind the curtain, pulling all the levers and controlling every little thing that happens. Good object-oriented programs written from the proper perspective read like a story, where each object has its role and responsibilities and all you need to do is create the objects and connect them to each other, then start a chain reaction by telling one object to do something. That object then works with other objects to do more things until finally, the collection of objects have done each of their "things" to contribute to a larger goal.

Here's an example of code that's not object-oriented:
       List<Product> products = ProductsHolder.getProducts();        Products.updateProductsWithQuantities(products, quantities);

Here's the first step of refactoring (not the last step, mind you) that can be done to make the perspective more object-oriented:

       List<Product> products = ProductsHolder.getProducts();        Iterator it = products.iterator();        for (int count: quantities) {           if (it.hasNext()) {              it.next().setQuantity(count);           } else {              // not enough products              break;           }        }

The next step would be to ask where lines 25-34 above belong, which object/class should be responsible for doing this. The answer is NOT the ProductsHolder class. That will probably violate the Single Responsibility Principle (SRP).

There are many more problems in your code but again, the main problem is that the code is written with a more procedural mindset rather than an object-oriented one.

[Junilu Lacar]

Here's another problem. Your Product class has a quantity field. Why? It may seem convenient to have the quantity field in Product but the name suggests a more general and long-term nature whereas a quantity is a temporal attribute, that is, it will change over time. Additionally, the quantity of a product is more closely related to a purchase, specifically, a line item in a specific purchase or transaction.  I would look to refactor product by separating attributes that are more permanent/long-term (like name, description) from the ones that are temporal and short-term (price, quantity).

[Junilu Lacar]

AmountPerQuantityDiscount and QuantityPerQuantityDiscount are strategies or rules that can be applied in calculating a discount.  Read up on the Strategy pattern to see how you can refactor those two classes so that they follow the Strategy pattern.

[Junilu Lacar]

Junilu Lacar wrote:
The next step would be to ask where lines 25-34 above belong, which object/class should be responsible for doing this. The answer is NOT the ProductsHolder class. That will probably violate the Single Responsibility Principle (SRP).

That answer is NOT the Products class either. That whole class is smelly and needs to go away.  My first inclination would be to explore the concept of a "purchase" or "order" which might have a list products to be purchased, the quantities of each product, and the kind of discount to be applied.

[Paolo Cartella]

Thanks for the feedback.

You're right. There's definitely value in introducing a Cart/Order/Transaction type object.

Here's what I've done so far:

-Realized that almost no code should remain in Controller. It should simply trigger a calculation, then return the info we need, no other responsibilities.
-Introduce an Order class. This class receives the list of products and quantities and maintains a map of line items with Product <-> Quantity.
-Moved all calculations from Discounts into Order, as Order has good visibility of the products and quantities.
-Realized that this gives Order too much responsibility. Introduced a DiscountCalculator object and gave it those discount-related calculations. An Order has-a DiscountCalculator.
-Renamed Info to OrderInfo. An Order has-an OrderInfo. (To simplify further, I could just get rid of this altogether and return the processed Order.)
-Attempted to introduce the Strategy pattern. Discount is now more flexible, but in my opinion somewhat kludgy. Look in DiscountsHolder where we create new Discounts to see what I mean. I did not see the value in keeping the two discount type classes around _and_ introduce strategies. But I've never used this pattern before.
-Created Strategies for calculating the two types of discounts we have so far.

Current setup:

Controller

package discounts;

import model.OrderInfo;
import model.Order;

public class Controller {

	public static void main(String[] args) throws Exception {
		
		int[] quantities = {1, 9, 6, 12};
		
		OrderInfo info = calculate(quantities);
		
		System.out.println(info);
		
	}

	protected static OrderInfo calculate(int[] quantities) throws Exception {

		Order order = new Order(quantities);

		return order.getInfo();

	}
}

DiscountsHolder

package discounts;

import java.util.Arrays;
import java.util.List;

import model.Discount;
import model.Product;
import model.AmountPerQuantityStrategy;
import model.QuantityPerQuantityStrategy;

public class DiscountsHolder {

	public static List<Discount> getDiscounts(Product product, int quantity) {

		Discount d1 = new Discount(12, 0, 6000, quantity, 0, "-6000 for 12"); //0 as dividend, 0 as product price - not needed
		d1.setStrategy(new AmountPerQuantityStrategy());

		Discount d2 = new Discount(1, 3, 0, quantity, product.getPrice(), "2=3"); //0 as value - not needed
		d2.setStrategy(new QuantityPerQuantityStrategy());

		return Arrays.asList(d1, d2);
	}

}

ProductsHolder, Product

Unchanged.

Discount

package model;

public class Discount {

	private int divisor;
	private int dividend;
	private int productQuantity;
	
	private double value;
	private double productPrice;
	
	String name;
	
	private DiscountStrategy strategy;

	public Discount(int newDivisor, int newDividend, double newValue, int newProductQuantity, double newProductPrice, String newName) {
		this.divisor = newDivisor;
		this.value = newValue;
		this.productQuantity = newProductQuantity;
		this.name = newName;
		this.dividend = newDividend;
		this.productPrice = newProductPrice;
	}
	
	public double getProductPrice() {
		return productPrice;
	}

	public void setProductPrice(double productPrice) {
		this.productPrice = productPrice;
	}

	public void setStrategy(DiscountStrategy newStrategy) {
		this.strategy = newStrategy;
	}

	public int getProductQuantity() {
		return productQuantity;
	}
	
	public void setProductQuantity(int productQuantity) {
		this.productQuantity = productQuantity;
	}
	
	public String getName() {
		return name;
	}
	
	public void setName(String newName) {
		this.name = newName;
	}

	public int getDivisor() {
		return divisor;
	}
	public double getValue() {
		return value;
	}

	public void setDivisor(int newDivisor) {
		this.divisor = newDivisor;
	}

	public void setValue(double newValue) {
		this.value = newValue;
	}

	public double calculateAmount() {
		return strategy.calculate(this);
	}

	public int getDividend() {
		return dividend;
	}

	public void setDividend(int dividend) {
		this.dividend = dividend;
	}

}

DiscountStrategy

package model;

public interface DiscountStrategy {
	double calculate(Discount discount);
}

AmountPerQuantityStrategy

package model;

/*
 * A strategy for an amount-per-quantity discount, such as 6000 USD/12 pieces.
 */
public class AmountPerQuantityStrategy implements DiscountStrategy {
	
	@Override
	public double calculate(Discount discount) {
		return discount.getProductQuantity() / discount.getDivisor() * discount.getValue();
	}	
}

QuantityPerQuantityStrategy

package model;

/*
 * A class for a quantity-per-quantity discount, such as 1 per 3.
 */
public class QuantityPerQuantityStrategy implements DiscountStrategy {

	@Override
	public double calculate(Discount discount) {
		return discount.getProductQuantity() / (discount.getDivisor() * discount.getDividend()) * discount.getProductPrice();
	}

}

Order

package model;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

import discounts.ProductsHolder;

public class Order {

	Map<Product, Integer> lineItems;
	OrderInfo orderInfo;
	DiscountCalculator discountCalculator;
	
	public Order(int[] quantities) throws Exception {
		
		orderInfo = new OrderInfo();
		
		List<Product> products = ProductsHolder.getProducts();
		lineItems = new HashMap<>();
		
		if (products.size() != quantities.length) {
			throw new Exception("Something blew up!");
		} else {
			for (int i = 0; i < quantities.length; i++) {
				lineItems.put(products.get(i), quantities[i]);
			}
		}
		
		discountCalculator = new DiscountCalculator(lineItems);
		
		orderInfo.setTotalPrice(getTotalPrice());
		orderInfo.setChosenType(discountCalculator.getBestDiscount());
		orderInfo.setDiscountedPrice(discountCalculator.getPriceWithDiscount(this));
		
	}

	public double getTotalPrice() {
		
		double totalPrice = 0;
		
		for (Map.Entry<Product, Integer> lineItem : lineItems.entrySet()) {
			totalPrice += lineItem.getKey().getPrice() * lineItem.getValue(); 
		}
		
		return totalPrice;
	}

	public OrderInfo getInfo() {
		return this.orderInfo;
	}
	
}

OrderInfo

package model;

public class OrderInfo {

	private double totalPrice;
	private double discountedPrice;
	private String chosenType;
	
	public OrderInfo() {}
	
	public double getTotalPrice() {
		return totalPrice;
	}
	
	public void setTotalPrice(double totalPrice) {
		this.totalPrice = totalPrice;
	}
	
	public double getDiscountedPrice() {
		return discountedPrice;
	}
	
	public void setDiscountedPrice(double discountedPrice) {
		this.discountedPrice = discountedPrice;
	}
	
	public String getChosenType() {
		return chosenType;
	}
	
	public void setChosenType(String chosenType) {
		this.chosenType = chosenType;
	}
	
	public String toString() {
		return String.format("Total price:%20.2f\tDiscounted price:%20.2f\tChosen type:%20s", totalPrice, discountedPrice, chosenType);
	}
	
}

DiscountCalculator

package model;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import discounts.DiscountsHolder;

public class DiscountCalculator {

	private Map<Product, Integer> lineItems;

	DiscountCalculator(Map<Product, Integer> newLineItems) {
		this.lineItems = newLineItems;
	}

	public Map<String, Double> calculateTotalSavingsByDiscountName() {

		Map<String, Double> totalSavingsByDiscountName = new HashMap<>();

		for (Product product : lineItems.keySet()) {

			Map<Discount, Double> savingsByDiscount = calculateAmounts(product, lineItems.get(product));

			for (Map.Entry<Discount, Double> savingByDiscount : savingsByDiscount.entrySet()) {

				double value = 0.0;
				String currentDiscountName = savingByDiscount.getKey().getName();

				if (totalSavingsByDiscountName.containsKey(currentDiscountName)) {
					value = totalSavingsByDiscountName.get(currentDiscountName);
				}

				value += savingByDiscount.getValue();
				totalSavingsByDiscountName.put(currentDiscountName, value);
			}
		}

		return totalSavingsByDiscountName;
	}

	public Map<Discount, Double> calculateAmounts(Product product, int quantity) {

		Map<Discount, Double> aggregate = new HashMap<>();

		List<Discount> discounts = DiscountsHolder.getDiscounts(product, quantity);
		List<Discount> discountsForThisProduct = assign(discounts, product);

		for (Discount discount : discountsForThisProduct) {
			aggregate.put(discount, discount.calculateAmount());
		}

		return aggregate;
	}

	public String chooseBest(Map<String, Double> totalSavingsByDiscountName) {

		String picked = "";
		double chosenDiscountAmount = 0;

		for (Map.Entry<String, Double> totalSavingByDiscountName : totalSavingsByDiscountName.entrySet()) {
			if (totalSavingByDiscountName.getValue() > chosenDiscountAmount) {
				picked = totalSavingByDiscountName.getKey();
				chosenDiscountAmount = totalSavingByDiscountName.getValue();
			}
		}

		//If no discounts
		if (chosenDiscountAmount == 0) {
			totalSavingsByDiscountName.put("None", 0.0);
			picked = "None";
		}

		return picked;
	}

	public List<Discount> assign(List<Discount> discounts, Product product) {

		List<Discount> assignedDiscounts = new ArrayList<>();

		//Note: The two discount types are exclusive to each other

		for (Discount discount : discounts) {
			if (discount.getName().startsWith("-6000") && product.getName().startsWith("-6000")) {
				assignedDiscounts.add(discount);
			} else if (!discount.getName().startsWith("-6000") && !product.getName().startsWith("-6000")) {
				assignedDiscounts.add(discount);
			}
		}

		return assignedDiscounts;

	}

	public String getBestDiscount() {

		Map<String, Double> totalSavingsByDiscountName = calculateTotalSavingsByDiscountName();

		return chooseBest(totalSavingsByDiscountName);

	}

	public double getPriceWithDiscount(Order order) {

		Map<String, Double> totalSavingsByDiscountName = calculateTotalSavingsByDiscountName();

		System.out.println(order.getTotalPrice());

		return order.getTotalPrice() - totalSavingsByDiscountName.get(order.orderInfo.getChosenType());

	}
}

Source code

[Junilu Lacar]

Your revised code remains very smelly.  

package discounts;
 
import model.OrderInfo;
import model.Order;
 
public class Controller {
 
    public static void main(String[] args) throws Exception {
         
        int[] quantities = {1, 9, 6, 12};
         
        OrderInfo info = calculate(quantities);
         
        System.out.println(info);
         
    }
 
    protected static OrderInfo calculate(int[] quantities) throws Exception {
 
        Order order = new Order(quantities);
 
        return order.getInfo();
 
    }
}

Let's start with some of the names you use. Forget what you know about the design and think about your code from the perspective of someone coming upon it for the very first time. If all they read was the code above, they'd probably ask, "What's the difference between an Order and an OrderInfo object? How do we know what product we're associating with each of those quantities? Why does the calculate() command give me back an OrderInfo object?"

This part of the design just does not make sense at all. The names you use and the behaviors that the objects exhibit just don't match and they don't tell a coherent, cohesive story. It gets even worse as you dig deeper.

[Paolo Cartella]

From that perspective, I would be very confused indeed.

Since products are not mentioned anywhere in that class, I would not necessarily even know that the quantities are for products.

It seemed like a good idea to put the three things we are interested in in a class, but it wasn't.

The calculate method is too generic. It also doesn't say anything about what it's calculating - discounts? A price perhaps, but what kind? Frequent buyer points?

One thing I did not reveal initially - sorry about that - is that the quantities arrive through a servlet.

I have made it so that an id arrives along with the quantity and we can populate our list items collection based on that. I hope the servlet/controller code makes more sense now.

I'm struggling with the DiscountCalculator class. Specifically, the assign() method is somewhat silly. There is a requirement that says a product can only have an 'amount per quantity' type discount applied if the product itself is 'that kind of product'. I am using the product name to check for that. Perhaps we would be better off with a field on Product to set that. Not sure. The rest of the methods are not very elegant either.

Source code

Unchanged

DiscountsHolder, ProductsHolder, Product, AmountPerQuantityStrategy, DiscountStrategy, QuantityPerQuantityStrategy

DiscountServlet

package discounts;

import java.io.IOException;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;

import javax.servlet.RequestDispatcher;
import javax.servlet.ServletException;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import helper.*;
import model.Order;
import model.Product;

@WebServlet(urlPatterns = {"/", "/discounts"})
public class DiscountServlet extends HttpServlet {

	private final List<Product> products = ProductsHolder.getProducts();

	protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

		Map<Product, Integer> lineItems = new TreeMap<>();

		for (int i = 0; i < products.size(); i++)  {
			lineItems.put(products.get(i), 0);
		}
		
		request.setAttribute("lineItems", lineItems);
		RequestDispatcher dispatcher = request.getRequestDispatcher("discounts.jsp");
		dispatcher.forward(request, response);
	}

	protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

		String[] ids = request.getParameterValues("ids");
		String[] sanitizedQuantities = InputSanitizer.clean(request.getParameterValues("quantities"));

		Map<Product, Integer> lineItems = new TreeMap<>();

		for (int i = 0; i < ids.length; i++)  {
			lineItems.put(products.get(Integer.valueOf(ids[i])), Integer.valueOf(sanitizedQuantities[i]));
		}

		Order order = new Order(lineItems);

		request.setAttribute("totalPrice", order.getTotalPrice());
		request.setAttribute("priceWithDiscount", order.getDiscountedPrice());
		request.setAttribute("bestDiscount", order.getBestDiscount());

		request.setAttribute("lineItems", lineItems);

		RequestDispatcher dispatcher = request.getRequestDispatcher("discounts.jsp");
		dispatcher.forward(request, response);
	}
}

InputSanitizer

package helper;

import java.util.Arrays;

import discounts.ProductsHolder;

public class InputSanitizer {

	public static String[] clean(String[] parameterValues) {

		if (parameterValues == null) {
			String[] returnThis = new String[ProductsHolder.getProducts().size()];
			Arrays.fill(returnThis, "0");
			return returnThis;
		} else {
			for (int i = 0; i < parameterValues.length; i++) {
				if (parameterValues[i] == null) {
					parameterValues[i] = "0";
				} else {
					for (char ch : parameterValues[i].toCharArray()) {
						if (!Character.isDigit(ch)) {
							parameterValues[i] = "0";
							break;
						}
					}
				}
			}
		}
		
		return parameterValues;
		
	}
	
}

DiscountCalculator

package model;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import discounts.DiscountsHolder;

public class DiscountCalculator {

	private Map<Product, Integer> lineItems;
	private Map<String, Double> totalSavingsByDiscountName;

	DiscountCalculator(Map<Product, Integer> newLineItems) {
		this.lineItems = newLineItems;
		this.totalSavingsByDiscountName = calculateTotalSavingsByDiscountName();
	}

	public Map<String, Double> calculateTotalSavingsByDiscountName() {

		Map<String, Double> totalSavingsByDiscountName = new HashMap<>();

		for (Product product : lineItems.keySet()) {

			Map<Discount, Double> savingsByDiscount = calculateAmounts(product, lineItems.get(product));

			for (Map.Entry<Discount, Double> savingByDiscount : savingsByDiscount.entrySet()) {

				double value = 0.0;
				String currentDiscountName = savingByDiscount.getKey().getName();

				if (totalSavingsByDiscountName.containsKey(currentDiscountName)) {
					value = totalSavingsByDiscountName.get(currentDiscountName);
				}

				value += savingByDiscount.getValue();
				totalSavingsByDiscountName.put(currentDiscountName, value);
			}
		}

		return totalSavingsByDiscountName;
	}

	public Map<Discount, Double> calculateAmounts(Product product, int quantity) {

		Map<Discount, Double> aggregate = new HashMap<>();

		List<Discount> discounts = DiscountsHolder.getDiscounts(product, quantity);
		List<Discount> discountsForThisProduct = assign(discounts, product);

		for (Discount discount : discountsForThisProduct) {
			aggregate.put(discount, discount.calculateAmount());
		}

		return aggregate;
	}

	public String chooseBest(Map<String, Double> totalSavingsByDiscountName) {

		String picked = "";
		double chosenDiscountAmount = 0;

		for (Map.Entry<String, Double> totalSavingByDiscountName : totalSavingsByDiscountName.entrySet()) {
			if (totalSavingByDiscountName.getValue() > chosenDiscountAmount) {
				picked = totalSavingByDiscountName.getKey();
				chosenDiscountAmount = totalSavingByDiscountName.getValue();
			}
		}

		//If no discounts
		if (chosenDiscountAmount == 0) {
			totalSavingsByDiscountName.put("None", 0.0);
			picked = "None";
		}

		return picked;
	}

	public List<Discount> assign(List<Discount> discounts, Product product) {

		List<Discount> assignedDiscounts = new ArrayList<>();

		//Note: The two discount types are exclusive to each other

		for (Discount discount : discounts) {
			if (discount.getName().startsWith("-6000") && product.getName().startsWith("-6000")) {
				assignedDiscounts.add(discount);
			} else if (!discount.getName().startsWith("-6000") && !product.getName().startsWith("-6000")) {
				assignedDiscounts.add(discount);
			}
		}

		return assignedDiscounts;

	}

	public String getBestDiscount() {
		return chooseBest(this.totalSavingsByDiscountName);
	}

	public double getPriceWithDiscount(double totalPrice) {
		return totalPrice - this.totalSavingsByDiscountName.get(getBestDiscount());
	}
}

Order

package model;

import java.util.Map;

public class Order {

	Map<Product, Integer> lineItems;
	DiscountCalculator discountCalculator;
	private double discountedPrice;
	private String bestDiscount;
	
	public Order(Map<Product, Integer> newLineItems) {
		
		this.lineItems = newLineItems;
		
		this.discountCalculator = new DiscountCalculator(lineItems);
		
		this.bestDiscount = discountCalculator.getBestDiscount();
		this.discountedPrice = discountCalculator.getPriceWithDiscount(this.getTotalPrice());
		
	}

	public double getTotalPrice() {
		return this.lineItems.entrySet().stream().map(entry -> entry.getKey().getPrice() * entry.getValue()).mapToDouble(Double::doubleValue).sum();
	}

	public double getDiscountedPrice() {
		return this.discountedPrice;
	}

	public String getBestDiscount() {
		return this.bestDiscount;
	}
	
}

Product

package model;

public class Product implements Comparable<Product> {

	private static int currentId = 0;

	private String name;
	private double price;
	private int quantity;
	private int id;

	public int getQuantity() {
		return quantity;
	}

	public int getId() {
		return id;
	}

	public void setQuantity(int newQuantity) {
		this.quantity = newQuantity;
	}

	public Product(String newName, double newPrice) {
		this.name = newName;
		this.price = newPrice;
		this.id = currentId++;
	}

	public Product(Product otherProduct) {
		this.name = otherProduct.getName();
		this.price = otherProduct.getPrice();
		this.id = currentId++;
	}

	public String getName() {
		return name;
	}
	public double getPrice() {
		return price;
	}

	@Override
	public String toString() {
		return String.format("Name: %s, Price: %.2f, Quantity: %d", this.getName(), this.getPrice(), this.getQuantity());
	}

	@Override
	public int compareTo(Product product) {
		return this.name.compareTo(product.name);
	}

}

discounts.jsp

<%@ page language="java" contentType="text/html; charset=UTF-8" pageEncoding="UTF-8"%> <!DOCTYPE html> <%@ taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c"%> <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> <title>Choose Quantities</title> <script type="text/javascript"> function subtractQty(id) { if (document.getElementById(id).value == 0) { return; } else { document.getElementById(id).value--; } } </script> <style> .center {    margin: auto;    width: 500px;    border: 2px solid darkblue;    padding: 10px;    font: 14px arial, sans-serif; } td { padding: 5px; } </style> </head> <body> <div class="center"> <h2>Choose Quantities</h2> <form name="lineItems" action="${pageContext.request.contextPath}/discounts" method="post"> <table> <tr> <td> <h4>Name</h4> </td> <td> <h4>Price</h4> </td> <td> <h4>Quantity</h4> </td> </tr> <c:forEach var="lineItem" items="${lineItems}" varStatus="row"> <tr> <td> <c:out value="${lineItem.key.name}"/> </td> <td> <c:out value="${lineItem.key.price}" /> </td> <td> <input type='hidden' name='ids' value="${lineItem.key.id}"/> <input type='text' pattern='\d+' name='quantities' id="${row.index}" value="${lineItem.value}" required/> </td> <td> <input type='button' name='add' onclick='javascript: document.getElementById(${row.index}).value++;' value='+' /></td> <td> <input type='button' name='subtract' onclick='javascript: subtractQty(${row.index});' value='-' /></td> </tr> </c:forEach> <tr> <td></td><td></td><td></td><td></td><td></td><td><input type="submit" name="submit" value="Submit" /></td></tr> </table> </form> <table> <c:if test="${!empty totalPrice}"> <tr> <td>Total Price:</td> <td><c:out value="${totalPrice}" /></td> </tr> </c:if> <c:if test="${!empty priceWithDiscount}"> <tr> <td>Discounted Price:</td> <td><c:out value="${priceWithDiscount}" /></td> </tr> </c:if> <c:if test="${!empty bestDiscount}"> <tr> <td>Discount Type:</td> <td><c:out value="${bestDiscount}" /></td> </tr> </c:if> </table> </div> </body> </html>

[Junilu Lacar]

Still a lot of non-object-oriented code. I'll just point out one glaring example. In the DiscountCalculator.assign() method:
       for (Discount discount : discounts) {            if (discount.getName().startsWith("-6000") && product.getName().startsWith("-6000")) {                assignedDiscounts.add(discount);            } else if (!discount.getName().startsWith("-6000") && !product.getName().startsWith("-6000")) {                assignedDiscounts.add(discount);            }        }
Nobody reading this (except maybe you) will understand this code. What is the significance of "-6000"? So what if the name starts with that? Why would a name start with a number anyway? This code is all about implementation detail and you showing intimate knowledge of it and manifesting that knowledge in code. However, the intent of the code is lost in the implementation details.  The perspective is still from your point of view, of you taking a reference to objects (discount and product), looking at their names (calling getName() method), then checking it against a specific value ("-6000") with no indication of what that value's significance is to the logic around it.

Then, in two cases, you will add the discount to the assignedDiscounts list. If you draw up a truth table for this logic involving the discount and product objects, there are two other cases that are not accounted for (I'll let you figure out what those cases are). While this may be what you intended, the way you wrote the logic is misleading because the if and the else parts of the statement lead to the same result. It's like saying "Heads I win, tails you lose" when calling a coin toss. it makes no sense.

A more object-oriented approach would be to flip your perspective 180 degrees so that instead of asking these objects for information and doing the logic evaluation outside of the object, you'd simple ask the object to evaluate the logic internally.  Again, I don't know what your intent is for checking startsWith("-6000") but let's just suppose that in the given context, the intent was to determine if a discount should be assigned.  Then, a more object-oriented design of the interaction with these objects might look something like this:

if (product.shouldAssign(discount)) {
    // assignedDiscounts.add(product, discount);  // EDIT -- the parameters don't match List.add() signature
    assignedDiscounts.add( ... ? ... );  // not entirely clear what should be passed in.
}

EDIT: I commented out the original suggestion since the call doesn't match the List.add() method signature. Since your design isn't very clear to me, I'm not really sure what kind of object would be passed. Logically, it would be some kind of object that establishes a relationship between the given discount and product objects.

The details of evaluating whether "should assign discount" is true or false occurs inside the object, where the "knowledge" of its name and whatever logic is involved related to name is contained and encapsulated.  Anything external to the product and discount objects don't have to care what those details are. They just need to ask the product object the question via the shouldAssign() method, "Should this discount be applied to you?"

If that becomes unwieldy or awkward, then another shift in perspective might lead you to a better design:

if (discount.isApplicableTo(product)) {
    assignedDiscounts.add(product, discount);
}

The question is more or less the same but the perspective is switched from asking the product if a discount is applicable to it to asking a discount if it can be applied to a particular product. I like to use refactoring as a  way to experiment with different perspectives to see whether assigning responsibilities one way or another results in a more coherent and cohesive design.

[Junilu Lacar]

Here's another point about refactoring. My main goals for refactoring are aligned with the Four Rules for Simple Design, in short, one or all of clarity, eliminating duplication, keeping things small, and making code testable and verifiable.

In my experience, Refactor-Rename is one of the simplest yet most neglected refactoring techniques available to programmers. This is why when I mentor other developers, Refactor-Rename is what I focus on first, accompanied with teaching them how to choose good names in code. Poorly named classes, interfaces, methods, fields, variables, etc. probably contribute to the more than 50% of the problems with code.

Take my first suggestion for example:

if (product.shouldAssign(discount)) {
    assignedDiscounts.add(product, discount);
}

After reading this again, the name shouldAssign() is not the best name to communicate the intent. To find a better name, apply Refactor - Rename several times, each time reading the code out loud to discern whether you're truly communicating the correct intent. Here are some alternatives:

if (product.isAssignable(discount)) ...

if (product.canBeAssigned(discount)) ...

if (product.isEligibleFor(discount)) ...

There could be other names you can use for that method that will convey the intent more clearly and concisely. Or perhaps you like the other perspective better:

if (discount.isApplicableTo(product))...

if (discount.appliesTo(product))...

The whole point of this Refactor-Rename exercise is to increase the clarity of intent of the code and thus make it more readable and understandable by others.

[Junilu Lacar]

If you can master the application of Refactor - Rename to clarify the intent of code, then add to your toolbox Refactor - Extract (to eliminate duplication, to make things smaller, to move behavior/responsibility), and Refactor - Compose Method to simplify and clarify, to get a Single Level of Abstraction, then you will have enough to clean up 80% or more of the problems in any code that you will write or encounter.

For me, the main keys to mastering these three refactoring techniques are:

1. Knowing how to properly assign responsibilities - this needs diligent study of design principles like SOLID, KISS, SLAP, and GRASP
2. Focusing on clarity of intent
3. Always thinking about the Four Rules of Simple Design
4. Practicing Test-Driven Development (with a strong focus on design thinking)