Updating database based on selected check boxes

Question

I posted the original question here, but I think it would be nice to have someone else's eye on this code for improving it.

public override void Update(UserProfileDto item)
{
 base.Update(item);
 var memberIntersestIds = _memberInterestService.GetAllPlusDeleted();
 var dbIds = memberIntersestIds.Select(p => p.InterestSubCategoryId);
 var union = dbIds.ToList().Union(item.InterestIds);
 if (item.InterestIds != null)
 {
 var guids = union as Guid[] ?? union.ToArray();
 foreach (var id in guids)
 if (_memberInterestService.IsSubInterestExist(id))
 _memberInterestService.Delete(id, item.Id, true);
 foreach (var id in guids)
 {
 if (!_memberInterestService.IsSubInterestExist(id))
 _memberInterestService.Add(new MemberInterestDto
 {
 UserProfileId = item.Id,
 InterestSubCategoryId = id
 });
 else
 _memberInterestService.Delete(id, item.Id, false);
 }
 }
 else
 foreach (var memberInterest in memberIntersestIds)
 _memberInterestService.Delete(memberInterest.InterestSubCategoryId, item.Id, true);
}

MemberIntersetService

public class MemberInterestService : IMemberInterestService
{
 public IQueryable<MemberInterest> Items { get { return _dbSet.Where(x => x.IsDeleted == false); } }
 public MemberInterestService(AppDbContext dbContext) : base(dbContext)
 {
 _dbSet = dbContext.MemberInterests;
 }
 public bool IsSubInterestExist(Guid subInterestId)
 {
 return _dbSet.Any(p => p.InterestSubCategoryId == subInterestId);
 }
 public bool IsSubInterestExistInItem(Guid subInterestId)
 {
 return Items.Any(p => p.InterestSubCategoryId == subInterestId);
 }
 public void Delete(Guid interestId, Guid userProfileId, bool delete)
 {
 var item = _dbSet.Single(x => x.InterestSubCategoryId == interestId && x.UserProfileId == userProfileId);
 //soft delete
 item.IsDeleted = delete;
 _dbSet.Update(item);
 }
 public IList<MemberInterestDto> GetAllPlusDeleted()
 {
 return _dbSet.Select(x => Mapper.Map<MemberInterest, MemberInterestDto>(x))
 .ToList();
 }
}

• Sirwan Afifi: It's actually an interface that contains some method to perform operation against MemberInterest @denis
• Sirwan Afifi: bool IsSubInterestExist(Guid subInterestId); bool IsSubInterestExistInItem(Guid subInterestId); void Delete(Guid interestId, Guid userProfileId, bool delete); IList<MemberInterestDto> GetAllPlusDeleted();
• t3chb0t: Where are the selected check-boxes?
• Sirwan Afifi: @t3chb0t I get them from user as IEnumerable<Guid> (item.InterestId)
• Denis: @SirwanAfifi I would suggest to add all of the necessary types, so reviewers can test, as it's unclear why you have a bool parameter in your Delete() method and why this makes things different. Also there might things to improve to your existing classes. Code size wont scare us.

Answer 1

OMITTING CURLY BRACES ?

Don't do that a single line statements like this are acceptable even tho they are still not recommended you should always have {} :

if (Foo is Bar)
 return "Foo is Bar";
else
 return "Foo is not Bar";

But this is a huge red flag for bad code style :

foreach (var id in guids)
{
 if (!_memberInterestService.IsSubInterestExist(id))
 _memberInterestService.Add(new MemberInterestDto
 {
 UserProfileId = item.Id,
 InterestSubCategoryId = id
 });
 else
 _memberInterestService.Delete(id, item.Id, false);
}
foreach (var id in guids)
 if (_memberInterestService.IsSubInterestExist(id))
 _memberInterestService.Delete(id, item.Id, true);

If your statement needs to be written on few lines You should have {} e.g in your foreach as it's body contains more than a single line, same goes for your if's it's kinda hard to read it that way and follow the flow of your program.

Aside from that I would move dbIds && union inside your if (item.InterestIds != null) as you are performing potentially unnecessary operations, they will be used only if the if is triggered anyway :

if (item.InterestIds != null)
{
 IEnumerable<Guid> dbIds = memberIntersestIds.Select(p => p.InterestSubCategoryId);
 IEnumerable<Guid> union = dbIds.ToList().Union(item.InterestIds);
 Guid[] guids = union as Guid[] ?? union.ToArray();
 //..
}

This line I assume you have because you are avoiding multiple enumeration

Guid[] guids = union as Guid[] ?? union.ToArray();

But why not call .ToArray() on union instead, there is no need to create new variable here, you are always working with guids and never using union itself as IEnumerable<> :

