3
\$\begingroup\$

I wondered if someone could give me some constructive criticism on my group system?

Holds the badge parts for groups.

public class GroupBadgeParts
{
    public int Id;
    public string AssetOne;
    public string AssetTwo;

    public GroupBadgeParts(int id, string assetOne, string assetTwo)
    {
        Id = id;
        AssetOne = assetOne;
        AssetTwo = assetTwo;
    }
}

Holds the colors for each group.

public class GroupColours
{
    public int Id;
    public string Colour;

    public GroupColours(int id, string colour)
    {
        Id = id;
        Colour = colour;
    }
}

Holds a enum for the group type

public enum GroupType
{
    Open,
    Locked,
    Private
}

Holds an instance of each group

public class Group : IDisposable
{
    public int Id;
    public string Name;
    public int AdminOnlyDeco;
    public string Badge;
    public int CreateTime;
    public int CreatorId;
    public string Description;
    public int RoomId;
    public int Colour1;
    public int Colour2;
    public bool ForumEnabled;
    public GroupType GroupType;
    public bool HasForum;
    public List<int> Members;
    public List<int> Requests;
    public List<int> Administrators;

    public Group(int id, string name, string description, string badge, int roomId, int owner, int time, GroupType type, int colour1, int colour2, int adminOnlyDeco, bool hasForum)
    {
        Id = id;
        Name = name;
        Description = description;
        RoomId = roomId;
        Badge = badge;
        CreateTime = time;
        CreatorId = owner;
        Colour1 = colour1 == 0 ? 1 : colour1;
        Colour2 = colour2 == 0 ? 1 : colour2;
        HasForum = hasForum;
        GroupType = type;
        AdminOnlyDeco = adminOnlyDeco;
        ForumEnabled = hasForum;
        Members = new List<int>();
        Requests = new List<int>();
        Administrators = new List<int>();

        Load();
    }

    public void Load()
    {
        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("SELECT `user_id`, `rank` FROM `group_memberships` WHERE `group_id` = @id");
            dbConnection.AppendParameter("id", Id);

            using (var reader = dbConnection.ExecuteReader())
            {
                while (reader.Read())
                {
                    var userId = reader.GetInt32("user_id");

                    if (reader.GetInt32("rank") != 0)
                    {
                        if (!Administrators.Contains(userId))
                        {
                            Administrators.Add(userId);
                        }
                    }
                    else
                    {
                        if (!Members.Contains(userId))
                        {
                            Members.Add(userId);
                        }
                    }
                }
            }

            dbConnection.SetQuery("SELECT `user_id` FROM `group_requests` WHERE `group_id` = @id");
            dbConnection.AppendParameter("id", Id);

            using (var reader = dbConnection.ExecuteReader())
            {
                while (reader.Read())
                {
                    var userId = reader.GetInt32("user_id");

                    if (Members.Contains(userId) || Administrators.Contains(userId))
                    {
                        dbConnection.SetQuery("DELETE FROM `group_requests` WHERE `group_id` = '" + Id + "' AND `user_id` = '" + userId + "'");
                        dbConnection.ExecuteNonQuery();
                    }
                    else if (!Requests.Contains(userId))
                    {
                        Requests.Add(userId);
                    }
                }
            }
        }
    }

    public List<int> GetAllMembers
    {
        get
        {
            var members = new List<int>(Administrators.ToList());
            members.AddRange(Members.ToList());

            return members;
        }
    }

    public int MemberCount => Members.Count + Administrators.Count;
    public int RequestCount => Requests.Count;

    public bool IsMember(int id)
    {
        return Members.Contains(id) || Administrators.Contains(id);
    }

    public bool IsAdmin(int id)
    {
        return Administrators.Contains(id);
    }

    public bool HasRequest(int id)
    {
        return Requests.Contains(id);
    }

    public void MakeAdmin(int id)
    {
        if (Members.Contains(id))
        {
            Members.Remove(id);
        }

        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("UPDATE group_memberships SET `rank` = '1' WHERE `user_id` = @uid AND `group_id` = @gid LIMIT 1");
            dbConnection.AppendParameter("gid", Id);
            dbConnection.AppendParameter("uid", id);
            dbConnection.ExecuteNonQuery();
        }

        if (!Administrators.Contains(id))
        {
            Administrators.Add(id);
        }
    }

    public void TakeAdmin(int userId)
    {
        if (!Administrators.Contains(userId))
        {
            return;
        }

        Administrators.Remove(userId);

        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("UPDATE group_memberships SET `rank` = '0' WHERE user_id = @uid AND group_id = @gid");
            dbConnection.AppendParameter("gid", Id);
            dbConnection.AppendParameter("uid", userId);
            dbConnection.ExecuteNonQuery();
        }

        Members.Add(userId);
    }

    public void AddMember(int id)
    {
        if (IsMember(id) || GroupType == GroupType.Locked && Requests.Contains(id))
        {
            return;
        }

        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            if (IsAdmin(id))
            {
                dbConnection.SetQuery("UPDATE `group_memberships` SET `rank` = '0' WHERE user_id = @uid AND group_id = @gid");
                Administrators.Remove(id);
                Members.Add(id);
            }
            else if (GroupType == GroupType.Locked)
            {
                dbConnection.SetQuery("INSERT INTO `group_requests` (user_id, group_id) VALUES (@uid, @gid)");
                Requests.Add(id);
            }
            else
            {
                dbConnection.SetQuery("INSERT INTO `group_memberships` (user_id, group_id) VALUES (@uid, @gid)");
                Members.Add(id);
            }

            dbConnection.AppendParameter("gid", Id);
            dbConnection.AppendParameter("uid", id);
            dbConnection.ExecuteNonQuery();
        }
    }

    public void DeleteMember(int id)
    {
        if (IsMember(id))
        {
            if (Members.Contains(id))
            {
                Members.Remove(id);
            }
        }
        else if (IsAdmin(id))
        {
            if (Administrators.Contains(id))
            {
                Administrators.Remove(id);
            }
        }
        else
        {
            return;
        }

        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("DELETE FROM group_memberships WHERE user_id=@uid AND group_id=@gid LIMIT 1");
            dbConnection.AppendParameter("gid", Id);
            dbConnection.AppendParameter("uid", id);
            dbConnection.ExecuteNonQuery();
        }
    }

    public void HandleRequest(int id, bool accepted)
    {
        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            if (accepted)
            {
                dbConnection.SetQuery("INSERT INTO group_memberships (user_id, group_id) VALUES (@uid, @gid)");
                dbConnection.AppendParameter("gid", Id);
                dbConnection.AppendParameter("uid", id);
                dbConnection.ExecuteNonQuery();

                Members.Add(id);
            }

            dbConnection.SetQuery("DELETE FROM group_requests WHERE user_id=@uid AND group_id=@gid LIMIT 1");
            dbConnection.AppendParameter("gid", Id);
            dbConnection.AppendParameter("uid", id);
            dbConnection.ExecuteNonQuery();
        }

        if (Requests.Contains(id))
        {
            Requests.Remove(id);
        }
    }

    public void ClearRequests()
    {
        Requests.Clear();
    }

    public void Dispose()
    {
        Requests.Clear();
        Members.Clear();
        Administrators.Clear();
    }
}

