2
\$\begingroup\$

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.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ You've done good posting your whole code! :) \$\endgroup\$ – IEatBagels Dec 4 '15 at 14:27
3
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$ – AndreySarafanov Dec 4 '15 at 15:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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