Does this ugly code fit into a design pattern?

Question

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?

Answer 1

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.

Answer 2

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();

Answer 3

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?