3
\$\begingroup\$

I have coded a fully working permission system for my gaming emulator to determine permissions for users.

What I really want to know is:

  • Is my code standard good enough?
  • Is the design of the class good? (Not sure about so many Initialize methods)
  • Are there any parts of the classes that can be improved (even small improvements)?
  • Is there anything I could've done better?

PermissionManager:

using System;
using System.Collections.Generic;
using System.Data;
using System.Diagnostics;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Sahara.Core.Database;
using Sahara.Core.Logging;

namespace Sahara.Base.Game.Permissions
{
    internal class PermissionManager
    {
        private readonly Dictionary<int, Permission> _permissions;
        private readonly Dictionary<string, PermissionCommand> _permissionCommands;
        private readonly Dictionary<int, PermissionGroup> _permissionGroups;
        private readonly Dictionary<int, List<string>> _permissionGroupRights;
        private readonly Dictionary<int, List<string>> _permissionSubscriptionRights;
        private readonly LogManager _logManager;

        public PermissionManager()
        {
            _permissions = new Dictionary<int, Permission>();
            _permissionCommands = new Dictionary<string, PermissionCommand>();
            _permissionGroups = new Dictionary<int, PermissionGroup>();
            _permissionGroupRights = new Dictionary<int, List<string>>();
            _permissionSubscriptionRights = new Dictionary<int, List<string>>();
            _logManager = Sahara.GetServer().GetLogManager();

            InitializePermissions();
        }

        private void InitializePermissions()
        {
            using (var mysqlConnection = Sahara.GetServer().GetMySql().GetConnection())
            {
                LoadPermissions(mysqlConnection);
                LoadCommandPermissions(mysqlConnection);
                LoadPermissionGroups(mysqlConnection);
                LoadPermissionRights(mysqlConnection);
                LoadPermissionSubscriptions(mysqlConnection);
            }
        }

        private void LoadPermissions(DatabaseConnection mysqlConnection)
        {
            try
            {
                var stopwatch = Stopwatch.StartNew();

                mysqlConnection.SetQuery("SELECT * FROM `permissions`");
                var permissionTable = mysqlConnection.GetTable();

                if (permissionTable == null)
                {
                    return;
                }

                foreach (DataRow permissionRow in permissionTable.Rows)
                {
                    _permissions.Add(Convert.ToInt32(permissionRow["id"]), new Permission(Convert.ToInt32(permissionRow["id"]), Convert.ToString(permissionRow["permission"])));
                }

                stopwatch.Stop();
                _logManager.Log("Loaded " + _permissions.Count + " permissions [" + stopwatch.ElapsedMilliseconds + "ms]", LogType.Information);
            }
            catch (Exception exception)
            {
                var method = System.Reflection.MethodBase.GetCurrentMethod().Name;
                _logManager.Log("Error in " + method + ": " + exception.Message, LogType.Error);
                _logManager.Log(exception.StackTrace, LogType.Error);
            }
        }

        private void LoadCommandPermissions(DatabaseConnection mysqlConnection)
        {
            try
            {
                var stopwatch = Stopwatch.StartNew();

                mysqlConnection.SetQuery("SELECT * FROM `permissions_commands`");
                var permissionTable = mysqlConnection.GetTable();

                if (permissionTable == null)
                {
                    return;
                }

                foreach (DataRow permissionRow in permissionTable.Rows)
                {
                    _permissionCommands.Add(Convert.ToString(permissionRow["command"]), new PermissionCommand(Convert.ToString(permissionRow["command"]), Convert.ToInt32(permissionRow["group_id"]), Convert.ToInt32(permissionRow["subscription_id"])));
                }

                stopwatch.Stop();
                _logManager.Log("Loaded " + _permissionCommands.Count + " command permissions [" + stopwatch.ElapsedMilliseconds + "ms]", LogType.Information);
            }
            catch (Exception exception)
            {
                var method = System.Reflection.MethodBase.GetCurrentMethod().Name;
                _logManager.Log("Error in " + method + ": " + exception.Message, LogType.Error);
                _logManager.Log(exception.StackTrace, LogType.Error);
            }
        }

