Generic method for checking database integrity on soft delete

Question

I have written this generic method to check whether a object has a dependent items or not:

public void ItemDependant<TEntity>(Guid id, 
 Expression<Func<TEntity, bool>> condition,
 Expression<Func<TEntity, string>> select) where TEntity : BaseModel
{
 var foundItems = _dbContext.Set<TEntity>().Where(x => x.IsDeleted == false).Where(condition);
 var isAny = foundItems.Any();
 if (isAny)
 throw new Exception($"You must first delete these dependent items: {foundItems.Select(select).ToList().Aggregate((i, j) => i + ", " + j)}");
}

Now in each delete method in our services we call this method this way:

public override void Delete(Guid id)
{
 // This entity has only one dependent item (ItemB)
 // It means that this entity is being used by ItemB
 // So we can't delete it from the database
 // We should first delete ItemB
 ItemDependant<ItemB>(id, m => m.ItemAId == id, s => s.Title);
 base.Delete(id);
}

It works like a charm But I'm wondering if there's a better way for doing this.

Update: There's also another problem with this approach, If the entity has several dependent entities, consumer the API must know about them and add them in order:

public override void Delete(Guid id)
{
 ItemDependant<ItemB>(id, m => m.ItemAId == id, s => s.Title);
 ItemDependant<ItemC>(id, m => m.ItemAId == id, s => s.Title);
 ItemDependant<ItemD>(id, m => m.ItemAId == id, s => s.Title);
 ItemDependant<ItemE>(id, m => m.ItemAId == id, s => s.Title);
 ItemDependant<ItemF>(id, m => m.ItemAId == id, s => s.Title);
 base.Delete(id);
}

One thing that comes in my mind is that, I should have only one ItemDependent(.... call, then this method must inspects all of the dependencies then at the end gives the user several error messages. I think this solution is better, Isn't it?

• RobH: It sounds like you want to cascade delete on your foreign key in the database.
• Vogel612: @RobH It sounds like you wanted to write an answer :)

Answer 1

I don't think this is the route you should travel. As an assumption of me, you will, if any dependent Items are found, show a message somehow which shows all the dependent items in a messagebox/label so the user knows which items should/need to be deleted first.

IMO this isn't really userfriendly because the user will need to delete each item individually and will have to remember the items to be deleted. If one of the subitems has dependent items as well another message will be shown which results in another delete marathon...and so on.

Maybe it would be better, if a to be deleted item has subitems to provide a possibility to delete all items at all by showing some kind of form where the user can decide if all items should be deleted or not.

You should add a method which returns the dependent items, if there aren't Any(), go on and delete the desired item. If ther are Any(), return them to the user asking if all of these should deleted as well.

---

Now comming to review the code in question

• Omitting braces {} although they might be optional is a dangerous decision. This can lead to bugs which are hard to find. I would like to encouracge you to always use them.

• usually I tend to use well named variables myself, but if it is obvious I wouldn't bother with them, resulting in less lines of code. What I mean here is the isAny. Having isAny won't give you much/any benefit in this small method. I would just write

  public void ItemDependant<TEntity>(Guid id, 
   Expression<Func<TEntity, bool>> condition,
   Expression<Func<TEntity, string>> select) where TEntity : BaseModel
  {
   var foundItems = _dbContext.Set<TEntity>().Where(x => x.IsDeleted == false).Where(condition);
   if (foundItems.Any())
   {
   throw new Exception($"You must first delete these dependent items: {foundItems.Select(select).ToList().Aggregate((i, j) => i + ", " + j)}");
   }
  }
  

  but I would use a variable to store the result of foundItems.Select(select).ToList().Aggregate((i, j) => i + ", " + j)} because it will increase the readability (IMO). Using C# 6 features is a good thing but you could do it to much.

• By looking at the condition parameter at the call to ItemDependant() inside the Delete() method I wonder why you didn't place the x => x.IsDeleted == false in there as well like so

  ItemDependant<ItemB>(id, m => !m.IsDeleted && m.ItemAId == id, s => s.Title);
  

  resulting in ItemDependant looking like so

  public void ItemDependant<TEntity>(Guid id, 
   Expression<Func<TEntity, bool>> condition,
   Expression<Func<TEntity, string>> select) where TEntity : BaseModel
  {
   var foundItems = _dbContext.Set<TEntity>().Where(condition);
   if (foundItems.Any())
   {
   throw new Exception($"You must first delete these dependent items: {foundItems.Select(select).ToList().Aggregate((i, j) => i + ", " + j)}");
   }
  }
  

• Sirwan Afifi: I do agree with these part: IMO this isn't really userfriendly because the user will need to delete each item individually and will have to remember the items to be deleted. If one of the subitems has dependent items as well another message will be shown which results in another delete marathon...and so on. I'm wondering How to achieve this.
• Heslacher: You could make ItemDependent() recursive returning a List<BaseModel> for each items and subitems which depends on each other.
• Sirwan Afifi: By looking at the condition parameter at the call to ItemDependant() inside the Delete() method I wonder why you didn't place the x => x.IsDeleted == false in there as well like so I just want to make the API easier to call. This way user doesn't need to specify !m.IsDeleted in each call.
• Heslacher: My bad, you didn't tell that this it an API to be consumed
• Sirwan Afifi: Yes it's an API which can be consumed by other services (in each service I have a Delete method) By the way, I have updated my question.