Holds an instance of each group member

public class GroupMember
{
    public int Id;
    public string Username;
    public string Look;

    public GroupMember(int id, string username, string look)
    {
        Id = id;
        Username = username;
        Look = look;
    }
}

The main class, the manager.

public class GroupManager
{
    public ConcurrentDictionary<int, Group> Groups;
    public List<GroupBadgeParts> Bases;
    public List<GroupBadgeParts> Symbols;
    public Dictionary<int, GroupColours> BackgroundColours;
    public List<GroupColours> BaseColours;
    public Dictionary<int, GroupColours> SymbolColours;

    public GroupManager()
    {
        Groups = new ConcurrentDictionary<int, Group>();
        Bases = new List<GroupBadgeParts>();
        Symbols = new List<GroupBadgeParts>();
        BackgroundColours = new Dictionary<int, GroupColours>();
        BaseColours = new List<GroupColours>();
        SymbolColours = new Dictionary<int, GroupColours>();
    }

    public void Init()
    {
        Bases.Clear();
        Symbols.Clear();
        BackgroundColours.Clear();
        BaseColours.Clear();
        SymbolColours.Clear();

        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("SELECT `id`,`type`,`firstvalue`,`secondvalue` FROM `groups_items` WHERE `enabled` = '1'");

            using (var reader = dbConnection.ExecuteReader())
            {
                while (reader.Read())
                {
                    switch (reader.GetString("type"))
                    {
                        case "base":
                            Bases.Add(new GroupBadgeParts(reader.GetInt32("id"), reader.GetString("firstvalue"), reader.GetString("secondvalue")));
                            break;

                        case "symbol":
                            Symbols.Add(new GroupBadgeParts(reader.GetInt32("id"), reader.GetString("firstvalue"), reader.GetString("secondvalue")));
                            break;

                        case "color":
                            BaseColours.Add(new GroupColours(reader.GetInt32("id"), reader.GetString("firstvalue")));
                            break;

                        case "color2":
                            SymbolColours.Add(reader.GetInt32("id"), new GroupColours(reader.GetInt32("id"), reader.GetString("firstvalue")));
                            break;

                        case "color3":
                            BackgroundColours.Add(reader.GetInt32("id"), new GroupColours(reader.GetInt32("id"), reader.GetString("firstvalue")));
                            break;
                    }
                }
            }
        }
    }

    public bool TryGetGroup(int id, out Group group)
    {
        group = null;

        if (Groups.ContainsKey(id))
        {
            return Groups.TryGetValue(id, out group);
        }

        if (Groups.ContainsKey(id))
        {
            return Groups.TryGetValue(id, out group);
        }

        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("SELECT * FROM `groups` WHERE `id` = @id LIMIT 1");
            dbConnection.AppendParameter("id", id);

            using (var reader = dbConnection.ExecuteReader())
            {
                while (reader.Read())
                {
                    group = new Group(
                        reader.GetInt32("id"),
                        reader.GetString("name"),
                        reader.GetString("desc"),
                        reader.GetString("badge"),
                        reader.GetInt32("room_id"),
                        reader.GetInt32("owner_id"),
                        reader.GetInt32("created"),
                        reader.GetInt32("state") == 0 ? GroupType.Open : reader.GetInt32("state") == 1 ? GroupType.Locked : GroupType.Private,
                        reader.GetInt32("colour1"),
                        reader.GetInt32("colour2"),
                        reader.GetInt32("admindeco"),
                        reader.GetInt32("forum_enabled") == 1);

                    Groups.TryAdd(group.Id, group);

                    return true;
                }
            }
        }

        return false;
    }

    public bool TryCreateGroup(PlayerData player, string name, string description, int roomId, string badge, int colour1, int colour2, out Group group)
    {
        group = new Group(0, name, description, badge, roomId, player.Id, (int) PlusEnvironment.GetUnixTimestamp(), 0, colour1, colour2, 0, false);

        if (string.IsNullOrWhiteSpace(name) || string.IsNullOrWhiteSpace(badge))
        {
            return false;
        }

        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("INSERT INTO `groups` (`name`, `desc`, `badge`, `owner_id`, `created`, `room_id`, `state`, `colour1`, `colour2`, `admindeco`) VALUES (@name, @desc, @badge, @owner, UNIX_TIMESTAMP(), @room, '0', @colour1, @colour2, '0')");
            dbConnection.AppendParameter("name", group.Name);
            dbConnection.AppendParameter("desc", group.Description);
            dbConnection.AppendParameter("owner", group.CreatorId);
            dbConnection.AppendParameter("badge", group.Badge);
            dbConnection.AppendParameter("room", group.RoomId);
            dbConnection.AppendParameter("colour1", group.Colour1);
            dbConnection.AppendParameter("colour2", group.Colour2);

            group.Id = Convert.ToInt32(dbConnection.GetLastId());

            group.AddMember(player.Id);
            group.MakeAdmin(player.Id);

            if (!Groups.TryAdd(group.Id, group))
            {
                return false;
            }

            dbConnection.SetQuery("UPDATE `rooms` SET `group_id` = @gid WHERE `id` = @rid LIMIT 1");
            dbConnection.AppendParameter("gid", group.Id);
            dbConnection.AppendParameter("rid", group.RoomId);
            dbConnection.ExecuteNonQuery();

            dbConnection.SetQuery("DELETE FROM `room_rights` WHERE `room_id` = '" + roomId + "'");
            dbConnection.ExecuteNonQuery();
        }

        return true;
    }

    public string GetColourCode(int id, bool colourOne)
    {
        if (colourOne)
        {
            if (SymbolColours.ContainsKey(id))
            {
                return SymbolColours[id].Colour;
            }
        }
        else
        {
            if (BackgroundColours.ContainsKey(id))
            {
                return BackgroundColours[id].Colour;
            }
        }

        return "";
    }

    public void DeleteGroup(int id)
    {
        Group group = null;

        if (Groups.ContainsKey(id))
        {
            Groups.TryRemove(id, out group);
        }

        group?.Dispose();
    }

    public List<Group> GetGroupsForUser(int userId)
    {
        var groups = new List<Group>();

        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("SELECT g.id FROM `group_memberships` AS m RIGHT JOIN `groups` AS g ON m.group_id = g.id WHERE m.user_id = @user");
            dbConnection.AppendParameter("user", userId);
            var getGroups = dbConnection.ExecuteTable();

            if (getGroups == null)
            {
                return groups;
            }

            foreach (DataRow row in getGroups.Rows)
            {
                if (TryGetGroup(Convert.ToInt32(row["id"]), out Group group))
                {
                    groups.Add(group);
                }
            }
        }

        return groups;
    }
}
\$\endgroup\$
5
\$\begingroup\$

