jeudi 4 juillet 2019

Builder pattern in place of method overloading

I have an interface TaxCalculator that looks like this

public interface TaxCalculator { int DEFAULT_AMOUNT_FOR_RATE_CALCULATION = 100;

/**
 *
 * @param cart the cart to calculate taxes for. Right now we are only calculating using the total taxable amount, but when we implement
 *             charging taxes properly, we should use data from the cart like line-item amounts, and types.
 * @return the tax rate to charge for this cart
 */
SalesTaxRates getSalesTaxes(ShoppingCart cart, Address address);
SalesTaxRates getSalesTaxes(ShoppingCart cart, String zipcode);

SalesTaxRates getSalesTaxes(Order order) throws Exception;



/** Since taxes ideally get calculated using information like line-item amounts, and types, you cannot get an accurate
 * rate from the taxable amount alone. This is a function we want to support, however, on pages such as calculate payboo savings.
 *
 * @param taxableAmount the amount to calculate taxes for
 * @return a quote for what taxes will likely come out to
 */
SalesTaxRates getSalesTaxes(double taxableAmount, String zipCode);

SalesTaxRates getSalesTaxes(String zipCode);

SalesTaxRates getSalesTaxes(Address address);

SalesTaxRates getSalesTaxes(double taxableAmount, Address address);

default SalesTaxRates getSalesTaxes(double taxableAmount, ZipCode zipCode){
    return getSalesTaxes(taxableAmount,zipCode.getZipCode());
}
default SalesTaxRates getSalesTaxes( ZipCode zipCode){
    return getSalesTaxes(zipCode.getZipCode());
}
default SalesTaxRates getSalesTaxes(ShoppingCart cart, ZipCode zipcode){
    return getSalesTaxes(cart,zipcode.getZipCode());
}

I have two implementations that calculate tax, one going to an api, and one internal system I have a third implementation that acts as a proxy - effectively on each call it works like this

@Override
    public SalesTaxRates getSalesTaxes(ShoppingCart cart, Address address)
    {
        Map<String,SalesTaxRates> cache = getTaxSessionCache();
        String cacheKey = generateCacheKey(cart,address);
        if(cache.containsKey(cacheKey))
            return cache.get(cacheKey);
        TaxCallable taxCallable = new TaxCallable() {
            @Override
            public SalesTaxRates call() {
                return this.getTaxCalculator().getSalesTaxes(cart,address);
            }
        };
        SalesTaxRates taxRates = callTaxCalculators(taxCallable);
        if(taxRates.isCacheable())
            cache.put(cacheKey, taxRates);
        return taxRates;
    }

And the method defined:

private static SalesTaxRates callTaxCalculators(TaxCallable callable)
    {
        for (TaxCalculator taxCalculator: TaxEngine.getTaxCalculators())
        {
            FutureTask<SalesTaxRates> futureTask = new FutureTask<>(callable.setTaxCalculator(taxCalculator));
            new Thread(futureTask).start();
            try {
                SalesTaxRates taxRates = futureTask.get(getTaxCalculatorTimeout(), TimeUnit.MILLISECONDS);
                if(taxRates!=null && taxRates.isCalculated())
                {
                    BHLogger.info("Used taxcalculator: " + taxRates.getTaxEngine() + "rate: " + taxRates.getTotalTaxRate());
                    return taxRates;
                }
            } catch (Exception e) {
                BHLogger.error(e);
            }
        }
        BHLogger.error("ShoppingCart.calculteSalesTax","No taxCalculator calculated taxes correctly");
        return SalesTaxRates.NOT_CALCULATED;
    }

it checks the cache, sees if its there, and returns it. Otherwise it calls the forwarding message and caches it So this seems very repetitive for each overloaded method, but there isnt an obvious way for me to call one method since the cacheing is different My manager suggested I used the builder pattern. Make a TaxRequest object, which has whatever relevant fields. It could take care of its own cacheing

I guess it would look like this

class TaxRequest{
private Order order;
private Cart cart;
private Address address;
private String zipcode;
private SalesTaxRates send(TaxCalculator tc){
   //check cache
   //else
   tc.getSalesTaxes(this);
   //handlecaching
}
}

The interface only needs one method, and the Forwarding class becomes very simple I guess the ApiTaxCalculator implementation now needs to handle this by having a method like this

SalesTaxRates getSalesTax(TaxRequest taxRequest)
{
   if(taxRequest.hasOrder()
       returnGetSalesTaxes(taxRequest.getOrder);
  //if has address and cart... if has only address... if has just a zip...
}

basically, the suggestion is to use the builder pattern instead of method overloading but that last method seems very awkward to me, Another reason why this was suggested, the internal taxCalculator doesnt do anything different with an Order than it does just a zipcode. Ideally it should take things like itemtype into account, or discounts etc. but it doesnt. It just looks up a rate based on zipcode so theres a lot of overhead in that class just forwarding it to that method

Any advice?

Aucun commentaire:

Enregistrer un commentaire