Strategy pattern implementation feedback

Question

I would like to have some feedback on my code, basically because I have been spending a few hours visiting other options before I ended up with this one, so I'm not sure I'm clear enough to value it.

I have an API controller that gets and stores products in a memcached database, they will fall into one of the two categories (basket and history)

public IHttpActionResult PostHistory(SubmitLiveProductsRequest request)
{
    // Omitting validation of request, exception handling for this snippet       
    if (_liveProductsService.Post(request, ProductType.History))
    {
        return Ok();
    }

    return StatusCode(HttpStatusCode.NoContent);
}

public GetProductsResponse GetHistory(GetLiveProductsRequest request)
{
    // Omitting validation of request, exception handling for this snippet
    return products = _liveProductsService.Get(request.SessionId, ProductType.History);
   }

public IHttpActionResult PostBasket(SubmitLiveProductsRequest request)
{
    // Omitting validation of request, exception handling for this snippet
    if(_liveProductsService.Post(request, ProductType.Basket))
    {
        return Ok();
    }
    return StatusCode(HttpStatusCode.NoContent);
}

public GetProductsResponse GetBasket(GetLiveProductsRequest request)
{
    // Omitting validation of request, exception handling for this snippet
    return products = _liveProductsService.Get(request.SessionId, ProductType.Basket);
}

The enum ProductType just being

public enum ProductType
{
    Basket,
    History
}

And the service would be

public class LiveProductsService : ILiveProductsService
{
    private ILiveProductsMergingStrategy _mergingStrategy;
    private readonly IMemcachedClient _client;

    public LiveProductsService(IMemcachedClient client)
    {
        _client = client;
    }

    public IEnumerable<Product> Get(string sessionId, ProductType type)
    {
        return TryGet(sessionId, type) ?? Enumerable.Empty<Product>();
    }


    public bool Post(SubmitLiveProductsRequest request, ProductType type)
    {
        var cachedProducts = TryGet(request.SessionId, type);
        var key = GetKey(request.SessionId, type);

        if (cachedProducts == null)
        {
            return _client.Store(StoreMode.Set, key, request.Products);
        }

        List<Product> mergedProducts = request.Products;
        if (cachedProducts.Any())
        {
            SetMergingStrategy(type);
            mergedProducts = _mergingStrategy.Merge(cachedProducts.ToList(), request.Products);
        }

        return _client.Store(StoreMode.Replace, key, mergedProducts);
    }

    private IEnumerable<Product> TryGet(string sessionId, ProductType type)
    {
        Object obj;
        var key = GetKey(sessionId, type);
        _client.TryGet(key, out obj);

        return obj as List<Product>;
    }

    private static string GetKey(string sessionId, ProductType type)
    {
        return String.Format("{0}-{1}", sessionId, type.ToString());
    }

    private void SetMergingStrategy(ProductType type)
    {
        switch (type)
        {
            case ProductType.Basket:
                _mergingStrategy = new ReplaceMergingStrategy();
                break;
            case ProductType.History:
                _mergingStrategy = new UpdateMergingStrategy();
                break;
            default:
                break;
        }
    }

Having two implementations of ILiveProductsMergingStrategy:

public class ReplaceMergingStrategy : ILiveProductsMergingStrategy
{
    public List<Product> Merge(List<Product> cachedProducts, List<Product> productsToAdd)
    {
        return productsToAdd;
    }
}

public class UpdateMergingStrategy : ILiveProductsMergingStrategy
{
    public List<Product> Merge(List<Product> cachedProducts, List<Product> productsToAdd)
    {
        var mergedProducts = new List<Product>();

        var productsToUpdate =
            (from cachedProduct in cachedProducts
             join productToAdd in productsToAdd on
                 cachedProduct.Name equals productToAdd.Name
             select productToAdd).ToList();

        var newProducts = productsToAdd.Except(productsToUpdate).ToList();
        foreach (var productToUpdate in productsToUpdate)
        {
            var product = cachedProducts.First(
                p => p.Name.Equals(productToUpdate.Name, StringComparison.InvariantCultureIgnoreCase));
            cachedProducts.Remove(product);
            cachedProducts.Add(productToUpdate);
        }

        mergedProducts.AddRange(newProducts);
        mergedProducts.AddRange(cachedProducts);

        return mergedProducts;
    }
}

Don't know if is much code for posting but was trying to put the whole case. My thoughts initially went for just creating a switch statement in the service based on the products types, other option I managed was just do two complete different implementations (and correspondent interfaces) for basket and history and use both in the controller. But eventually I thought this was a proper scenario to use strategy pattern.

So as I said, I would love to hear any feedback on this as I'm trying to improve the way I code and don't get the opportunity to get much direct opinions on it.

One thing that made me doubt was that I'm so used to DI containers that I feel weird not putting the IMergingStrategy into it.

\$\begingroup\$ You've done good posting your whole code! :) \$\endgroup\$

IEatBagels
– IEatBagels

2015-12-04 14:27:37 +00:00
Commented Dec 4, 2015 at 14:27 — IEatBagels
– IEatBagels, Commented Dec 4, 2015 at 14:27

IEatBagels · Accepted Answer · 2015-12-04 14:39:44Z

Your implementation of the Strategy pattern is good. You've understood the pattern pretty well but I don't think the choice of which pattern to apply should be a responsibility of the LiveProductsService class!

This looks like a perfect fit for a Factory pattern. You've got an interface to use and you need to pick one implementation based on a conditional statement. That line is the best explanation I can give about the factory. :p

So let's start by extracting this factory from your class :

public class LiveProductsMergingStrategyFactory
{
    public ILiveProductsMergingStrategy Create(ProductType productType)
    {

        switch (type)
        {
            case ProductType.Basket:
                return new ReplaceMergingStrategy();
            case ProductType.History:
                return new UpdateMergingStrategy();
            default:
                throw new InvalidOperationException("wth");
        }    
    }    
}

Now, you're talking about your DI container. This factory should be in your DI container (meaning you'll need an interface for it, but I'll skip this 'cause I assume you know how it works).

For the rest of the code :

You always retrieve the Strategy to apply, meaning that having a private field for _mergingStrategy is kind of useless since you'll always re-set it before using it. Consider keeping this field as a scoped variable inside your method.

The TryGet pattern usually returns a bool and sets the "return" value in an out parameter. You should stick to this or rename that method otherwise you're kind of breaking a standard.

I don't know if you have access to C#6, but if so you could replace this :

return String.Format("{0}-{1}", sessionId, type.ToString());

By :

return $"{sessionId}-{type}";

String interpolation, it's called. Looks kinda cool eh?

You shouldn't return a List<>. Ever. Unless you're in a private method, which isn't your case here. List<> is an implementation detail. You want to return ICollection<>if you want the client to be able to add/remove from your list. Otherwise return IEnumerable<> to offer a view of your result that cannot be modified.

A small point: it's better to return IReadOnlyCollection<> when you want to return a readonly collection. IEnumerable should be left exclusively for the cases when you work with "hot" collections (yield return or some kind of batching). That makes code clearer as well. — AndreySarafanov
– AndreySarafanov, Commented Dec 4, 2015 at 15:37

Stack Exchange Network

Strategy pattern implementation feedback

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Strategy pattern implementation feedback

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions