4
\$\begingroup\$

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.

\$\endgroup\$
3
  • 3
    \$\begingroup\$ Why you want to make one method , explicit is much better. I think GetUserAndGroupNames need more attention , as you are using out variable rather it should be returning User class. (Cannot put it as an answer) \$\endgroup\$
    – Paritosh
    Oct 16, 2015 at 12:14
  • \$\begingroup\$ @paritosh I wasn't sure which is why last line says "if it would be consid..", however can you please put your words into code as an answer as i am really bad understanding words only :(, thanks \$\endgroup\$ Oct 16, 2015 at 13:15
  • \$\begingroup\$ The desire to improve code is implied for all questions on this site. Question titles should reflect the purpose of the code, not how you wish to have it reworked. See How to Ask. \$\endgroup\$
    – Jamal
    Oct 16, 2015 at 15:08

1 Answer 1

10
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ This signature smells because it returns void but has 2 out parameters. msdn.microsoft.com/en-us/library/ms182131.aspx .. there is a warning for it.. though, I don't necessarily agree with avoiding them.. I do use a TryQuery<TEntity> pattern on my ServiceBase. \$\endgroup\$ Oct 16, 2015 at 18:27
  • \$\begingroup\$ Thanks for your positive answer, but I still think more improvements can be made in future, i will leave that for future. Thanks again :) \$\endgroup\$ Nov 4, 2015 at 13:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.