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'm trying to figure out a pattern to best accommodate this pretty ugly code:

var userDiscounts = _discountRepository.GetUserDiscountsForUsersWithDiscounts(new List<int> { buyerUserId, sellerUserId }).ToList();

// Buyer.
var buyerHasFreeCommission = false;
var buyerDiscounts = userDiscounts.Where(ud => ud.UserId == buyerUserId);
foreach(var userDiscount in buyerDiscounts)
{
    switch (userDiscount.Discount.Type)
    {
        case DiscountTypes.SingleFreeCommisionPerUser:
            buyerHasFreeCommission = true;
            break;
    }
}

var sellerHasFreeCommission = false;
var sellerDiscounts = userDiscounts.Where(ud => ud.UserId == sellerUserId);
foreach(var userDiscount in sellerDiscounts)
{
    switch (userDiscount.Discount.Type)
    {
        case DiscountTypes.SingleFreeCommisionPerUser:
            sellerHasFreeCommission = true;
            break;
    }
}

if (!sellerHasFreeCommission)
{
    var sellerCommisionChargeTransaction = new Transaction
    {
        AccountId = sellerAccountId,
        Type = TransactionTypes.Fee,
        Amount = -2
    };

    var sellerTransferFeeTransaction = new Transaction
    {
        AccountId = 5020,
        Type = TransactionTypes.Fee,
        Amount = 2
    };

    _accountRepository.AddTransaction(sellerCommisionChargeTransaction);
    _accountRepository.AddTransaction(sellerTransferFeeTransaction);
}

if (!buyerHasFreeCommission)
{
    var buyerCommisionChargeTransaction  = new Transaction
    {
        AccountId = buyerAccountId,
        Type = TransactionTypes.Fee,
        Amount = -2
    };

    var buyerTransferFeeTransaction = new Transaction
    {
        AccountId = 5020,
        Type = TransactionTypes.Fee,
        Amount = 2
    };

    _accountRepository.AddTransaction(buyerCommisionChargeTransaction);
    _accountRepository.AddTransaction(buyerTransferFeeTransaction);
}

if (!buyerHasFreeCommission || !sellerHasFreeCommission)
    _accountRepository.SaveChanges();

This won't be very maintainable when I need to add a new discount... I was thinking maybe somehow I could use the strategy pattern?

share|improve this question
1  
Did you forgot to finish your sentence? –  svick Dec 16 '12 at 23:54
    
Haha, yes I did I was having trouble sending the questions... –  Ryan Dec 17 '12 at 1:07
    
Please describe what will change if a new discount is added –  almaz Dec 17 '12 at 6:14
add comment

3 Answers

up vote 2 down vote accepted

The strategy pattern is a good candidate for this, one way you could implement it would be something like as follows (the discounts are the strategies).

Define an interface for classes which can apply discounts:

public interface IApplyDiscount
{
    void Apply(buyerUserId, buyerTransaction, sellerUserId, sellerTransaction);
}

Create an implementation of the interface for the SingleFreeCommisionPerUser discount.

public class SingleFreeCommisionPerUserDiscount : IApplyDiscount
{
    public void Apply(buyerUserId, buyerTransaction, sellerUserId, sellerTransaction)
    {
        var userDiscounts = _discountRepository.GetUserDiscountsForUsersWithDiscounts(new List<int> { buyerUserId, sellerUserId }).ToList();

        var buyerHasFreeCommission = userDiscounts.Any(ud => ud.UserId == buyerUserId && ud.Discount.Type == DiscountTypes.SingleFreeCommisionPerUser);

        if(buyerHasFreeCommission)
        {
            buyerTransaction.Amount = 0;
        }

        var sellerHasFreeCommission = userDiscounts.Any(ud => ud.UserId == buyerUserId && ud.Discount.Type == DiscountTypes.SingleFreeCommisionPerUser);

        if(sellerHasFreeCommission)
        {
            sellerTransaction.Amount = 0;
        }
    }
}