Name individual object's classes in the singular, for example: GroupBadgePart, not GroupBadgeParts


public class GroupColours

Holds a single colour. Change name to GroupColor. The name implies its used in Group class. Group class does not use it; if this is a more general "colour", then perhaps rename it Colour.

Group class has colour1, colour2. Should these attributes be GroupColours properties?

Id vis-a-vis Colour. Seem redundant to me. Isn't Colour property unique per se? Or do you want two "black" to have different Ids, for example?

No public setters

You defeat the purpose of the constructor by allowing explicit property settings. And in the long run its a bad idea in general. Rewrite to:

public Colour { get; protected set; }
public Id { get; protected set; }

Unneeded lookup

if (Groups.ContainsKey(id))
{
    Groups.TryRemove(id, out group);
}

TryRemove returns a boolean so there is no need to wrap it in if(Contains()).

Ditto for TryGetValue().


Do not assume class behavior, define it.

I suspect the classes' Ids are coming from a database because they're constructor parameters. I say this because I wonder how id's are generated and are guaranteed unique. I assume they are unique! Just because it's name is "id" means nothing. The code assumes that I am assuming that id's are unique and are used for object identity. That a heluva lot of assuming all around!!

Definitively declare the uniqueness concept in the class design by overriding Equals. This leverages the often untapped power of .NET collections - they automatically use the override in Contains(), Find(), etc.

