Question

we had a heated discussion about a method name.

We have a class User. There is property called "Groups" on the user. It contains all groups that contain the user directly. That's ok. What we have problem with, is the name of the method that would recursively list all user's groups and their "parent" groups and return list of all groups, of which the user can be considered as member.

User u = <get user>;
IList<UserGroup> groups = u.XYZ();

Console.WriteLine("User {0} is member of: ", u);
foreach(UserGroup g in groups) 
   Console.WriteLine("{0}", g);

My colleagues brought:

u.GetAllGroups();       // what groups?
u.GetMemberOfGroups();  // doesn't make sense
u.GroupsIAmMemberOf();  // long
u.MemberOf();           // short, but the description is wrong
u.GetRolesForUser();    // we don't work with roles, so GetGroupsForUser ?
u.GetOccupiedGroups();  // is the meaning correct?

What name would you propose?

Was it helpful?

Solution

I agree with Greg, but would make it simpler:

 u.GroupMembership();

I think appending the verb Get is kind of useless, given the return type (List of Groups)

OTHER TIPS

In the interests of high cohesion and low coupling, I would suggest keeping that functionality out of your User class entirely. It should also be easier to implement caching for multiple calls if that functionality was in a different class.

For example:

User u = <get user>;
IList<UserGroup> groups = SecurityModel.Groups.getMembership(u);

You then have the option to cache the group/user memberships in the Groups object, improving efficiency for future group membership requests for other users.

I think I would choose:

u.GetGroupMembership()
u.GetGroups()

unless there was some ambiguity about the meaning of groups in your application, i suppose. (i like typing less when possible!)

Given that there are no parameters, I suggest a property e.g.

u.Groups;

or

u.UserGroups; // if Groups is ambiguous
if (the signature of the property Groups cannot be changed)
{
    I think you are screwed
    and the best thing I can think of
    is another property named AllGroups
    // u.Groups and u.GetWhatever() look very inconsistently
}
else
{
    if (you are okay with using the term "group")
    {
        I would select one of these variants:
            {
                a pair of properties named ParentGroups and AncestorGroups
            }
            or
            { 
                a parameterized method or property Groups(Level)
                where Level can be either PARENTS (default) or ANCESTORS
            }
    }
    else
    {
        I would consider replacing "group" with "membership"
        and then I would select one of these variants:
            {
                a pair of properties named DirectMemberships and AllMemberships
            }
            or
            { 
                a parameterized method or property Memberships(Level)
                where Level can be either DIRECT_ONLY (default) or ALL
            }
    }
}

Does any of this make any sense? ;-)

I'm from Stej's team:-) There is already property called "Groups" on the user. It contains all groups that contain the user directly. That's ok.

What we have problem with, is the name of the method that would recursively list all user's groups and their "parent" groups and return list of all groups, of which the user can be considered as member.

Depending on what environment you guys are working in, you may be able to leverage existing frameworks for this sort of thing rather than rolling your own. If you are using .NET 2.0 or higher, I'd suggest leveraging the System.Web.Security.RoleProvider class. A previous answer of mine has more thoughts on this here.

Roles are flat, we need something more powerful. We don't want to be bound to web environment either.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top