1
\$\begingroup\$

This is part of a document management application that is in a prior question.

Manage users, roles, and groups. A user can have only one role but they can be in many groups. There is join struct UserIDGroupID to manage the relationship for user to groups. This the same as the DB model.

A group cannot contain a group. That makes for some long SQL and document authority is already relatively expensive.

public class UserGroup
{
    public enum Role { Basic, Edit, DocAdmin, UserAdmin };

    public static List<User> LibraryUsers { get; } = new List<User>();
    public static List<Group> LibraryGroups { get; } = new List<Group>();
    private static List<UserIDGroupID> UserIDsGroupIDs = new List<UserIDGroupID>();
    private static SDocsServer SDocsServer;

    public class User
    {
        public UInt16 ID { get; }
        public string Name { get; }
        public string Initials { get; }
        public DateTime LastLogOn { get; }
        public Role Role { get; }
        public bool Locked { get; }
        private List<Group> groups = null;
        public List<Group> Groups
        {
            get
            {
                //most of the time groups will not be called also want a fast startup
                if (groups == null)
                {
                    groups = new List<Group>();
                    foreach (UserIDGroupID userIDGroupID in UserIDsGroupIDs.Where(x => x.UserID == ID))
                    {
                        groups.AddRange(LibraryGroups.Where(x => x.ID == userIDGroupID.GroupID));
                    }
                } 
                return groups;
            }
        }
        internal User(UInt16 id, string name, string initials, DateTime lastLogOn, Role role, bool locked)
        {
            ID = id;
            Name = name;
            Initials = initials;
            LastLogOn = lastLogOn;
            Locked = locked;
            LibraryUsers.Add(this);  //not sure this is the best way 
        }
    }

    public class Group
    {
        public UInt16 ID { get; }
        public string Name { get; }
        private List<User> users = null;
        public List<User> Users
        {
            get
            {
                //Users.AddRange(LibraryUsers.Where(x => x.UserIDsGroupIDs.Any(g => g.Group‌​ID == ID)));
                //most of the time Users will not be called also want a fast startup
                if (users == null)
                {
                    users = new List<User>();
                    foreach (UserIDGroupID userIDGroupID in UserIDsGroupIDs.Where(x => x.GroupID == ID))
                    {
                        Users.AddRange(LibraryUsers.Where(x => x.ID == userIDGroupID.UserID));
                    }
                }
                return users;
            }

        }
        internal Group(UInt16 id, string name)
        {
            ID = id;
            Name = name;
            LibraryGroups.Add(this);  //not sure this is the best way 
        }
    }

    //User can have only one role but many groups      
    internal struct UserIDGroupID
    {
        public UInt16 UserID { get; }
        public UInt16 GroupID { get; }
        public UserIDGroupID(UInt16 userID, UInt16 groupID)
        {
            UserID = userID;
            GroupID = groupID;
        }
    }

    internal UserGroup(SDocsServer sDocsServer)
    {
        SDocsServer = sDocsServer;
        foreach (Group group in sDocsServer.GetGroups())
        {
        }
        foreach (User user in sDocsServer.GetUsers())
        {
        }           
        foreach (UserIDGroupID userIDGroupID in sDocsServer.GetUserIDGroupID())
        {
            //should not have duplicate - trusting server
        }
    }
}

public class UserGroupTest
{
    public UserGroupTest()
    {
        UserGroup userGroup = new UserGroup(new SDocsServer());
        foreach (UserGroup.User user in UserGroup.LibraryUsers)
        {
            Debug.WriteLine($"{user.Name} {user.Role}");
            foreach (UserGroup.Group group in user.Groups)
            {
                Debug.WriteLine($"  {group.Name}");
            }
        }
        foreach (UserGroup.Group group in UserGroup.LibraryGroups)
        {
            Debug.WriteLine($"{group.Name}");
            foreach (UserGroup.User user in group.Users)
            {
                Debug.WriteLine($"  {user.Name}");
            }
        }
    }
}

public class SDocsServer
{
    //this is a whole diffent server side program 
    //this is thousands of line of code 
    //NOT asking for review of this code 
    //this ONLY is here so you do not get syntax error if you load the code 
    internal bool UpdateDocPropValue(SDocsClient.DocPropValue DocPropValue)
    {
        if (!Test())
        {
            throw new ArgumentException("server null or not available");
        }
        return true;
    }
    internal bool AddDoc(SDocsClient.Document document)
    {
        if (!Test())
        {
            throw new ArgumentException("server null or not available");
        }
        return true;
    }
    internal IEnumerable<UserGroup.User> GetUsers()
    {
        yield return new UserGroup.User(1, "Jane Doe", "JD", new DateTime(2000, 1, 1), UserGroup.Role.Basic, false);
        yield return new UserGroup.User(2, "John Doe", "JD", new DateTime(2000, 1, 1), UserGroup.Role.Basic, false);
    }
    internal IEnumerable<UserGroup.Group> GetGroups()
    {
        yield return new UserGroup.Group(1, "One");
        yield return new UserGroup.Group(2, "Two");
    }
    internal IEnumerable<UserGroup.UserIDGroupID> GetUserIDGroupID()
    {
        yield return new UserGroup.UserIDGroupID(1, 1);
        yield return new UserGroup.UserIDGroupID(1, 2);
        yield return new UserGroup.UserIDGroupID(2, 2);
    }
    public List<SDocsClient.Document> Search(List<SDocsClient.SearchItem> SearchItems)
    {
        if (!Test())
        {
            throw new ArgumentException("server null or not available");
        }
        return new List<SDocsClient.Document>();
    }
    public bool Test()
    {
        return true;
    }
    public SDocsServer()
    {
        //connect to db
    }
}
\$\endgroup\$
  • \$\begingroup\$ Missing UserIDsGroupIDs.Add(userIDGroupID); in foreach (UserIDGroupID userIDGroupID in sDocsServer.GetUserIDGroupID()). I deleted after posting for not good reason. \$\endgroup\$ – paparazzo Aug 5 '17 at 5:36
