Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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) { }

                BrandAccountDiscount brandAccountDiscount =
                    BrandAccountDiscount
                        .GetBrandAccountDiscountByUserAndBrandId(
                            product.BrandId, mUser.ProviderUserKey.ToString());
                if (brandAccountDiscount != null) {
                    _brandAccountDiscountRate = brandAccountDiscount.DiscountRate;
                }
            }
        }

        decimal currencyMultiplier = Currency.GetCurrencyValue(product.CurrencyCode);
        decimal _listPriceTL = product.ListPrice * currencyMultiplier;

        decimal _productCampaignPrice = _listPriceTL * (1 - product.DiscountRate / 100);
        decimal _accountPrice = _listPriceTL * (1 - _accountDiscount / 100);
        decimal _brandPrice = _accountPrice * (1 - _brandAccountDiscountRate / 100);

        lblListPrice.Text = product.ListPrice.ToString("N2") + " " + product.CurrencyCode;

        if (product.DiscountRate > 0) {
            product.SalePrice = _productCampaignPrice;

            lblSalePrice.Text = _productCampaignPrice.ToString("C2") + " + KDV";
            lblListPrice.CssClass += " strike";
            lblIconCampaign.Text = "+%" + product.DiscountRate.ToString("N0");
            lblIconCampaign.Visible = true;
        } else {
            if (_accountPrice < _listPriceTL) {
                product.SalePrice = _accountPrice;

                lblIconOnSale.Text = "%" + _accountDiscount.ToString();
                lblIconOnSale.Visible = true;
            }

            if (_brandAccountDiscountRate > 0) {
                product.SalePrice = _brandPrice;

                lblSalePrice.Text = _brandPrice.ToString("C2") + " +KDV";
            }
        }

        return new BasketHelper {
            _product = product,
            _account = account
        };
    }
}
share|improve this question

4 Answers 4

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);
share|improve this answer
    
I agree with all above you said. The thing is that I couldn't find way to refactor it effectivelly inspite of fact that I have some theoretic knowledge of code design and principles. –  black123 Apr 8 at 12:55
    
I really didn't know where to put this code, because currently code design is like one class is responsible for one table in database. Classes are designed to be strongly typed with constructor involve all classes properties. So, moving some properties in BasketHelper to for example Product class will lead to more bad design, IMO. I do not like this code design too. But thing is I have to use it. –  black123 Apr 8 at 13:21
    
I am not suggesting to move properties, but the product campaign price calculation only requires properties of the product itself (CurrencyCode, ListPrice, DiscountRate), so why not creating a method in Product to do the calculation in place, instead of extracting those informations from the product and do the calculation somewhere else? –  Olivier Jacot-Descombes Apr 8 at 13:28
    
upvoted. But seems like I will stay with this solutions. Anyway thank for recommendations. I will look for some refactoring book, maybe Refactoring: Improving the Design of Existing Code‏ and than try to do it better next time. –  black123 Apr 8 at 14:43
    
Added a note with two options for the GetProductCampaignPrice placement. –  Olivier Jacot-Descombes Apr 8 at 16:06

