5
\$\begingroup\$

I have UserGroups for Users to be assigned to in a ListBox in HTML form. Items from listbox can be multi-selected or completely unselected.

In summary currently, I am getting a int[] of GroupId from client into my action and I am pulling existing assigned usergroups for the user into the action as well. Then I am comparing the Length of both int[] to determine whether UserGroups for the User was added or removed which is depicted in the following flow chart:

flow_chart

Actual code:

var existUserGroups = db.Users.Where(u => u.ID == user.ID).Single().UserGroups;
var existUserGroupsIds = db.Users.Where(u => u.ID == user.ID).Single().UserGroups.Select(ug => ug.ID).ToArray<int>();
var newUserGroupsIds = user.GroupId ?? new int[0];

//No change made
if (!existUserGroupsIds.SequenceEqual(newUserGroupsIds))
{
    //Add
    if (newUserGroupsIds.Length > existUserGroupsIds.Length)
    {
        for (int i = 0; i < newUserGroupsIds.Length; i++)
        {
            if (!existUserGroupsIds.Any(id => id == newUserGroupsIds[i]))
            {
                int id = newUserGroupsIds[i];
                existUserGroups.Add(db.UserGroups.Where(ug => ug.ID == id).Single());
            }
        }
    }
    //Remove
    else if (newUserGroupsIds.Length < existUserGroupsIds.Length)
    {
        var diff = existUserGroupsIds.Except(newUserGroupsIds);
        if (diff != null && diff.Count() > 0)
        {
            for (int i = 0; i < diff.Count(); i++)
            {
                int id = diff.ElementAt(i);
                existUserGroups.Remove(db.UserGroups.Where(ug => ug.ID == id).Single());
            }
        }
    }
    //Update
    else
    {
        existUserGroups.Remove(existUserGroups.ElementAt(0));
        int id = newUserGroupsIds[0];
        existUserGroups.Add(db.UserGroups.Where(ug => ug.ID == id).Single());
    }

    db.SaveChanges();
}

This is working but the code seems to be very tangled. So, please suggest how I can improve this code or how devs usually handle such situations.

\$\endgroup\$
  • 1
    \$\begingroup\$ Just want to check I'm guessing correctly: you're using an ORM like Entity Framework, Users and Groups are each entities with their own table, and because they're in a many-to-many relationship you also have a UserGroups table to map them. Is that correct? \$\endgroup\$ – Ben Aaronson Jul 15 '14 at 11:01
  • \$\begingroup\$ @BenAaronson You guessed correct! But EF handles the join table stuff so all I have for typed models are User and Group. \$\endgroup\$ – lbrahim Jul 15 '14 at 11:04
  • 1
    \$\begingroup\$ So db.UserGroup is what you're calling a Group? \$\endgroup\$ – Ben Aaronson Jul 15 '14 at 11:05
  • \$\begingroup\$ @BenAaronson Yeah, yeah. Sorry for the confusion. Technically, typed models are UserGroup and User. I will edit my question. \$\endgroup\$ – lbrahim Jul 15 '14 at 11:37
2
\$\begingroup\$

Small Code duplication

var existUserGroups = db.Users.Where(u => u.ID == user.ID).Single().UserGroups;
var existUserGroupsIds = db.Users.Where(u => u.ID == user.ID).Single().UserGroups.Select(ug => ug.ID).ToArray<int>();

can be reduced to

var existUserGroups = db.Users.Where(u => u.ID == user.ID).Single().UserGroups;
var existUserGroupsIds = existUserGroups.Select(ug => ug.ID).ToArray<int>();

The //Add part

Should be simplified using a foreach loop and made more readable by using the Contains() method

foreach (int newId in newUserGroupsIds)
{
    if (!existUserGroupsIds.Contains(newId))
    {
        existUserGroups.Add(db.UserGroups.Where(ug => ug.ID == newId).Single());
    }
}

The //Remove part

var diff = existUserGroupsIds.Except(newUserGroupsIds);

diff will never be null => the null check can be skipped
using a for..each instead of a for loop => the check for diff.Count() > 0 can be skipped

var diff = existUserGroupsIds.Except(newUserGroupsIds);
foreach (int id in diff)
{
    existUserGroups.Remove(db.UserGroups.Where(ug => ug.ID == id).Single());
}
\$\endgroup\$

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.