1
\$\begingroup\$

Here is a moderation library that I have coded and would like some help in improving the quality of my code.

Most of the code is styled by rules of Resharper; Resharper has helped me out a lot with my naming conventions, redundant code and much more.

It consists of 5 classes:

  • ModerationManager
  • ModerationBan
  • ModerationBanType
  • ModerationPresetActionMessage
  • ModerationTicket

I know the quality of my other questions haven't been very good and I have tried improving the quality here by including all the code that is involved to hopefully get some good reviews.

ModerationManager:

using Sahara.Base.Utility;
using Sahara.Core.Database;
using Sahara.Core.Logging;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Data;
using System.Diagnostics;
using System.Linq;

namespace Sahara.Base.Game.Moderation
{
    internal class ModerationManager
    {
        private int _moderationTicketCount;
        private readonly List<string> _playerPresets;
        private readonly List<string> _roomPresets;
        private readonly Dictionary<string, ModerationBan> _moderationBans;
        private readonly Dictionary<int, string> _playerActionPresetCategories;
        private readonly Dictionary<int, List<ModerationPresetActionMessage>> _playerActionPresetMessages;
        private readonly ConcurrentDictionary<int, ModerationTicket> _moderationTickets;
        private readonly LogManager _logManager;

        public ModerationManager()
        {
            _moderationTicketCount = 1;
            _playerPresets = new List<string>();
            _roomPresets = new List<string>();
            _moderationBans = new Dictionary<string, ModerationBan>();
            _playerActionPresetCategories = new Dictionary<int, string>();
            _playerActionPresetMessages = new Dictionary<int, List<ModerationPresetActionMessage>>();
            _moderationTickets = new ConcurrentDictionary<int, ModerationTicket>();
            _logManager = new LogManager();

            LoadModeration();
        }

        private void LoadModeration()
        {
            using (var mysqlConnection = Sahara.GetServer().GetMySql().GetConnection())
            {
                LoadModerationPresets(mysqlConnection);
                LoadModerationCategorys(mysqlConnection);
                LoadModerationMessages(mysqlConnection);
                LoadModerationBans(mysqlConnection);
            }
        }

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

                mysqlConnection.SetQuery("SELECT * FROM `moderation_presets`");
                var moderationPresetsTable = mysqlConnection.GetTable();

                if (moderationPresetsTable != null)
                {
                    foreach (DataRow presetRow in moderationPresetsTable.Rows)
                    {
                        var presetType = Convert.ToString(presetRow["type"]).ToLower();
                        var message = Convert.ToString(presetRow["message"]);

                        switch (presetType)
                        {
                            case "user":
                                _playerPresets.Add(message);
                                break;
                            default:
                                _roomPresets.Add(message);
                                break;
                        }
                    }
                }

