HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

E-Commerce 'sale price' calculator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
salecalculatorpricecommerce

Problem

I have code which I need to refactor in order to use in many places. So I tried some solutions but always ending with repeated and messy code. So, I decided to ask what possible best solutions are.

This code is used for calculating sale price in e-commerce project. My goal is to put some code in method which will not change over time, or better to say, which will be managed only from one place. This part is considering of setting sale price based on some comparison. And problem occurs in this comparison. I want also to do some formatting of controls like Label based on this result, like setting currency code. The currency code will be sometimes in $, sometimes in USD. So, this means I should somehow isolate this currency code also.

In short, I want to refactor this currency code and formatting controls based on calculated sale price.

So, I created BasketHelper class with Product and Account properties and method SetBasketPayment that return properties I set in this method. Basically, I use this approach to group properties of Product and Account classes and then return values.

Here is my code. Any further explanation, I will provide upon your request.

```
public class BasketHelper
{
public Product _product { get; set; }
public Account _account { get; set; }

public BasketHelper SetBasketPayment(Product product, Account account,
HyperLink lblIconOnSale, Label lblSalePrice, Label lblListPrice,
HyperLink lblIconCampaign)
{
decimal _brandAccountDiscountRate = default(decimal);
decimal _accountDiscount = default(decimal);

if (HttpContext.Current.Request.IsAuthenticated) {
MembershipUser mUser = Membership.GetUser();
if (mUser != null) {
account = Account.GetAccountByUserId(
(Guid)Membership.GetUser().ProviderUserKey);
try {
_accountDiscount = account.DiscountRate;
} catch (Exception ex) {

Solution

BasketHelper mixes the different aspects of identifying and authenticating users, calculating prices and displaying information. These aspects should be separated. Especially manipulating the view (setting label texts, choosing CSS-formatting etc.) should not be mixed with business logic. The view logic shouldn't need to calculate prices and discounts. It should only work on results calculated somewhere else.

You have a Product class. Why do you not move product related calculations to Product? For instance add a method GetProductCampaignPrice to Product. You could make it a property as well, but since it is not returning a "static" value but depends on other properties, a method seems more appropriate.

public decimal GetProductCampaignPrice()
{
    decimal currencyMultiplier = Currency.GetCurrencyValue(CurrencyCode);
    decimal listPriceTL = ListPrice * currencyMultiplier;
    return listPriceTL * (1 - DiscountRate / 100);
}


Except for the currency calculator the calculation only needs properties of the product itself (CurrencyCode, ListPrice and DiscountRate). An even better design would be to inject a currency calculator into this method in order to remove the dependency on a specific currency calculator.

public decimal GetProductCampaignPrice(ICurrencyCalculator currencyCalculator)
{
    decimal listPriceNormalized = currencyCalculator.ToStandardCurrency(ListPrice, CurrencyCode);
    return listPriceNormalized * (1 - DiscountRate / 100);
}


Also note that I have removed the leading underscore (_) from the local variable. Underscores are used for fields, i.e. "variables" at class or struct level only.

The stuff in helper classes should be kept to a strict minimum and should really on hold things that don’t fit in anywhere else.

Look at your code and try to identify things that you can move to more specific classes.

A possible class integrating product and account related calculations could be called PriceCalculator; this is a much better name than FooHelper and it leads to a better design. Naming is crucial and goes hand in hand with good designs.

Note: If you don't want to or cannot change the Product class (e.g. because it is generated automatically), you could split the class into two partial classes, one holding the data and one with additional methods. You could also create a ProductExtensions class and make the method an extension method

public static class ProductExtensions
{
    public static decimal GetProductCampaignPrice(this Product product,
                                                  ICurrencyCalculator currencyCalculator)
    {
        decimal listPriceNormalized =
            currencyCalculator.ToStandardCurrency(ListPrice, CurrencyCode);
        return listPriceNormalized * (1 - DiscountRate / 100);
    }
}


You can call this method as if it was a member of Product

decimal result = product.GetProductCampaignPrice(currencyCalculator);

Code Snippets

public decimal GetProductCampaignPrice()
{
    decimal currencyMultiplier = Currency.GetCurrencyValue(CurrencyCode);
    decimal listPriceTL = ListPrice * currencyMultiplier;
    return listPriceTL * (1 - DiscountRate / 100);
}
public decimal GetProductCampaignPrice(ICurrencyCalculator currencyCalculator)
{
    decimal listPriceNormalized = currencyCalculator.ToStandardCurrency(ListPrice, CurrencyCode);
    return listPriceNormalized * (1 - DiscountRate / 100);
}
public static class ProductExtensions
{
    public static decimal GetProductCampaignPrice(this Product product,
                                                  ICurrencyCalculator currencyCalculator)
    {
        decimal listPriceNormalized =
            currencyCalculator.ToStandardCurrency(ListPrice, CurrencyCode);
        return listPriceNormalized * (1 - DiscountRate / 100);
    }
}
decimal result = product.GetProductCampaignPrice(currencyCalculator);

Context

StackExchange Code Review Q#46634, answer score: 7

Revisions (0)

No revisions yet.