1
\$\begingroup\$

I tried to simplify your solution, while keeping the lazy-loading...

User Class

public class User
{
    public UInt16 ID { get; }
    public string Name { get; }
    public string Initials { get; }
    public DateTime LastLogOn { get; }
    public Role Role { get; }
    public bool Locked { get; }

    internal User(UInt16 id, string name, string initials, DateTime lastLogOn, Role role, bool locked)
    {
        ID = id;
        Name = name;
        Initials = initials;
        LastLogOn = lastLogOn;
        Locked = locked;
    }
}

Group Class

public class Group
{
    public UInt16 ID { get; }
    public string Name { get; }

    internal Group(UInt16 id, string name)
    {
        ID = id;
        Name = name;
    }
}

Join Class

public struct UserIDGroupID
{
    public UInt16 UserID { get; }
    public UInt16 GroupID { get; }

    public UserIDGroupID(UInt16 userID, UInt16 groupID)
    {
        UserID = userID;
        GroupID = groupID;
    }
}

UserGroup Class

public class UserGroup
{
    private readonly ReadOnlyDictionary<UInt16, User> _users;
    private readonly ReadOnlyDictionary<UInt16, Group> _groups;
    private readonly IReadOnlyList<UserIDGroupID> _joins;

    private readonly Dictionary<UInt16, List<Group>> _userIdToGroups;
    private readonly Dictionary<UInt16, List<User>> _groupIdToUsers;

    public UserGroup(SDocsServer sDocsServer)
    {
        // populate the users
        Dictionary<UInt16, User> users = new Dictionary<UInt16, User>();
        foreach (var user in sDocsServer.GetUsers())
        {
            users.Add(user.ID, user);
        }
        _users = new ReadOnlyDictionary<UInt16, User>(users);

        // populate the groups
        Dictionary<UInt16, Group> groups = new Dictionary<UInt16, Group>();
        foreach (var group in sDocsServer.GetGroups())
        {
            groups.Add(group.ID, group);
        }
        _groups = new ReadOnlyDictionary<UInt16, Group>(groups);

        // populate the joins
        _joins = sDocsServer.GetUserIDGroupID().ToList();

        _userIdToGroups = new Dictionary<UInt16, List<Group>>();
        _groupIdToUsers = new Dictionary<UInt16, List<User>>();
    }

    public IReadOnlyList<User> LibraryUsers
    {
        get { return _users.Values.ToList(); }
    }

    // lazy-loading the your joins
    public IReadOnlyList<Group> GroupsByUserId(UInt16 userId)
    {
        if (!_groupIdToUsers.ContainsKey(userId))
        {
            var groups = new List<Group>();
            var joins = _joins.Where(x => x.UserID == userId);
            // you noted you trusted the mapping to be 100% free of errors,
            // otherwise check "if (joins == null)"
            foreach (var join in joins)
            {
                groups.Add(_groups[join.GroupID]);
            }
            _userIdToGroups.Add(userId, groups);
        }

        return _userIdToGroups[userId];
    }

    // lazy-loading the your joins
    public IReadOnlyList<User> UsersByGroupId(UInt16 groupId)
    {
        if (!_groupIdToUsers.ContainsKey(groupId))
        {
            var users = new List<User>();
            var joins = _joins.Where(x => x.GroupID == groupId);
            // you noted you trusted the mapping to be 100% free of errors,
            // otherwise check "if (joins == null)"
            foreach (var join in joins)
            {
                users.Add(_users[join.UserID]);
            }
            _groupIdToUsers.Add(groupId, users);
        }

        return _groupIdToUsers[groupId];
    }
}

Based on my understanding of your code, I think this gets you the kind of optimization you were looking for in the first place, as the methods that populate the search indexers are separately queryable.

The main priority was to try and get rid of the unusual pattern of constructors and static members.


Extra

If you wanted to keep the lists within the models, such as the List<Group> Groups property in the User class, you could do something like this.

public class User
{
    public UInt16 ID { get; }
    public List<Group> Groups { get (UserGroup.GroupsByUserId(this.ID)); }
}