The main code then creates the initial commission transactions:

var sellerCommisionChargeTransaction = new Transaction
{
    AccountId = sellerAccountId,
    Type = TransactionTypes.Fee,
    Amount = -2
};

var buyerCommisionChargeTransaction  = new Transaction
{
    AccountId = buyerAccountId,
    Type = TransactionTypes.Fee,
    Amount = -2
};

Applies all the available discounts:

List<IApplyDiscount> discounts = GetAllDiscounts();
discounts.ForEach(d => d.Apply(buyerUserId, buyerCommisionChargeTransaction, sellerUserId, sellerCommisionChargeTransaction));

Then, only add the corresponding charge if a transaction amount is due.

if (sellerCommisionChargeTransaction.Amount < 0)
{
    var sellerTransferFeeTransaction = new Transaction
    {
        AccountId = 5020,
        Type = TransactionTypes.Fee,
        Amount = -sellerCommisionChargeTransaction.Amount
    };

    _accountRepository.AddTransaction(sellerCommisionChargeTransaction);
    _accountRepository.AddTransaction(sellerTransferFeeTransaction);
}

if (buyerCommisionChargeTransaction.Amount < 0)
{
    var buyerTransferFeeTransaction = new Transaction
    {
        AccountId = 5020,
        Type = TransactionTypes.Fee,
        Amount = -buyerCommisionChargeTransaction.Amount
    };

    _accountRepository.AddTransaction(buyerCommisionChargeTransaction);
    _accountRepository.AddTransaction(buyerTransferFeeTransaction);
}

Then if you want to add other discount types and rates, you can just create additional IApplyDiscount implementations and include them in the results of the GetAllDiscounts() call.

This implementation is more flexible, you don't need to change any of the current implementation to add additional discounts.

The discounts could be percentages and the corresponding TransferFeeTransaction will cater for that by charging the positive value of whatever amount remains on the CommissionChargeTransaction.

share|improve this answer
add comment

You can filter all the data needed right from repository, and then iterate over collection containing required elements only:

var usersWithoutSingleFreeCommision = _discountRepository
    .GetUserDiscountsForUsersWithDiscounts(new List<int> { buyerUserId, sellerUserId })
    .GroupBy(ud => ud.UserId)
    .Where(g => g.All(ud => ud.Discount.Type != DiscountTypes.SingleFreeCommisionPerUser))
    .Select(g => g.Key)
    .ToArray();

//you might already have this mapping somewhere. Otherwise just cache it in readonly field
var accountIdMapping = new Dictionary<int, int> { { buyerUserId, buyerAccountId }, { sellerUserId, sellerAccountId } };

foreach (var userId in usersWithoutSingleFreeCommision)
{
    var commisionChargeTransaction = new Transaction
    {
        AccountId = accountIdMapping[userId],
        Type = TransactionTypes.Fee,
        Amount = -2
    };

    var transferFeeTransaction = new Transaction
    {
        AccountId = 5020,
        Type = TransactionTypes.Fee,
        Amount = 2
    };

    _accountRepository.AddTransaction(commisionChargeTransaction);
    _accountRepository.AddTransaction(transferFeeTransaction);
}

//you can remove this check as usually SaveChanges() is always called. It just makes the life easier
if (usersWithoutSingleFreeCommision.Any())
    _accountRepository.SaveChanges();
share|improve this answer
add comment

Put the contents, or all of each if and foreach into their own functions. Unit test the functions. Put everything else into small functions too.

In the end your main function should read like a small story:

var buyerHasFreeCommision = GetBuyerFreeCommision(userDiscouts);
if (buyerHasFreeCommision) AddBuyerFees(userDiscounts);
var sellerHasFreeCommision = GetSellerFreeCommision(userDiscounts);
if (seller...)

If you do this, you'll probably see some pattern emerge. I can already see two classes, or rather instances of a- instead of one. Maybe something like Strategy?

share|improve this answer
add comment

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.