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.

• IEatBagels: You've done good posting your whole code! :)

Answer 1

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.

• AndreySarafanov: 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.