                stopwatch.Stop();
                _logManager.Log(
                    $"Loaded {_playerPresets.Count + _roomPresets.Count} moderation presets [{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 LoadModerationCategorys(DatabaseConnection mysqlConnection)
        {
            try
            {
                var stopwatch = Stopwatch.StartNew();

                mysqlConnection.SetQuery("SELECT * FROM `moderation_preset_action_categories`");
                var moderationCategorysTable = mysqlConnection.GetTable();

                if (moderationCategorysTable != null)
                {
                    foreach (DataRow presetRow in moderationCategorysTable.Rows)
                    {
                        _playerActionPresetCategories.Add(Convert.ToInt32(presetRow["id"]), Convert.ToString(presetRow["caption"]));
                    }
                }

                stopwatch.Stop();
                _logManager.Log($"Loaded {_playerActionPresetCategories.Count} moderation categorys [{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 LoadModerationMessages(DatabaseConnection mysqlConnection)
        {
            try
            {
                var stopwatch = Stopwatch.StartNew();

                mysqlConnection.SetQuery("SELECT * FROM `moderation_preset_action_messages`");
                var moderationMessagesTable = mysqlConnection.GetTable();

                if (moderationMessagesTable != null)
                {
                    foreach (DataRow presetRow in moderationMessagesTable.Rows)
                    {
                        var parentId = Convert.ToInt32(presetRow["parent_id"]);

                        if (!_playerActionPresetMessages.ContainsKey(parentId))
                        {
                            _playerActionPresetMessages.Add(parentId, new List<ModerationPresetActionMessage>());
                        }

                        _playerActionPresetMessages[parentId].Add(new ModerationPresetActionMessage(Convert.ToInt32(presetRow["id"]), Convert.ToInt32(presetRow["parent_id"]), Convert.ToString(presetRow["caption"]), Convert.ToString(presetRow["message_text"]), Convert.ToInt32(presetRow["mute_hours"]), Convert.ToInt32(presetRow["ban_hours"]), Convert.ToInt32(presetRow["ip_ban_hours"]), Convert.ToInt32(presetRow["trade_lock_days"]), Convert.ToString(presetRow["notice"])));
                    }
                }

                stopwatch.Stop();
                _logManager.Log($"Loaded {_playerActionPresetMessages.Count} moderation messages [{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 LoadModerationBans(DatabaseConnection mysqlConnection)
        {
            try
            {
                var stopwatch = Stopwatch.StartNew();

                mysqlConnection.SetQuery("SELECT `bantype`, `value`, `reason`, `expire` FROM `bans` WHERE `bantype` = 'machine' OR `bantype` = 'user'");
                var moderationBanTable = mysqlConnection.GetTable();

                if (moderationBanTable != null)
                {
                    foreach (DataRow banRow in moderationBanTable.Rows)
                    {
                        var banValue = Convert.ToString(banRow["value"]);
                        var banReason = Convert.ToString(banRow["reason"]);
                        var banExpirationDate = (double)banRow["expire"];
                        var banType = Convert.ToString(banRow["bantype"]);
                        var banTypeValue = (banType == "ip" ? ModerationBanType.ByIp : banType == "machine" ? ModerationBanType.ByMachine : ModerationBanType.ByUsername);
                        var ban = new ModerationBan(banTypeValue, banValue, banReason, banExpirationDate);

                        if (banExpirationDate <= UnixTimestampGenerator.GetNow())
                        {
                            mysqlConnection.SetQuery("DELETE FROM `bans` WHERE `bantype` = '" + banType + "' AND `value` = @Key LIMIT 1");
                            mysqlConnection.AddParameter("Key", banValue);
                            mysqlConnection.RunQuery();
                        }
                        else if (!_moderationBans.ContainsKey(banValue))
                        {
                            _moderationBans.Add(banValue, ban);
                        }
                    }
                }

                stopwatch.Stop();
                _logManager.Log($"Loaded {_moderationBans.Count} moderation bans [{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);
            }
        }

        public void AddBan(string moderatorUsername, ModerationBanType banType, string banValue, string banReason, double expireTimestamp)
        {
            var type = (banType == ModerationBanType.ByIp ? "ip" : banType == ModerationBanType.ByMachine ? "machine" : "user");

            using (var mysqlConnection = Sahara.GetServer().GetMySql().GetConnection())
            {
                mysqlConnection.SetQuery("REPLACE INTO `bans` (`bantype`, `value`, `reason`, `expire`, `added_by`, `added_date`) VALUES (@banType, @banValue, @reason, @expireTimestamp, @moderatorUsername, @addedTimestamp)");
                mysqlConnection.AddParameter("banType", type);
                mysqlConnection.AddParameter("banValue", banValue);
                mysqlConnection.AddParameter("reason", banReason);
                mysqlConnection.AddParameter("expireTimestamp", expireTimestamp);
                mysqlConnection.AddParameter("moderatorUsername", moderatorUsername);
                mysqlConnection.AddParameter("addedTimestamp", UnixTimestampGenerator.GetNow());
                mysqlConnection.RunQuery();
            }

            if (banType != ModerationBanType.ByMachine && banType != ModerationBanType.ByUsername)
            {
                return;
            }

            if (!_moderationBans.ContainsKey(banValue))
            {
                _moderationBans.Add(banValue, new ModerationBan(banType, banValue, banReason, expireTimestamp));
            }
        }

        public ICollection<string> PlayerMessagePresets => _playerPresets;
        public ICollection<string> RoomMessagePresets => _roomPresets;
        public ICollection<ModerationTicket> GetTickets => _moderationTickets.Values;

        public Dictionary<string, List<ModerationPresetActionMessage>> PlayerActionPresets
        {
            get
            {
                var resultDictionary = new Dictionary<string, List<ModerationPresetActionMessage>>();

                foreach (var categoryKeyValuePair in this._playerActionPresetCategories.ToList())
                {
                    resultDictionary.Add(categoryKeyValuePair.Value, new List<ModerationPresetActionMessage>());

                    if (!this._playerActionPresetCategories.ContainsKey(categoryKeyValuePair.Key))
                    {
                        continue;
                    }

                    foreach (var data in this._playerActionPresetMessages[categoryKeyValuePair.Key])
                    {
                        resultDictionary[categoryKeyValuePair.Value].Add(data);
                    }
                }

                return resultDictionary;
            }
        }

        public bool TryAddModerationTicket(ModerationTicket moderationTicket)
        {
            moderationTicket.TicketId = _moderationTicketCount++;
            return _moderationTickets.TryAdd(moderationTicket.TicketId, moderationTicket);
        }

        public bool TryGetModerationTicket(int ticketId, out ModerationTicket ticket)
        {
            return _moderationTickets.TryGetValue(ticketId, out ticket);
        }

        private bool BanValueFound(string banValue, out ModerationBan moderationBan)
        {
            if (!_moderationBans.TryGetValue(banValue, out moderationBan))
            {
                moderationBan = null;
                return false;
            }

            if (!moderationBan.HasExpired)
            {
                return true;
            }

            using (var mysqlConnection = Sahara.GetServer().GetMySql().GetConnection())
            {
                mysqlConnection.SetQuery("DELETE FROM `bans` WHERE `bantype` = @type AND `value` = @value LIMIT 1");
                mysqlConnection.AddParameter("type", moderationBan.BanType == ModerationBanType.ByIp ? "ip" : moderationBan.BanType == ModerationBanType.ByMachine ? "machine" : "username");
                mysqlConnection.AddParameter("value", banValue);
                mysqlConnection.RunQuery();
            }

            if (_moderationBans.ContainsKey(banValue))
            {
                _moderationBans.Remove(banValue);
            }

            return false;
        }

        public bool MachineBanFound(string machineId)
        {
            ModerationBan machineBan;

            if (!BanValueFound(machineId, out machineBan))
            {
                return true;
            }

            using (var mysqlConnection = Sahara.GetServer().GetMySql().GetConnection())
            {
                mysqlConnection.SetQuery("SELECT * FROM `bans` WHERE `bantype` = 'machine' AND `value` = @value LIMIT 1");
                mysqlConnection.AddParameter("value", machineId);
                var banRow = mysqlConnection.GetRow();

                if (banRow != null)
                {
                    return true;
                }

                RemoveBan(machineId);
                return false;
            }
        }

        public bool UsernameBanFound(string username)
        {
            ModerationBan usernameBan;

            if (!BanValueFound(username, out usernameBan))
            {
                return true;
            }

            using (var mysqlConnection = Sahara.GetServer().GetMySql().GetConnection())
            {
                mysqlConnection.SetQuery("SELECT * FROM `bans` WHERE `bantype` = 'user' AND `value` = @value LIMIT 1");
                mysqlConnection.AddParameter("value", username);
                var banRow = mysqlConnection.GetRow();

                if (banRow != null)
                {
                    return true;
                }

                RemoveBan(username);
                return false;
            }
        }

        private void RemoveBan(string banValue)
        {
            _moderationBans.Remove(banValue);
        }
    }
}

ModerationBan:

    using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Sahara.Base.Utility;

namespace Sahara.Base.Game.Moderation
{
    internal class ModerationBan
    {
        private readonly ModerationBanType _banType;
        private readonly string _banValue;
        private readonly string _banReason;
        private readonly double _banExpirationDate;

        public ModerationBan(ModerationBanType banType, string banValue, string banReason, double banExpirationDate)
        {
            _banType = banType;
            _banValue = banValue;
            _banReason = banReason;
            _banExpirationDate = banExpirationDate;
        }

        public ModerationBanType BanType => _banType;
        public bool HasExpired => UnixTimestampGenerator.GetNow() >= _banExpirationDate;
    }
}

ModerationBanType:

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

namespace Sahara.Base.Game.Moderation
{
    public enum ModerationBanType
    {
        ByIp = 1,
        ByMachine = 2,
        ByUsername = 3
    }
}

ModerationPresetActionMessage:

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

namespace Sahara.Base.Game.Moderation
{
    class ModerationPresetActionMessage
    {
        private readonly int _id;
        private readonly int _parentId;
        private readonly string _caption;
        private readonly string _messageContent;
        private readonly int _muteTime;
        private readonly int _banTime;
        private readonly int _ipBanTime;
        private readonly int _tradeLockTime;
        private readonly string _noticeMessage;

        public ModerationPresetActionMessage(int id, int parentId, string caption, string messageContent, int muteTime, int banTime, int ipBanTime, int tradeLockTime, string noticeMessage)
        {
            _id = id;
            _parentId = parentId;
            _caption = caption;
            _messageContent = messageContent;
            _muteTime = muteTime;
            _banTime = banTime;
            _ipBanTime = ipBanTime;
            _tradeLockTime = tradeLockTime;
            _noticeMessage = noticeMessage;
        }

        public int Id => _id;
        public int ParentId => _parentId;
        public string Caption => _caption;
        public string MessageContent => _messageContent;
        public int MuteTime => _muteTime;
        public int BanTime => _banTime;
        public int IpBanTime => _ipBanTime;
        public int TradeLockTime => _tradeLockTime;
        public string NoticeMessage => _noticeMessage;
    }
}

ModerationTicket:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Sahara.Base.Game.Players;
using Sahara.Base.Game.Rooms;

namespace Sahara.Base.Game.Moderation
{
    class ModerationTicket
    {
        private int _ticketId;
        private readonly int _ticketType;
        private readonly double _ticketTimestamp;
        private readonly int _ticketCategory;
        private readonly int _ticketPriority;
        private readonly bool _ticketAnswered;
        private readonly PlayerData _ticketSender;
        private readonly PlayerData _ticketTarget;
        private readonly PlayerData _moderator;
        private readonly string _issue;
        private readonly RoomInformation _ticketRoomInformation;

        public ModerationTicket(int ticketId, int ticketType, double ticketTimestamp, int ticketCategory, int ticketPriority, PlayerData ticketSender, PlayerData ticketTarget, string issue, RoomInformation ticketRoomInformation)
        {
            _ticketId = ticketId;
            _ticketType = ticketType;
            _ticketTimestamp = ticketTimestamp;
            _ticketCategory = ticketCategory;
            _ticketPriority = ticketPriority;
            _ticketAnswered = false;
            _ticketSender = ticketSender;
            _ticketTarget = ticketTarget;
            _moderator = null;
            _issue = issue;
            _ticketRoomInformation = ticketRoomInformation;
        }

        public int TicketId
        {
            get { return _ticketId; }
            set { _ticketId = value; }
        }

        public int TicketType => _ticketType;
        public double TicketTimestamp => _ticketTimestamp;
        public int TicketCategory => _ticketCategory;
        public int TicketPriority => _ticketPriority;
        public bool TicketAnswered => _ticketAnswered;
        public PlayerData TicketSender => _ticketSender;
        public PlayerData TicketTarget => _ticketTarget;
        private PlayerData Moderator => _moderator;
        public string Issue => _issue;
        public RoomInformation TicketRoomInformation => _ticketRoomInformation;

        public int GetTicketStatus(int id)
        {
            if (Moderator == null)
            {
                return -1;
            }

            if (Moderator.PlayerId == id && _ticketAnswered)
            {
                return 2;
            }

            return 3;
        }
    }
}

Here I have included some of the code usage of this library. This code isn't really in the review but to show you how this library is used:

Some usage:

//Let's have a quick search for a ban before we successfully authenticate..
ModerationBan BanRecord = null;
if (!string.IsNullOrEmpty(MachineId))
{
    if (Sahara.GetServer().GetGame().GetModerationManager().IsBanned(MachineId, out BanRecord))
    {
        if (Sahara.GetServer().GetGame().GetModerationManager().MachineBanCheck(MachineId))
        {
            Disconnect();
            return false;
        }
    }
}

if (userData.user != null)
{
    //Now let us check for a username ban record..
    BanRecord = null;
    if (Sahara.GetServer().GetGame().GetModerationManager().IsBanned(userData.user.Username, out BanRecord))
    {
        if (Sahara.GetServer().GetGame().GetModerationManager().UsernameBanCheck(userData.user.Username))
        {
            Disconnect();
            return false;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Reader is typically more efficient than table if all you need is to read the values \$\endgroup\$ – paparazzo Oct 29 '16 at 18:32
  • \$\begingroup\$ I would suggest that you provide more context as to what your library does, so that readers will have some idea what they are looking for. Also, Maybe provide a short description of each class and its responsibility, providing a link to some github repo on which the code resides.. It would make your post much shorter and less cumbersome to read. \$\endgroup\$ – Eyal Perry Oct 29 '16 at 18:51
5
\$\begingroup\$

Off the top of my head:

1. Identify the Entities

As a rule of thumb, identify the entities participating in your application domain and create types which aggregate (as properties) data that is relevant to each entity.

Use these entities instead of primitives (or strings, for this matter). Doing so helps promote readability and overall robustness of the system.

What do you think is more readable, this:

private readonly List<string> _playerPresets;

Or this:

private readonly List<PlayerPreset> _playerPresets;

I would have no idea, when examining the former field, what kind of information does the string denote, whereas the latter approach forces you to "populate" the entity object with properties, where each property is properly described by its name.

Also, consider which of the above approaches would better scale as more features are required from the application?

If your entire application would just expect strings and other primitives instead of entity objects, then in the case where additional information would be required, you would have to refactor every method signature where the said primitive is expected. This becomes quite the effort as time goes by and the application becomes complex.

Consider how easy it would be refactoring a code base which expects entities. You simply add a property to the entity, and add code which processes the property values.

Your interface does not have to change as a result, which is especially crucial when creating libraries.

2. Consider the DAL and DAO patterns

A DAL (Data Access Layer) is a layer (a set) of types which abstract the means of acquiring data from business (application logic) code. Which means that your app does not care whether the data came from a file on disc, a database, a web API or from a dummy data provider in a test environment.

A DAO (Data Access Object) serves as a layer of abstraction between a concrete DAL implementation and a specific persistence mechanism. (i.e. Entity Framework abstracts the fact that an SQL server even exists, exposing data via LINQ and having types which implement the IQueryable interface).

3. Programming against an Abstraction

Definition: Type A is dependent on type B if type A consumes members of type B.

When consuming a type directly (instead of by means of an interface), you create a dependency on it- you can not remove it from your code base, or replace it with another without breaking compilation. Also, the functionality of the dependent type becomes tightly coupled with its dependency. Furthermore, you are not capable of "tweaking" the dependent type's behavior by means of polymorphism.

Instead of directly consuming a type, consider defining an interface with common members which other types would implement. In the consuming type, define a field of the interface's type and consume it. this would make your code less coupled with a concrete dependency's implementation.

4. Consider Dependency Injection

Sorry, ran out of juice for today - But there are plenty of articles out there.

| improve this answer | |
\$\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.