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!

• t3chb0t: I have rolled back the last edit. Please see What should I do when someone answers my question? Do not add an improved version of the code after receiving an answer. Including revised versions of the code makes the question confusing, especially if someone later reviews the newer code.
• Marshall: @t3chb0t that makes sense. sorry about that

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.

• Marshall: Thanks for your impact. I will quickly make changes you suggested. But.. about the Repository (your first point), why? Repository is built in DbContext. This is application layer. Are you suggesting I should abstract repository pattern from EF? Cause many C# devs don't like it.
• t3chb0t: @Marshall I just mean name changing ;-] No real repository patterns. I always name classes that interact with a database SomethingRepository and just drop all my queries there. It's rather a naming convention/pattern than a the actuall-repository one. Sorry for the confusion. I like to know which class works with a database and the Repository part lets me identify them more easily.
• Marshall: ohh.. interesting. So, do you have a ServiceLayer, between your Repositories and ViewModel/WCF or any other client you use? And the second related question: What if you have two databases, and your calculation is done using both Contexts?
• t3chb0t: @Marshall I do, I just call them accordingly to what they to. If it works with a database, then it's a repository - still, just a name ;-) if it's some encapsulated logic that does some useful things and I have no idea how to name it, then it becomes a SomethingService but I don't name classes by the layer they belong to. I try to give them names that clearly communicate ther purpose. So a class that finds files on the disk could be a FileFinder or if it does more than that I name it a FileSystem not a FileService.
• t3chb0t: @Marshall I try not to name anything Service because I find it means I have no idea how to name this thing ;-]