patterncsharpModerate
Combining these Add and Remove methods into 1
Viewed 0 times
thesecombiningintoremovemethodsandadd
Problem
I was wondering if it's possible to move these add remove methods into a single method:
I am only looking to merge these methods, if it would be considered best practice.
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:
I will expand on what paritosh commented though.
This signature smells because it returns void but has 2 out parameters.
A better signature would be like:
Note that I've introduced
Your other methods would then become something like:
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
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.