2
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • \$\begingroup\$ It's actually an interface that contains some method to perform operation against MemberInterest @denis \$\endgroup\$ – Sirwan Afifi Jan 11 '17 at 15:06
  • \$\begingroup\$ bool IsSubInterestExist(Guid subInterestId); bool IsSubInterestExistInItem(Guid subInterestId); void Delete(Guid interestId, Guid userProfileId, bool delete); IList<MemberInterestDto> GetAllPlusDeleted(); \$\endgroup\$ – Sirwan Afifi Jan 11 '17 at 15:08
  • 1
    \$\begingroup\$ Where are the selected check-boxes? \$\endgroup\$ – t3chb0t Jan 11 '17 at 15:28
  • \$\begingroup\$ @t3chb0t I get them from user as IEnumerable<Guid> (item.InterestId) \$\endgroup\$ – Sirwan Afifi Jan 11 '17 at 15:29
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – Denis Jan 11 '17 at 15:32
6
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ But apparently you don't trust your methods my favourite part ;-) \$\endgroup\$ – t3chb0t Jan 11 '17 at 16:34
  • \$\begingroup\$ Thanks for suggestions :) If possible please take at look at original question \$\endgroup\$ – Sirwan Afifi Jan 11 '17 at 17:54
2
\$\begingroup\$

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...

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ When will the else even be triggered ? \$\endgroup\$ – Denis Jan 11 '17 at 15:30
  • 1
    \$\begingroup\$ I think OP should add more context and more types to the question so a proper review can be done. \$\endgroup\$ – Denis Jan 11 '17 at 15:33
  • \$\begingroup\$ I am not sure what the third parameter means in _memberInterestService.Delete(id, item.Id, false); \$\endgroup\$ – Malachi Jan 11 '17 at 15:34
  • 1
    \$\begingroup\$ @Malachi It's like a toggle delete, by setting true, it deletes the item from the database;IsDeleted = true \$\endgroup\$ – Sirwan Afifi Jan 11 '17 at 15:36

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.