the drawback, if you consider it one, is that the UserGroup class/methods would need to become static.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This works but the most convenient use is for groups to be a property of user and users to be a property group. \$\endgroup\$ – paparazzo Aug 5 '17 at 15:42
  • \$\begingroup\$ @Paparazzi - you could still do that. I just didn't go that far to bridging it back together, so the concept was clearer. \$\endgroup\$ – Svek Aug 5 '17 at 15:46
  • \$\begingroup\$ But User does not have an object UserGroup. I don't follow. \$\endgroup\$ – paparazzo Aug 5 '17 at 15:55
  • \$\begingroup\$ @Paparazzi -- I added it to my answer. \$\endgroup\$ – Svek Aug 5 '17 at 15:55
  • \$\begingroup\$ I don't mean to sound unappreciative but User does not have a property Groups \$\endgroup\$ – paparazzo Aug 5 '17 at 16:06
4
\$\begingroup\$

It took me a while to figure out what this code is doing, because it contains some very surprising constructions.

A quick summary: it's loading users and groups from a server, via a 3-method API (one method for fetching users, one for fetching groups and one for fetching a user-group mapping table). It then exposes these as a list of Users and a list of Groups. The relationships between users and groups are processed on-demand for performance reasons.

Problems

  • UserGroup gives access to users and groups via static lists. Statics are essentially global variables, which are generally troublesome:
    • It's easy to use them from all over the code, which quickly leads to code that's difficult to maintain due to the large amount of dependencies.
    • It also makes dependencies less visible, making the code more difficult to understand. If class A's constructor requires a B argument, then the relationship is obvious just by looking at the signature of A's constructor. But if A's constructor accesses a static field in B instead, then the dependency is only visible when you look at the actual implementation.
    • It's harder to swap out for something else. A method that takes an argument can be passed a different object. A method that accesses a static field is stuck with it. This makes (automated) testing more difficult.
    • What if you need to support multiple 'domains', each with its own users and groups? Using non-static fields allows you to create as few or as many UserGroups as you need. With static fields your options are, well, fairly static.
  • I consider using constructors to register objects in (global) lists to be very surprising, and not in a good way. It results in very counter-intuitive code such as empty foreach loops. It also makes these classes difficult to use properly: for example, iterating the result of SDocServer.GetUsers twice results in users being registered twice.
  • Likewise, UserGroup's constructor is only used for its side effects (filling those static lists). And UserGroupTest's constructor is used to run tests. Constructors are meant to create and initialize objects, not to do all sorts of work or to have side-effects. That's what most programmers expect, and code that goes against such expectations is often a lot harder to follow. That, in turn, makes it easier to make mistakes:
  • UserIDGroupID's are not being registered in their respective list, so user/group lookups currently don't work.

Minor issues

  • Separating methods with newlines would make the code a little easier to read.
  • Putting enum values on a line of their own is easier to read.
  • Personally I'm not a fan of using inner classes for things that aren't internal to a class. Having to prefix their names with the parent class' name looks cumbersome.
  • UInt16 is normally written as ushort. But I'm not sure why those IDs need to be unsigned shorts rather than just ints?
  • This may come down to personal preference, but I think var, when used appropriately, makes code easier to read.

Alternative

Here's how I would probably do this:

// User and Group's constructors are modified to have no side-effects.

public class UserGroup
{
    // IReadOnlyList<T> might be better here:
    public List<User> Users { get; }
    public List<Group> Groups { get; }


    public UserGroup(IEnumerable<User> users, IEnumerable<Group> groups)
    {
        // Argument checks left out for brevity:
        Users = users.ToList();
        Groups = groups.ToList();
    }
}

// Some initialization method, somewhere:
public UserGroup LoadUserGroup(SDocsServer docServer)
{
    var users = docServer.GetUsers().ToArray();
    var groups = docServer.GetGroups().ToArray();
    var userGroupLinks = docServer.GetUserIDGroupID().ToArray();

    // Link users and groups together. This isn't lazy,
    // but the dictionary and lookup should make this fairly efficient:
    var userLookup = users.ToDictionary(user => user.ID, user => user);
    var groupUsersLookup = userGroupLinks.ToLookup(link => link.GroupID, link => link.UserID);

    // Again, a few checks left out for brevity:
    foreach (var group in groups)
    {
        var userIDs = groupUsersLookup[group.ID];
        foreach (var userID in userIDs)
        {
            var user = userLookup[userID];
            group.AddUser(user);
            user.AddGroup(group);
        }
    }

    return new UserGroup(users, groups);
}

// Other code that depends on users or groups should take a UserGroup as argument:
public void DoSomething(UserGroup userGroup)
{
    // TODO
}

Code is put in descriptively named methods instead of constructors, dependencies should be easy to determine from method signatures, there's no useless-looking code anymore, creating a user or group elsewhere in the code doesn't magically add it to a global list anymore, and you're no longer limited to a single UserGroup.

If you do still need the lazy user/group link behavior, then one approach would be to give users and groups a reference to the UserGroup that contains them. UserGroup can then provide methods such as GetUsersInGroup(int groupID), while using dictionaries or lookup tables internally to efficiently look up these relationships.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks good feedback. Does not need to support multiple domains. \$\endgroup\$ – paparazzo Aug 5 '17 at 5:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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