        private void LoadPermissionGroups(DatabaseConnection mysqlConnection)
        {
            try
            {
                var stopwatch = Stopwatch.StartNew();

                mysqlConnection.SetQuery("SELECT * FROM `permissions_groups`");
                var permissionGroupsTable = mysqlConnection.GetTable();

                if (permissionGroupsTable == null)
                {
                    return;
                }

                foreach (DataRow permissionRow in permissionGroupsTable.Rows)
                {
                    _permissionGroups.Add(Convert.ToInt32(permissionRow["id"]), new PermissionGroup(Convert.ToString(permissionRow["name"]), Convert.ToString(permissionRow["badge"])));
                }

                stopwatch.Stop();
                _logManager.Log("Loaded " + _permissionGroups.Count + " permission groups [" + stopwatch.ElapsedMilliseconds + "ms]", LogType.Information);
            }
            catch (Exception exception)
            {
                var method = System.Reflection.MethodBase.GetCurrentMethod().Name;
                _logManager.Log("Error in " + method + ": " + exception.Message, LogType.Error);
                _logManager.Log(exception.StackTrace, LogType.Error);
            }
        }

        private void LoadPermissionRights(DatabaseConnection mysqlConnection)
        {
            try
            {
                var stopwatch = Stopwatch.StartNew();

                mysqlConnection.SetQuery("SELECT * FROM `permissions_rights`");
                var permissionRightsTable = mysqlConnection.GetTable();

                if (permissionRightsTable == null)
                {
                    return;
                }

                foreach (DataRow permissionRow in permissionRightsTable.Rows)
                {
                    var groupId = Convert.ToInt32(permissionRow["group_id"]);

                    if (!_permissionGroups.ContainsKey(groupId))
                    {
                        continue;
                    }

                    Permission newPermission = null;
                    var permissionId = Convert.ToInt32(permissionRow["permission_id"]);

                    if (!_permissions.TryGetValue(permissionId, out newPermission))
                    {
                        continue;
                    }

                    if (_permissionGroupRights.ContainsKey(groupId))
                    {
                        _permissionGroupRights[groupId].Add(newPermission.PermissionName);
                    }
                    else
                    {
                        var permissionRightSets = new List<string> {newPermission.PermissionName};
                        _permissionGroupRights.Add(groupId, permissionRightSets);
                    }
                }

                stopwatch.Stop();
                _logManager.Log("Loaded " + _permissionGroupRights.Count + " permission group rights [" + stopwatch.ElapsedMilliseconds + "ms]", LogType.Information);
            }
            catch (Exception exception)
            {
                var method = System.Reflection.MethodBase.GetCurrentMethod().Name;
                _logManager.Log("Error in " + method + ": " + exception.Message, LogType.Error);
                _logManager.Log(exception.StackTrace, LogType.Error);
            }
        }

        private void LoadPermissionSubscriptions(DatabaseConnection mysqlConnection)
        {
            try
            {
                var stopwatch = Stopwatch.StartNew();

                mysqlConnection.SetQuery("SELECT * FROM `permissions_subscriptions`");
                var permissionRightsTable = mysqlConnection.GetTable();

                if (permissionRightsTable == null)
                {
                    return;
                }

                foreach (DataRow permissionRow in permissionRightsTable.Rows)
                {
                    Permission newPermission = null;
                    var permissionId = Convert.ToInt32(permissionRow["permission_id"]);

                    if (!_permissions.TryGetValue(permissionId, out newPermission))
                    {
                        continue;
                    }

                    var subscriptionId = Convert.ToInt32(permissionRow["subscription_id"]);

                    if (_permissionSubscriptionRights.ContainsKey(subscriptionId))
                    {
                        _permissionSubscriptionRights[subscriptionId].Add(newPermission.PermissionName);
                    }
                    else
                    {
                        var permissionRightSets = new List<string> { newPermission.PermissionName };
                        _permissionSubscriptionRights.Add(subscriptionId, permissionRightSets);
                    }
                }

                stopwatch.Stop();
                _logManager.Log("Loaded " + _permissionSubscriptionRights.Count + " permission subscription rights [" + stopwatch.ElapsedMilliseconds + "ms]", LogType.Information);
            }
            catch (Exception exception)
            {
                var method = System.Reflection.MethodBase.GetCurrentMethod().Name;
                _logManager.Log("Error in " + method + ": " + exception.Message, LogType.Error);
                _logManager.Log(exception.StackTrace, LogType.Error);
            }
        }
    }
}

