tags:

views:

195

answers:

9

Hello all, 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?

+1  A: 

I think I would choose:

u.GetGroupMembership()
Greg Hewgill
+1  A: 
u.GetGroups()

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

Owen
Agreed (+1). Unless there is another relationship between a user and multiple groups (e.g. can they be a creator of multiple groups, without being a member of such groups?), "getGroups" is perfectly unambiguous.
Bobby Jack
A: 

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)

Vinko Vrsalovic
+1  A: 

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

u.Groups;

or

u.UserGroups; // if Groups is ambiguous
Panos
A: 

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.

+3  A: 

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.

J c
A: 

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.

J c
A: 

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

+1  A: 
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? ;-)

Yarik