Embedding desired class behavior in the class itself makes that behavior consistant, enforced, and required (so to speak) throughout the application. Relying on client code to assume desired behavior and to implement it consistantly is the recipe for a lifetime of debugging frustration.

Oh, the final gross assumption is that DB data is consistant and valid. Your class design should enforce all the business rules. IMHO if there is some redundancy in the DB, that's fine. But realize that relational database integrity and business rules are different things.


public class Group

Rewrite properties as shown above. Don't allow client code to set properties, as a general rule.

public List<int> GetAllMembers - I'd rename this to AllMembers because it is a property. If it were a method then leave the name as is.

if (IsMember(id) || GroupType == GroupType.Locked && Requests.Contains(id)) - This is ambiguous because I am unfamiliar with your business domain. Put parentheses to clarify.


Incomplete or Incoherent Design

I see GroupMembers (make it singular) but no classes for admins and requests. Is GroupMembers used?

TakeAdmin() - what does it mean to "take" an admin?

MakeAdmin & AddMember - they should both be "Makexxx" or "Addxxxx". And "MakeAdmin" suggests that it makes an Admin object, but there is no Admin class to make it from.

Why not instantiate member, manager, and request objects? You are forcing the client code to instantiate them. That's a guaranteed way to corrupt the integrity of your data.

public List<int> Members;
public List<int> Requests;
public List<int> Administrators;

Lots of internal coupling: AddMember also fusses with requests and administrators. TakeAdmin also adds members.MakeAdmin removes member(s). DB code is intertwined. As a matter of code structure and organization I don't have confidence in correct member/admin/request CRUDing.

A cursory code skim looks like all administrators are members and vice versa, and there are separate lists for these. Perhaps the member class should have a IsAdmin property. Synchronizing (partially) redundant lists is problematic. Even if the DB has separate entities the domain model should expose the "a member may also be an admin" concept coherently.


Refactor DB calls out of classes

i.e. Separation of concerns; Single Responsibility Principle; Maximize cohesion and minimize coupling.

To facilitate this, you may want to design "data transfer objects (DTO)" classes (collections) to pass to your new database code layer. The idea is an object of data w/o little or no behavior; simply for the purpose of cleanly and conveniently passing data. And certainly your classes can integrate these easily.

You may want to think about making these DTOs inner classes so Group or GroupManager, as appropriate, has total control over them.

public class Group {
    public GroupDto MyData { get; protected set; }
}

public class GroupDto {
    public ConcurrentDictionary<int, Group> Groups { get; protected set; };
    public List<GroupBadgeParts> Bases { get; protected set; };
    public List<GroupBadgeParts> Symbols { get; protected set; };
    // etc.
}

Once refactored, put try/catch around the database code. Catch specific exceptions thrown by your database engine.

P.S.DTO mis-application leads to bad code: property-only classes manipulated by massive "manager" classes containing complex/nested control structures in every method; with a sense of redundant repetition everywhere.


return "";

String.Empty is preferred, less error prone.


DB call control flow

Right in the middle of GroupManager.TryCreateGroup(), Group.MakeAdmin (and member) gets called. What happens if there is a partial failure somewhere? I don't see clean recovery here. At the very least I'd separate group, admin, member DB calls for better flow control. Is it essential that an Admin gets set in the DB before creating the group is finished?

In any case look into your DB's support for transactions. I'm not saying you need transactional behavior, that's a design analysis issue.

\$\endgroup\$

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.