you have no comments in your code - you need some with any code, but especially something like this. Start by extracting the individual functions (you may find this easier once you've added comments); for example:

private void DiscountNotZero()
{
    product.SalePrice = _productCampaignPrice;

    lblSalePrice.Text = _productCampaignPrice.ToString("C2") + " + KDV";
    lblListPrice.CssClass += " strike";
    lblIconCampaign.Text = "+%" + product.DiscountRate.ToString("N0");
    lblIconCampaign.Visible = true;
}

That should give you a much smaller function.

Secondly, I would take all your instance variables: _brandPrice, _listPrice, etc. and extract them into a separate model.

The third point is that you are mixing your UI and business layers. Once you have a small function that returns a model, you should then be able to call it, and then subsequently read it into (or better, bind it to) the UI layer.

Just to add slightly to this, and to address your "time constraints". The Visual Studio IDE has a Refactor menu. Just highlight the code you wish to refactor, and select Refactor -> Extract Method. If you then give the extracted function a descriptive name, this should go at least some way to breaking up your function.

share|improve this answer
    
The thing about comments is that this code was developed by another dev and he didn't comment anything, so commenting it will take at least 2 weeks, since there is a lot of code and I don't have that time. Anyway, I will try to do something with this mess –  black123 Apr 8 at 12:50
2  
If you start splitting the code into smaller functions, you may find that does some of the commenting for you. –  pm_2 Apr 8 at 13:00
2  
Be careful with comments. Comments that try to explain bad code are better replaced by self-explaining code. Comments providing background information are good. They can help to answer questions like: "Why was this algorithm used?", "What value ranges are allowed", "Are null arguments allowed", "What kind of result will be returned if the user cannot be authenticated? Will null be returned, will an exception be thrown?" and so on. Doc-comments (XML-comments) are a good place to start with commenting. –  Olivier Jacot-Descombes Apr 8 at 13:05
    
I almost agree. As I said, splitting into smaller functions would massively help with this, and the comments, ideally should be function comments, because the functions are so small. However, even a couple of comments in his original function to say what was being done next would be better than it currently is. Certainly not ideal, I agree. –  pm_2 Apr 8 at 13:09

I see Helpers as utility classes that just perform simple quick repetitive operations, I would never put business logic on a Helper class.

BasketHelper is therefore for me a bad name: I would call it for instance BasketService.

You need interfaces: without interfaces it is very hard to refactor nor test anything.

Yo need one interface for your BasketService class, one for Product and one for Account (so your input parameters would not be of type Product but say of type IProduct). By creating interfaces you can create classes that do just one thing each (as opposed to one class doing everything), this is the famous Single responsibility principle

If you look at the code, you are basically updating the SalePrice and the 4 associated labels/hyperlinks. Return a new Basket class with these, and try not to modify any input objects (ie the product).

I don't quite understand the logic of the discounts: if a product has a discount then both the account and brand discounts are ignored (even if they are bigger discounts), this does not make sense to me.

I have put together a skeleton of a partial possible structure -it does not cover everything-, it looks rather big, but it helps segregate the functionality into separate classes (see http://en.wikipedia.org/wiki/Single_responsibility_principle) and by using interfaces, it hides the actual implementation details.

using System;
using System.Web;

public partial class _Default : System.Web.UI.Page
{
    protected void Page_Load(object sender, EventArgs e)
    {
        IAccount account = null;//your product here
        IProduct product = null;//your account here

        IDiscountService discountService = new DiscountService();
        ICurrencyService currencyService = new CurrencyService();

        IBasketService basketService = new BasketService(discountService, currencyService);

        BasketPayment basketPayment = basketService.CalculateBasketPayment(product, account, HttpContext.Current.Request);

        //now populate the ui elements
        //with the basketPayment information
    }
}


public interface IProduct
{
    double ListPrice { get; set; }
    string CurrencyCode { get; set; }
}

public interface IAccount
{
    double DiscountRate { get; set; }
}



public interface IDiscountService
{
    //ideally this interface should not have to know about the HttpRequest, but about the MembershipUser
    double CalculateBrandDiscount(IProduct product, HttpRequest request);
    double CalculateAccountDiscount(IAccount account, HttpRequest request);
}


public interface IBasketService
{
    BasketPayment CalculateBasketPayment(IProduct product, IAccount account, HttpRequest request);
}

public interface ICurrencyService
{
    double GetCurrencyMultiplier(string currencyCode);
}

//This class only knows about Currencies
public class CurrencyService :  ICurrencyService
{

    public double GetCurrencyMultiplier(string currencyCode)
    {
        switch (currencyCode.ToUpper())
        {
            case "EUR" : return 1.0;
            default: return 1.0;
        }
    }
}

/// <summary>
/// This class only calculates Discounts
/// </summary>
public class DiscountService : IDiscountService
{

    public double CalculateBrandDiscount(IProduct product, HttpRequest request)
    {
        //to be implemented
        return 0.0;
    }

    public double CalculateAccountDiscount(IAccount account, HttpRequest request)
    {
        //to be implemented
        return 0.0;
    }
}


public class BasketPayment
{
    public double SalePrice { get; set; }

    //etc
}


public class BasketService : IBasketService
{
    IDiscountService DiscountService;
    ICurrencyService CurrencyService;

    public BasketService(IDiscountService discountService, ICurrencyService currencyService)
    {
        // Consider Using Dependency Injection to provide this parameters automagically
        DiscountService = discountService;
        CurrencyService = currencyService;
    }

    /// <summary>
    /// Interface method. All the data it needs should be in its parameters, in the constructor of the class or as result of previous operations,
    /// avoid using environment data such as HttpContext.Current.Request, this way the code can be mocked and tested in a controlled environment.
    /// </summary>
    /// <param name="product"></param>
    /// <param name="account"></param>
    /// <param name="request"></param>
    /// <returns></returns>
    public BasketPayment CalculateBasketPayment(IProduct product, IAccount account, HttpRequest request)
    {
        var brandDiscount = DiscountService.CalculateBrandDiscount(product, request);
        var accountDiscount = DiscountService.CalculateAccountDiscount(account, request);

        BasketPayment result = new BasketPayment();
        result.SalePrice = _CalculatePrice(product, brandDiscount, accountDiscount);

        return result;
    }

    /// <summary>
    /// The actual implementation is private to the class
    /// </summary>
    /// <param name="product"></param>
    /// <param name="brandDiscount"></param>
    /// <param name="accountDiscount"></param>
    /// <returns></returns>
    private double _CalculatePrice(IProduct product, double brandDiscount, double accountDiscount)
    {
        var currencyMultipler = CurrencyService.GetCurrencyMultiplier(product.CurrencyCode);

        var effectiveDiscount = brandDiscount < accountDiscount ? brandDiscount : accountDiscount;

        return product.ListPrice * currencyMultipler * (1 - effectiveDiscount / 100);
    }
}
share|improve this answer
    
I just read your answer and now I am gonna play with it. I was looking for answer like you provided. As of discount parts, our customer wanted it in that way, so if there is a product discount the other two discounts are ignored. So this logic is a requirement. –  black123 Apr 8 at 18:45
    
Hope it works for you. The customer is king! –  Leopoldo Salvo Apr 8 at 19:13

First of all, BasketHelper is a good name for static class, but not for your case.

public Product _product { get; set; }
public Account _account { get; set; }

should be renamed:

public Product Product { get; set; }
public Account Account { get; set; }

because Product and Account are public properties.

public BasketHelper SetBasketPayment(Product product, Account account, HyperLink lblIconOnSale, Label lblSalePrice, Label lblListPrice, HyperLink lblIconCampaign)

I'm not sure that is a good idea to put labels and hyperlinks as parameters of method. It looks like mix of view and business logic.

decimal _brandAccountDiscountRate = default(decimal);
decimal _accountDiscount = default(decimal);

Use names without _ for local variables. By convention _ uses for private fields. Use var for local variables, I didn't get using default values for those variables.

Just put:

var brandAccountDiscountRate = 0;
var accountDiscount = 0;

There is no handling exceptions. Put errors into log, display for user etc.:

catch (Exception ex) { }

Method SetBasketPayment should only set paymant, but a lot of logic that should be in view:

        lblSalePrice.Text = _productCampaignPrice.ToString("C2") + " + KDV";
        lblListPrice.CssClass += " strike";
        lblIconCampaign.Text = "+%" + product.DiscountRate.ToString("N0");
        lblIconCampaign.Visible = true;

It looks like this class can be static with method SetBasketPayment. I hope it helps :)

share|improve this answer
    
Thank you for good points. I know that everything is mixed. Thaht's why I posted this question here. –  black123 Apr 8 at 13:41

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.