HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Combining these Add and Remove methods into 1

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thesecombiningintoremovemethodsandadd

Problem

I was wondering if it's possible to move these add remove methods into a single method:

public void AddToVipGroup(int companyID, string userID, string auditTrailUrl)
{
    string userName = "", groupName = "";
    GetUserAndGroupNames(companyID, userID, out userName, out groupName);
    GroupUtilities.AddUserToGroup(userName, groupName, auditTrailUrl);
}

public void RemoveFromVipGroup(int companyID, string userID, string auditTrailUrl)
{
    string userName = "", groupName = "";
    GetUserAndGroupNames(companyID, userID, out userName, out groupName);
    GroupUtilities.RemoveUserFromGroup(userName, groupName, auditTrailUrl);
}

private void GetUserAndGroupNames(int companyID, string userID, out string userName, out string groupName)
{
    var vipGroupSID = new MyDataAccess().GetVipGroupSID(companyID);
    if (string.IsNullOrEmpty(vipGroupSID))
        throw new Exception(string.Format("Company id: {0} has no vips group in db", companyID));

    //add user in Company Access vips group
    DBUser user = DBUser.LoadBySid(userID);
    DBGroup group = DBGroup.LoadBySid(vipGroupSID);

    userName = user.UserName;
    groupName = group.Name;
}


I am only looking to merge these methods, if it would be considered best practice.

Solution

It isn't a good idea to merge these methods. They do completely opposite things - you never want a method like:

public void DoSomething(bool arbitraryFlag)
{
    if (arbitraryFlag)
    {
        DoSomething1();
    }
    else
    {
        DoSomethingCompletelyDifferent();
    }
}


I will expand on what paritosh commented though.

private void GetUserAndGroupNames(int companyID, string userID, out string userName, out string groupName)


This signature smells because it returns void but has 2 out parameters.

A better signature would be like:

public class UserAndGroup
{
    public string Username { get; set; }
    public string GroupName { get; set; }
}

public UserAndGroup GetUserAndGroup(int companyId, string userId)
{
    var vipGroupSID = new MyDataAccess().GetVipGroupSID(companyID);
    if (string.IsNullOrEmpty(vipGroupSID))
    {
        // see edit
        throw new Exception(string.Format("Company id: {0} has no vips group in db", companyID));
    }

    var user = DBUser.LoadBySid(userID);
    var group = DBGroup.LoadBySid(vipGroupSID);

    return new UserAndGroup 
    {
        Username = user.UserName,
        GroupName = group.Name
    };
}


Note that I've introduced var where they type is obvious, added braces around the if statements and removed the very confusing comment that was there.

Your other methods would then become something like:

public void RemoveFromVipGroup(int companyID, string userID, string auditTrailUrl)
{
    var userAndGroup = GetUserAndGroup(companyID, userID);
    GroupUtilities.RemoveUserFromGroup(userAndGroup.Username, userAndGroup.GroupName, auditTrailUrl);
}


I still think it's a bit weird though... Is there only one group? It's odd to add/remove a user to a group without being able to specify what that group is.

EDIT

You should never throw System.Exception - there is always a better, specific exception to throw. If there isn't, create one (as a last resort).

For example, you could create a GroupMissingException or something that makes sense for your domain.

Code Snippets

public void DoSomething(bool arbitraryFlag)
{
    if (arbitraryFlag)
    {
        DoSomething1();
    }
    else
    {
        DoSomethingCompletelyDifferent();
    }
}
private void GetUserAndGroupNames(int companyID, string userID, out string userName, out string groupName)
public class UserAndGroup
{
    public string Username { get; set; }
    public string GroupName { get; set; }
}

public UserAndGroup GetUserAndGroup(int companyId, string userId)
{
    var vipGroupSID = new MyDataAccess().GetVipGroupSID(companyID);
    if (string.IsNullOrEmpty(vipGroupSID))
    {
        // see edit
        throw new Exception(string.Format("Company id: {0} has no vips group in db", companyID));
    }

    var user = DBUser.LoadBySid(userID);
    var group = DBGroup.LoadBySid(vipGroupSID);

    return new UserAndGroup 
    {
        Username = user.UserName,
        GroupName = group.Name
    };
}
public void RemoveFromVipGroup(int companyID, string userID, string auditTrailUrl)
{
    var userAndGroup = GetUserAndGroup(companyID, userID);
    GroupUtilities.RemoveUserFromGroup(userAndGroup.Username, userAndGroup.GroupName, auditTrailUrl);
}

Context

StackExchange Code Review Q#107773, answer score: 10

Revisions (0)

No revisions yet.