IEnumerable<Guid> dbIds = memberIntersestIds.Select(p => p.InterestSubCategoryId);
IEnumerable<Guid> guids = dbIds.ToList().Union(item.InterestIds).ToArray();

This part of your method :

foreach (var id in guids)
{
 if (_memberInterestService.IsSubInterestExist(id))
 {
 _memberInterestService.Delete(id, item.Id, true);
 }
}
foreach (var id in guids)
{
 if (!_memberInterestService.IsSubInterestExist(id))
 {
 _memberInterestService.Add(new MemberInterestDto
 {
 UserProfileId = item.Id,
 InterestSubCategoryId = id
 });
 }
 else
 {
 _memberInterestService.Delete(id, item.Id, false);
 }
}

It's weird, and so is the .Delete() method. First you have a foreach that remove duplicates from the database

foreach (var id in guids)
{
 if (_memberInterestService.IsSubInterestExist(id))
 {
 _memberInterestService.Delete(id, item.Id, true);
 }
}

But apparently you don't trust your methods and you keep searching if there is any item that exist in your db with a matching ID from the set you've previously iterated and already removed. What's the point ?

But why you deleting the items from the db only the first time ? What's different ? It's the same ID collection.

Since there is no obvious reason why are you doing this, I would suggest to combine those in a single foreach :

foreach (var id in guids)
{
 if (_memberInterestService.IsSubInterestExist(id))
 {
 _memberInterestService.Delete(id, item.Id, true);
 }
 else
 {
 _memberInterestService.Add(new MemberInterestDto
 {
 UserProfileId = item.Id,
 InterestSubCategoryId = id
 });
 }
}

With all of those changes your code should look like this :

public void Update(UserProfileDto item)
{
 MemberInterestService _memberInterestService = new MemberInterestService();
 IList<MemberInterestDto> memberIntersestIds = _memberInterestService.GetAllPlusDeleted();
 if (item.InterestIds != null)
 {
 IEnumerable<Guid> dbIds = memberIntersestIds.Select(p => p.InterestSubCategoryId);
 IEnumerable<Guid> guids = dbIds.ToList().Union(item.InterestIds).ToArray();
 foreach (var id in guids)
 {
 if (_memberInterestService.IsSubInterestExist(id))
 {
 _memberInterestService.Delete(id, item.Id, true);
 }
 else
 {
 _memberInterestService.Add(new MemberInterestDto
 {
 UserProfileId = item.Id,
 InterestSubCategoryId = id
 });
 }
 }
 }
 else
 {
 foreach (var memberInterest in memberIntersestIds)
 {
 _memberInterestService.Delete(memberInterest.InterestSubCategoryId, item.Id, true);
 }
 }
}

Your Delete method might be better of with a .FirstOrDefault() instead of .Single() unless you really want to verify there is exactly one item, .FirstOrDefault() should be faster in most cases as it will return whenever the first item is found, where .Single() will always be O(n) operation because it needs to verify if there is exactly one occurrence.

Probably more can be improved, but again more context is required.

• t3chb0t: But apparently you don't trust your methods my favourite part ;-)
• Sirwan Afifi: Thanks for suggestions :) If possible please take at look at original question

Answer 2

Please Simplify this and only do this in one foreach loop. I didn't see any reason that you needed to have 2 foreach loops here

 foreach (var id in guids)
 if (_memberInterestService.IsSubInterestExist(id))
 _memberInterestService.Delete(id, item.Id, true);
 foreach (var id in guids)
 {
 if (!_memberInterestService.IsSubInterestExist(id))
 _memberInterestService.Add(new MemberInterestDto
 {
 UserProfileId = item.Id,
 InterestSubCategoryId = id
 });
 else
 _memberInterestService.Delete(id, item.Id, false);
 }

you should only have to iterate over the guids once like this

foreach (var id in guids)
{ 
 if (_memberInterestService.IsSubInterestExist(id))
 {
 _memberInterestService.Delete(id, item.Id, true);
 } 
 else
 {
 _memberInterestService.Add(new MemberInterestDto
 {
 UserProfileId = item.Id,
 InterestSubCategoryId = id
 });
 }
}

I also used proper indentation and used Curly Brackets Everywhere because otherwise it is unclear, at quick glance, what is going on in the nested if statements.

I also removed the else that was setting the Delete value of the item to false (soft delete) although I am not sure what the purpose of "Delete" really is, you do not make it clear that you are not actually deleting anything.

I am not sure what is real in your code...

• Denis: When will the else even be triggered ?
• Denis: I think OP should add more context and more types to the question so a proper review can be done.
• Malachi: I am not sure what the third parameter means in _memberInterestService.Delete(id, item.Id, false);
• Sirwan Afifi: @Malachi It's like a toggle delete, by setting true, it deletes the item from the database;IsDeleted = true