Permission:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Sahara.Base.Game.Permissions
{
    internal class Permission
    {
        private readonly int _permissionId;
        private readonly string _permissionName;

        public Permission(int permissionId, string permissionName)
        {
            _permissionId = permissionId;
            _permissionName = permissionName;
        }

        public string PermissionName => _permissionName;
    }
}

PermissionCommand:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Sahara.Base.Game.Permissions
{
    internal class PermissionCommand
    {
        private readonly string _commandName;
        private readonly int _groupId;
        private readonly int _subscriptionIdRequired;

        public PermissionCommand(string commandName, int groupId, int subscriptionIdRequired)
        {
            _commandName = commandName;
            _groupId = groupId;
            _subscriptionIdRequired = subscriptionIdRequired;
        }
    }
}

PermissionGroup:

namespace Sahara.Base.Game.Permissions
{
    internal class PermissionGroup
    {
        private readonly string _permissionGroupName;
        private readonly string _permissionBadgeRequired;

        public PermissionGroup(string permissionGroupName, string permissionBadgeRequired)
        {
            _permissionGroupName = permissionGroupName;
            _permissionBadgeRequired = permissionBadgeRequired;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ just out of curiosity: why don't you create an account? :-] \$\endgroup\$ – t3chb0t Oct 28 '16 at 17:53
  • \$\begingroup\$ I have a really big tendancy of forgetting my passwords, so I have learnt to just make a new account and keep logged in, at some point I'll get around to fully creating this account and hopefully keep it. \$\endgroup\$ – Liam Oct 28 '16 at 17:55
  • \$\begingroup\$ haha, ok, I noticed there are quite many of you :-] \$\endgroup\$ – t3chb0t Oct 28 '16 at 17:57
  • \$\begingroup\$ The only public thing about the PermissionManager is its constructor... how do you use this object? ;-| I'm confused. This can't possibly work. \$\endgroup\$ – t3chb0t Oct 28 '16 at 19:34
  • \$\begingroup\$ When I am done with it, it will have methods such as TryGetPermission() \$\endgroup\$ – Liam Oct 28 '16 at 19:41
4
\$\begingroup\$

Default class access modifier

Unless otherwise specified, classes in C# are internal by default. This means that you can remove the internal access modifier in places like these:

internal class PermissionManager
internal class Permission
internal class PermissionCommand

However, if you like explicitly specifying the internal access modifier, then you can keep it.


Lambda expressions => properties

In your Permission class you declare the following lambda expression which returns the private readonly field _permissionName:

public string PermissionName => _permissionName;

If you've added this to allow for the reading of the private field, but not writing, then you can simply turn _permissionName into a property with a private setter, like so:

public string PermissionName { get; private set; }

If this wasn't your original intent, then you can keep the lambda expression.


Proper string formatting

There are many places in your code where you are concatenating values together into strings, like this:

_logManager.Log("Loaded " + _permissions.Count + " permissions [" + stopwatch.ElapsedMilliseconds + "ms]", LogType.Information);

Instead of doing this you can use string.Format() or string interpolation. Either of these two methods will increase the readability of your code. Using string.Format(), the above code will become this:

_logManager.Log(string.Format("Loaded {0} permissions [{1}ms]", _permissions.Count, stopwatch.ElapsedMilliseconds), LogType.Information);

If you use string interpolation, then your code will become this:

_logManager.Log($"Loaded {_permissions.Count} permissions [{stopwatch.ElapsedMilliseconds}ms]", LogType.Information);

Take note of the "$" at the beginning of the string in the above example.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I appreciate you taking the time to answer my question, nice touch with the string interpolation, I think I'll be doing this full time now. \$\endgroup\$ – Liam Oct 28 '16 at 19:01
2
\$\begingroup\$

I am really visiting this question based on MySQL tag, so I wanted to add a thought in this area.

You should consider moving away from SELECT * queries against any sort of relational databases. These can cause problems by:

  • making your application pull in more data than is actually necessary to do whatever the function at hand is related to the query.
  • making the code harder to understand - a developer has to go actually look at the database table to understand what fields are going to be available on every record.
  • making your application fragile to changes in the DB schema. If you had a section of code that was, for example mapping DB field value to class properties, the sudden addition of a new column on the table could break your code.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Great answer on the MySQL side, thanks for the tip. \$\endgroup\$ – Liam Oct 28 '16 at 19:43

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.