Service Layer, using Entity Framework 6.2

Question

I would like you to review a part of my warehouse application, a Service Layer to be exact. The architecture of the program is:

DB --> .Data (EF's DbContext/Entities) --> .ServiceLayer (Business) --> WCF services

I have Repo/UoW patterns implemented in Data (EF's DbConext has it built in). Basically nothing more. Raw DbSets and Entities. The whole magic is being done in Business layer obviously.

In Business layer I have implemented a bunch of Services, like PurchaseOrderService or ReceivingService. The thing is those services can be used by WCF Service directly AND by Business themselves. For example GetItemStatus can be used by WCF Service with the same name "GetItmeStatus" OR it can be used by GetItemQuantity service (because if the Item is Blocked, the Qty is 0). Let me post the code:

public class ItemService
{
    IContextFactory ContextFactory { get; set; }

    public ItemService(IContextFactory contextFactory)
    {
        ContextFactory = contextFactory;
    }

    public ItemStatus GetItemStatus(int itemId, WarehouseContext warehouseContext = null)
    {
        ItemStatus returnedItemStatus = ItemStatus.New;

        //DBContext should be disposed only if it was created here, locally
        bool shouldBeDisposed = (warehouseContext == null);
        if (shouldBeDisposed) warehouseContext = ContextFactory.CreateWarehouseContext();

        try
        {
            returnedItemStatus = warehouseContext.Items.Where(p => p.ItemId == itemId).Status;
        }
        finally
        {
            if (shouldBeDisposed) warehouseContext?.Dispose();
        }

        return returnedItemStatus;
    }

    public int GetItemQuantity(int itemId, WarehouseContext warehouseContext = null)
    {
        int returnedQuantity = 0;

        //DBContext should be disposed only if it was created here, locally
        bool shouldBeDisposed = (warehouseContext == null);
        if (shouldBeDisposed) warehouseContext = ContextFactory.CreateWarehouseContext();

        try
        {
            if (GetItemStatus(itemId, warehouseContext) != ItemStatus.Blocked) 
            {
                returnedQuantity = warehouseContext.Items.Where(p => p.ItemId == itemId).Quantity;
            }
        }
        finally
        {
            if (shouldBeDisposed) warehouseContext?.Dispose();
        }

        return returnedQuantity;
    }
}

Of course, this example shows two methods from the same service. It's possible that one service will create another service and calls its function.

I would like you, to review the way I handle optional Context parameter, try finally block instead of pretty using block. This way, I don't have to create DbContext during every trip to the database. I can create a DbContext per Context. When I must get ItemStatus (ItemStatus only) from the WCF (or any other client) layer.. I can do it! and the DbContext will be created and disposed.

How do you like it? Thanks!

Answer 1

How do you like it?

I don't like it ;-)

Because...

• This is a repository and should be named ItemRepository
• Unless you can prove (with proper profiler results) that reusing the context has a better performance I wouldn't care about it. Because of this opmization your API gets more complicated and confusing. See Performance Considerations (Entity Framework) Actually you might win more performance by disabling change-tracking than by reusing the context.
• The ContextFactory property is public and it has a setter. Why? It's already set via the constructor. Make it either a private field or remove the constructor because a constructor informs me that this value is mandatory. For this class however it's both. Mandatory for creation but it can be set to null after creating the ItemService. This is very confusing.
• You don't need any of the helper variables returnedItemStatus or returnedQuantity. Just return the value where you get it.
• ItemStatus.New is another confusing thing. If I try to GetItemStatus and it does not exist then I expect it to either throw or return a status that tells me the item actually does not exist. How can it be New if it wasn't found? The same applies to the other method. If the item does not exist I expect either an exception or a -1 for the quantity that informs me that the item does not exist. Saying 0 for something that isn't even there is just wrong. It makes me think it could be or was there but is sold out. So, does a 0 mean you don't offer it at all or it's currently not in stock? 0 is not a very useful result in this case. You should better differentiate both cases.
• Here I'm done reviewing your code because you cannot return a single Status from this query .Where(p => p.ItemId == itemId).Status which makes me think this is not your real service...
• One last thought... instead of querying each item property at a time with queries like these... why don't you simply get the item or an info-object about it. I would save you a lot of database trips.