How can I improve this block of code?

public class OrderProcessor {

  public Double calcula开发者_如何学CteTotalPriceWithDiscountCode(Order order,
        char discountCode) {

    Double itemTotal = order.getItemTotal();
    Double discountAmount = 0.0;

    switch (discountCode) {
    case 'A':
        discountAmount = 0.95 * itemTotal;
        break;
    case 'B':
        discountAmount = 0.15 * itemTotal;
        break;
    }
    return itemTotal - discountAmount;
}

Current implementation in order processor is closed for extension and open for modification for adding new discount codes, how can i improve the design to get rid of this limitation

In general, the presence of a switch is a pretty good giveaway as to what should be a class instead. So, a first stab at it would be something like this:

public interface Discounter {
  public double applyDiscount(double itemTotal);
}

public class OrderProcessor {
  private Map<Char, Discounter> discounts = new HashMap<Char, Discounter>();

  public void addDiscounter(Char discountCode, Discounter discounter) {
    discounts.put(discountCode, discounter);
  }

  public Double calculateTotalPriceWithDiscountCode(Order order, char discountCode) {
    double itemTotal = order.getItemTotal();
    double discountAmount = 0.0;

    if (discounts.hasKey(discountCode))
      discountAmount = discounter.applyDiscount(itemTotal);

    return itemTotal - discountAmount;
  }
}

This would then be extended through something like this:

processor.addDiscounter('A', new Discounter() {
  public double applyDiscount(double itemTotal) {
    return 0.95 * itemTotal;
  }
});

You could make a remove method as well, and then your discounters can become more complex, consulting external data, etc. You may want to open up the interface a bit and pass in the whole order for more examination.

Note: If this is something you're going to be doing in production I have two pieces of unsolicited advice:

1. Consider using something like JBoss Drools to handle your business logic instead of this; it's a lot more powerful and flexible.

2. Please don't use double for actual financial calculations. :)

As you have pointed out that the discount code needs to be changeable, it is best to separate out the actual discount codes from the code. Maintain the discount codes in an xml or settings file and lazy load the values into a dictionary/hashset before using them.

For example your xml could look like this,

<Discounts>
  <Discount code="A" value="0.05"/>
  <Discount code="B" value="0.10"/>
  <Discount code="C" value="0.15"/>
</Discounts>

In your calculateTotalPriceWithDiscountCode, populate a dictionary with the values read from this xml.

if(discountsDictionary == null)
{    
    discountsDictionary = LoadValuesFromXml(xmlFilePath);
}

In case your discounts xml is likely to change during program execution then perform the Load operation when you want the discount value (as compared to loading once in the above code fragment).

Then access the code (key) to retrieve the discount (value),

if(discountsDictionary.ContainsKey(discountCode))
{
    discount = discountsDictionary[discountCode];
    discountedItemPrice = itemPrice * ( 1 - discount);
}
else
{
     // Invalid discount code...
}

Pull the logic for calculating totals based on an order into its own class/interface:

class OrderProcessor {
    // Probably inject this or load it some other way than a static factory
    private Collection<TotalCalculator> calculators = TotalCalculatorFactory.getTotalCalculators();

    public Double calculateTotalPriceWithDiscountCode(Order order, char discountCode) {
        for (TotalCalculator calculator : calculators) {
            if (calculator.supports(discountCode)) {
                return calculator.calculateTotal(order);
            }
        }
        return order.getItemTotal();
    }
}

class TotalCalculator {
    private char discountCode;
    private double discountRatio;

    public TotalCalculator(char discountCode, double discountRatio) {
        this.discountCode = discountCode;
        this.discountRatio = discountRatio;
    }

    public boolean supports(char discountCode) {
        return this.discountCode == discountCode;
    }

    public Double calculateTotal(Order order) {
        return order.getItemTotal() - order.getItemTotal() * discountRatio;
    }
}

class TotalCalculatorFactory {
    public static Collection<TotalCalculator> getTotalCalculators() {
        return Arrays.asList(
                new TotalCalculator('A', 0.95),
                new TotalCalculator('B', 0.15)
        );
    }
}

In a real system, I'd make the TotalCalculator an interface, which would have the additional advantage of being able to calculate order totals in other ways than just a percentage discount reduction.

This solution is also very flexible in allowing you to create your TotalCalculators (or implementations if an interface) using other mechanisms than manually coding them into a factory. You can use an IoC container to create and inject them or use a ServiceLoader to find and load them, for example.

My idea is maintain discounts separately.

public class OrderProcessor {

  public Double calculateTotalPriceWithDiscountCode(Order order,
    char discountCode) {

   Double itemTotal = order.getItemTotal();
 return itemTotal - (itemTotal* Discounts.GetDiscounts(discountCode));
  }
}

////////////////

class Discounts
{
 public static double GetDiscounts(char discountCode)
    {
       switch (discountCode) {
case 'A':
    return 0.95d;

case 'B':
    return 0.15d;

default:
      return 0.00d;
  }
 }
}

In this calculateTotalPriceWithDiscountCode function, it is not a good idea to pass a discount code as parameter. Well, 3rd person , who review your code , would not understand would not understand what does it mean except you, a kind of code smell.

One of a suggestion is that you need to create another class called Discount, and you pass the Discount object as a parameter, and you get the internal value from its public method.

i.e.

public class Discount {
//Use a hash map to store your Code associate with your discountAmount

    public Discount(char discountCode){
        this.discountCode = discountCode
    }

    public int getDiscountAmount(){
        ....
       ....
    }
}

Right now, what you actually need to modify will be the Discount Class only, and your calculateTotalPriceWithDiscountCode will not need to care.

You could use a separate xml file for storing codes as well as their calculation mechanishms.

This will remove the limitation of inablility to add new discount code.

XML File: discounts.xml

<discount-codes>
    <discount-code>
       <code>A</code>
       <val>0.15</val>
    </discount-code>
    <discount-code>
        <code>B</code>        
        <val>0.95</val>
    </discount-code>
</discount-codes>

Note: Operation code (What am I intended to do with the values?) is not currently implemented. You can implement the same at your end.

Use an XML parser class:

Class: DiscountModel.java (This class is the model to store the discount codes)

public class DiscountModel {
    char code;
    Double val;

    // Implement getters and setters
}

Class: DiscountParser.java (This will parse the discounts.xml file and store the codes in a list)

public class DiscountParser {
    List<DiscountModel> discountsList;

    // Getters and Setters for discountsList

    // Parser Code
    public void parseDiscounts() {
         // Code here
    }

    // Add new discount
    public void addDiscount() {
        // Code 
    }

    // Remove discount
    public void removeDiscount () {
       // Code
    }
}

Class: OrderProcessor.java (This will bring out the discounted value after calculation)

/**
 *  Call this class when calculations need to be done.
 */
public class OrderProcessor {
    // Declare instance of DocumentParser
    DocumentParser dc1;

    // Getter and setter for dc1

    public Double calculateTotalPriceWithDiscountCode(Order order, char discountCode) {
        // Find the corresponding discountcode and 
        // value from the list of values in the 
        // Class DocumentParser          

        // Use the corresponding values to calculate 
        // the discount and return the value
    }
}  

Whenever a new code is to be added, you can insert the same to the xml file. The same applies if the discount code needs to be removed.

Hope the above helps.