Remove an element from an array

Question

I have a method which works perfectly. It removes an element from an array. Now i would like to have a clean and simple code.

private void removeUserFromGroup(baseClass[] member)
{
    try
    {
        this.ExHandling = null;
        searchPathMultipleObject groupSearchPath = new searchPathMultipleObject();
        groupSearchPath.Value = "CAMID(\":" + this.DataViewModel.GroupModel.SelectedGroup + "\")";

        propEnum[] props = { propEnum.defaultName, propEnum.searchPath, propEnum.members };

        // get the current group membership.
        group cognosGroup = (group)this.LogonModel.CBICMS.query(groupSearchPath, props, new sort[] { }, new queryOptions())[0];
        if (cognosGroup.members.value.Length > 0)
        {
            for (int y = 0; y < cognosGroup.members.value.Length; y++)
            {
                //check if member[y] is the one that need to be deleted
                if (cognosGroup.members.value[y].searchPath.value == member[0].searchPath.value)
                {
                    int lenght = cognosGroup.members.value.Length - 1;

                    baseClass[] newMembers = new baseClass[lenght];
                    int index = 0;
                    baseClass obj = null;
                    // go trough group
                    for (int i = 0; i <= lenght; i++)
                    {
                        if (i != y)
                        {
                            //create user
                            obj = cognosGroup.members.value[i];
                            newMembers[index] = obj;
                            index++;
                        }
                        else
                        {
                            this._messageText = "*Successfully removed " + this.RemoveUsername.ToLower() + " from " + this.DataViewModel.GroupModel.SelectedGroup;
                        }
                    }
                    cognosGroup.members = new baseClassArrayProp();
                    cognosGroup.members.value = newMembers;
                    this.LogonModel.CBICMS.update(new baseClass[] { cognosGroup }, new updateOptions());
                    y--;
                    Log4NetLogger.Logger.Info("Successfully removed: " + this.RemoveUsername.ToLower() + " from " + this.DataViewModel.GroupModel.SelectedGroup);
                    return;
                }
                else
                {
                    this._messageText = "*" + this.RemoveUsername.ToLower() + " is not a user from " + this.DataViewModel.GroupModel.SelectedGroup;
                }
            }
        }
        else
        {
            Log4NetLogger.Logger.Error(this.DataViewModel.GroupModel.SelectedGroup + " is empty");
            return;
        }
    }
    catch (Exception ex)
    {
        Log4NetLogger.Logger.Error("Error: ", ex);
    }
}

I know it doesn't look good and it isn't coded as if the person who ends up maintaining my code is a violent psychopath who knows where I live. So now I want to have a "clean" code. Can anyone help me and give me some tips?

Thanks!

Answer 1

You should be able to simplify that whole inner loop with a simple Where extension:

If you have the equality operator implemented:

baseClass obj = cognosGroup.members.value[y];
cognosGroup.members.value = cognosGroup.members.value.Where(x => x != obj).ToArray()

Or use the index option:

cognosGroup.members.value = cognosGroup.members.value.Where((x,i) => i != y).ToArray()

Another option is to store the data in a List<baseClass> instead and use the Remove or RemoveAt extension.

Answer 2

A small contribution, but I'm usually in favor of putting validators at the top, rather than at the bottom

if (cognosGroup.members.value.Length == 0)
{
    Log4NetLogger.Logger.Error(this.DataViewModel.GroupModel.SelectedGroup + " is empty");
    return;
}
for (int y = 0; y < cognosGroup.members.value.Length; y++)
{
    //check if member[y] is the one that need to be deleted
    if (cognosGroup.members.value[y].searchPath.value != member[0].searchPath.value)
    {
        this._messageText = "*" + this.RemoveUsername.ToLower() + " is not a user from " + this.DataViewModel.GroupModel.SelectedGroup;
    }
    else
    {
        int length = cognosGroup.members.value.Length - 1;
        ...

Answer 3

If I understand correctly, you only want to remove the member that matches the searchPath value of the first member in the array that is passed in.

If you are able to use LINQ, this solution should work. The only downside is that it will require a full iteration of the array/List.

//Made a variable of the matching criteria
string searchValue = member[0].searchPath.value;

group cognosGroup = (group)this.LogonModel.CBICMS.query(groupSearchPath, props, new sort[] { }, new queryOptions())[0];
var memberList = cognosGroup.members.value.ToList<baseClass>();

if (memberList.Count == 0)
{
    Log4NetLogger.Logger.Error($"{this.DataViewModel.GroupModel.SelectedGroup} is empty");
    return;
}

//Create a list of members that do not match the pattern you are looking for
var notMatched = memberList.Where(member => member.searchPath.value != searchPath).ToList();

//If the count is the same then the match was not found
if(notMatched.Count == memberList.Count)
{
    this._messageText = $"* {this.RemoveUsername.ToLower()} is not a user from {this.DataViewModel.GroupModel.SelectedGroup}";
}
//Otherwise, replace the list with all but the match
else
{
    cognosGroup.members = new baseClassArrayProp();
    cognosGroup.members.value = notMatched.ToArray();
    this.LogonModel.CBICMS.update(new baseClass[] { cognosGroup }, new updateOptions());
    string message = $"Successfully removed {this.RemoveUsername.ToLower()} from {this.DataViewModel.GroupModel.SelectedGroup}"
    this._messageText = $"*{message}";
    Log4NetLogger.Logger.Info(message);
    return;
}

Answer 4

It's hard to provide a complete code review (IMHO) without knowing the API, but here's my comments in lieu of that:

• Method Size - This method is too long. Ideally you want to break down each method to the point that it serves one function that is accurately described by the method name.
• Loops within Loops - Generally speaking loops within loops impede comprehension. Each loop should ideally be implemented in method, with each method looping and calling the nested method.
• Intent - It looks like you're getting a list of users for a given group, dropping n users from the group collection, and then updating the group with the new list. If so, the only thing you need to do is get the list, and filter out all users that match any of the members being dropped. (@tinstaafl has the implementations). In this case, you would only need one method because you're not running through loops this time.

EDIT: Additional Comments and Example Implementation

I've revised the method in the gist here. It's not fully revised to be compliant with best practices, but captures most of the logic you'd want.

You can use Linq to remove the looping logic. Here's where most of the magic happens:

var updatedList = listPriorToRemove.SkipWhile(existingMember =>
    presentMembers.Any(removeMember => removeMember.searchPath == existingMember.searchPath));

In this case, we're taking the list of users who already exist (listPriorToRemove), using the IEnumerable.SkipWhile method to drop any users that exist within the presentMembers enumerable. The IEnumerable.Any method accepts a predicate Func<TSource, bool> which returns true if the searchPath matches. In other words, we're checking each existingMember of the listPriorToRemove and dropping it if any of the presentMembers matches it.

Assumptions

I've made the following assumptions:

• baseClass represents the users to remove.
• searchPath.value is the URL for each user, and different instances of users will have identical searchPath.value values regardless of where the instance had originated from.

I couldn't figure out the class structure here, the group type seems to have a members property of matching the baseClass contract, but you're also using it to initialize an array baseClass[]{cognosGroup}. I'm not sure where I went wrong.

Other Observations

• This code base doesn't adhere to the C# Coding Conventions, which are best practice unless there is some compelling reason to depart from them. So that means PascalCase for type names (class, enum type and enum member, interface, etc).
• The RemoveUsername property suggests that only one user can be removed, but your method has the parameter baseClass[] member, which suggests there are multiple members that can be removed.
• I think there's a preference that we don't try to catch all exceptions, only the ones we know can arise. So catching Exception e is overbroad.