2
\$\begingroup\$

As of now, instead of the previous 4 or 5 out of 10 (relative to my C# programming skill), I'd say I'm a solid 6, and some of that is thanks to the advice I got from the last revision.

I realize the code is very long, and I tried to condense it all down to what was necessary (referenced in these main files). The full project can be found on GitHub here.

Eve.cs, primary file

User class

public class User {
    public User() {
        Messages = new List<Message>();
    }

    public int Id { get; set; }
    public string Nickname { get; set; }
    public string Realname { get; set; }
    public int Access { get; set; }
    public DateTime Seen { get; set; }
    public List<Message> Messages { get; set; }
}

Message class

public class Message {
    public string Sender { get; set; }
    public string Contents { get; set; }
    public DateTime Date { get; set; }
}

Config class

public class Config {
    public bool Joined { get; set; }
    public bool Identified { get; set; }
    public string Server { get; set; }
    public string[] Channels { get; set; }
    public string Nick { get; set; }
    public string Name { get; set; }
    public int Port { get; set; }
}

Variables class

public class Variables : IDisposable {
    public Variables(bool initDb) {
        if (!initDb) return;

        if (!File.Exists("users.sqlite")) {
            Console.WriteLine("||| Users database does not exist. Creating database.");

            SQLiteConnection.CreateFile("users.sqlite");

            Db = new SQLiteConnection("Data Source=users.sqlite;Version=3;");
            Db.Open();

            SQLiteCommand com = new SQLiteCommand(
                "CREATE TABLE users (int id, string nickname, string realname, int access, string seen)", Db);
            SQLiteCommand com2 =
                new SQLiteCommand("CREATE TABLE messages (int id, string sender, string message, string datetime)", Db);
            com.ExecuteNonQuery();
            com2.ExecuteNonQuery();
        }
        else
            try {
                Db = new SQLiteConnection("Data Source=users.sqlite;Version=3;");
                Db.Open();
            }
            catch (Exception e) {
                Console.WriteLine("||| Unable to connec to database, error: " + e);
            }

        using (SQLiteCommand a = new SQLiteCommand("SELECT COUNT(id) FROM users", Db))
            if (Convert.ToInt32(a.ExecuteScalar()) == 0) {
                Console.WriteLine("||| Users table in database is empty. Creating initial record.");

                using (
                    SQLiteCommand b =
                        new SQLiteCommand("INSERT INTO users VALUES (0, 'semiviral', 'semiviral', 0) ",
                            Db))
                    b.ExecuteNonQuery();
            }

        using (SQLiteDataReader d = new SQLiteCommand("SELECT * FROM users", Db).ExecuteReader()) {
            while (d.Read())
                Users.Add(new User {
                    Id = (int) d["id"],
                    Nickname = (string) d["nickname"],
                    Realname = (string) d["realname"],
                    Access = (int) d["access"],
                    Seen = DateTime.Parse((string) d["seen"])
                });

            try {
                using (SQLiteDataReader m = new SQLiteCommand("SELECT * FROM messages", Db).ExecuteReader())
                    while (m.Read())
                        Users.FirstOrDefault(e => e.Id == Convert.ToInt32(m["id"]))?.Messages.Add(new Message {
                            Sender = (string) m["sender"],
                            Contents = (string) m["message"],
                            Date = DateTime.Parse((string) m["datetime"])
                        });
            }
            catch (NullReferenceException) {
                Console.WriteLine(
                    "||| NullReferenceException occured upon loading messages from database. This most likely means a user record was deleted and the ID cannot be referenced from the message entry.");
            }

            if (Users != null) return;

            Console.WriteLine("||| Failed to read from database.");
        }
    }

    public void Dispose() {
        Db?.Close();
    }

    public User QueryName(string name) {
        return Users.FirstOrDefault(e => e.Realname == name);
    }

    #region Variable initializations

    public SQLiteConnection Db;
    public User CurrentUser = new User();

    public string Info =
        "Evealyn is a utility IRC bot created by SemiViral as a primary learning project for C#. Version 2.0";

    public List<User> Users = new List<User>();
    public readonly List<string> Channels = new List<string>();

    public readonly List<string> IgnoreList = new List<string> {
        "eve",
        "nickserv",
        "chanserv",
        "vervet.foonetic.net",
        "belay.foonetic.net",
        "anchor.foonetic.net",
        "daemonic.foonetic.net",
        "staticfree.foonetic.net"
    };

    public readonly Dictionary<string, List<string>> UserChannelList = new Dictionary<string, List<string>>();
    public readonly Dictionary<string, int> UserAttempts = new Dictionary<string, int>();
    public readonly Dictionary<string, string> Commands = new Dictionary<string, string>();

    #endregion
}

Primary stuff-happens class, IrcBot

public class IrcBot : IDisposable, IModule {
        private readonly Config _config;
        private TcpClient _connection;
        private StreamWriter _log;
        private NetworkStream _networkStream;
        private StreamReader _streamReader;
        private StreamWriter _streamWriter;

        // set config
        public IrcBot(Config config) {
            _config = config;
        }

        public static Variables V { get; set; } = new Variables(true);

        public static Dictionary<string, Type> Modules { get; set; } = LoadModules();

        public void Dispose() {
            _streamReader?.Close();
            _streamWriter?.Close();
            _networkStream?.Close();
            _log?.Close();
            _connection?.Close();
        }

        public Dictionary<String, String> Def => null;

        public ChannelMessage OnChannelMessage(ChannelMessage c) {
            // 376 is end of MOTD command
            if (c.Type == "376" && !_config.Identified) {
                SendData("PRIVMSG", "NICKSERV IDENTIFY evepass");
                SendData("MODE", "Eve +B");

                foreach (string s in _config.Channels) {
                    SendData("JOIN", s);
                    V.Channels.Add(s);
                }

                _config.Joined = true;
                _config.Identified = true;
            }

            c._Args = c.Args?.Trim().Split(new[] {' '}, 4).ToList();
            c.Target = c.Recipient;

            try {
                foreach (ChannelMessage cm in Modules.Values
                    .Select(m => ((IModule) Activator.CreateInstance(m)).OnChannelMessage(c))) {
                    if (cm == null) continue;

                    var stopLoop = false;
                    switch (cm.ExitType) {
                        case 0: // Immediately end loop
                            stopLoop = true;
                            break;
                        case 1: // End loop after sending message
                            SendData("PRIVMSG", $"{cm.Target} {cm.Message}");
                            stopLoop = true;
                            break;
                    }

                    if (stopLoop) break;

                    if (cm.MultiMessage.Any())
                        foreach (string s in cm.MultiMessage)
                            SendData(cm.Type, $"{cm.Target} {s}");
                    else if (!String.IsNullOrEmpty(cm.Message))
                        SendData(cm.Type, $"{cm.Target} {cm.Message}");

                    c.Reset(c.Recipient);
                }
            }
            catch (Exception e) {
                Console.WriteLine($"||| Error: {e}");
            }
            return null;
        }

        // send raw data to server
        private void SendData(string cmd, string param) {
            try {

                if (param == null) {
                    _streamWriter.WriteLine(cmd);
                    _streamWriter.Flush();
                    Console.WriteLine(cmd);
                }
                else {
                    _streamWriter.WriteLine($"{cmd} {param}");
                    _streamWriter.Flush();

                    Console.WriteLine(cmd == "PONG" ? "Pong" : $"{cmd} {param}");
                }
            }
            catch {
                Console.WriteLine("||| Failed to send message to server. Attempting reconnection.");
                Dispose();
                InitializeConnections();
            }
        }

        // <summary>
        // Method for initialising all data streams
        // </summary>
        public void InitializeConnections() {
            try {
                _connection = new TcpClient(_config.Server, _config.Port);
            }
            catch {
                Console.WriteLine("||| Connection failed.");
                return;
            }

            try {
                _networkStream = _connection.GetStream();
                _streamReader = new StreamReader(_networkStream);
                _streamWriter = new StreamWriter(_networkStream);
                _log = new StreamWriter("./logs.txt", true);

                SendData("USER", $"{_config.Nick} 0 * {_config.Name}");
                SendData("NICK", _config.Nick);
            }
            catch {
                Console.WriteLine("||| Communication error.");
            }
        }

        /// <summary>
        ///     Recieves incoming data, parses it, and passes it to <see cref="OnChannelMessage(ChannelMessage)" />
        /// </summary>
        public void Runtime() {
            String data = _streamReader.ReadLine(); // raw data from stream
            if (data == null) return;

            DateTime messageTime = DateTime.UtcNow;
            Regex messageRegex = new Regex(@"^:(?<Sender>[^\s]+)\s(?<Type>[^\s]+)\s(?<Recipient>[^\s]+)\s?:?(?<Args>.*)",
                RegexOptions.Compiled);
            //var argMessageRegex = new Regex(@"^:(?<Arg1>[^\s]+)\s(?<Arg2>[^\s]+)\s(?<Arg3>[^\s]+)\s?:?(?<Arg4>.*)", RegexOptions.Compiled);
            Regex senderRegex = new Regex(@"^(?<Nickname>[^\s]+)!(?<Realname>[^\s]+)@(?<Hostname>[^\s]+)", RegexOptions.Compiled);
            Regex pingRegex = new Regex(@"^PING :(?<Message>.+)", RegexOptions.None);

            // write raw data if conditions not met
            if (pingRegex.IsMatch(data))
                Console.Write("Ping ... ");

            // write timestamp and raw data to log
            _log.WriteLine($"({DateTime.Now}) {data}");
            _log.Flush();

            if (messageRegex.IsMatch(data)) {
                Match mVal = messageRegex.Match(data);
                String mSender = mVal.Groups["Sender"].Value;
                Match sMatch = senderRegex.Match(mSender);

                // initialise new ChannelMessage to passed into OnChannelMessage()
                ChannelMessage c = new ChannelMessage {
                    Nickname = mSender,
                    Realname = mSender.ToLower(),
                    Hostname = mSender,
                    Type = mVal.Groups["Type"].Value,
                    Recipient = mVal.Groups["Recipient"].Value.StartsWith(":") // Checks if first argument starts with a colon
                        ? mVal.Groups["Recipient"].Value.Substring(1) // if so, remove it
                        : mVal.Groups["Recipient"].Value,
                    Args = mVal.Groups["Args"].Value,
                    Time = messageTime
                };

                // if mVal["Sender"] matches Sender regex, reset the values of ChannelMessage c
                if (sMatch.Success) {
                    String realname = sMatch.Groups["Realname"].Value;
                    c.Nickname = sMatch.Groups["Nickname"].Value;
                    c.Realname = realname.StartsWith("~") ? realname.Substring(1) : realname;
                    c.Hostname = sMatch.Groups["Hostname"].Value;
                }

                // if user exists in database, update their last seen datetime and check if their nickname has changed
                if (V.QueryName(c.Realname) != null) {
                    V.Users.First(e => e.Realname == c.Realname).Seen = messageTime;
                    using (
                        SQLiteCommand com = new SQLiteCommand($"UPDATE users SET seen='{messageTime}' WHERE realname='{c.Realname}'",
                            V.Db))
                        com.ExecuteNonQuery();

                    if (V.CurrentUser != null)
                        if (V.CurrentUser.Nickname != c.Nickname)
                            using (
                                SQLiteCommand com = new SQLiteCommand($"UPDATE users SET nickname='{c.Nickname}' WHERE realname='{c.Realname}'",
                                    V.Db))
                                com.ExecuteNonQuery();
                }
                else if (V.QueryName(c.Realname) == null
                         && !V.IgnoreList.Contains(c.Realname.ToLower())) {
                    // checks if user exists and is also not in the ignoreList
                    Console.WriteLine($"||| User {c.Realname} currently not in database. Creating database entry for user.");

                    int id = -1;

                    // create data adapter to obtain all id's from users table
                    using (SQLiteDataReader x = new SQLiteCommand("SELECT MAX(id) FROM users", V.Db).ExecuteReader())
                        while (x.Read())
                            id = Convert.ToInt32(x.GetValue(0)) + 1;

                    // insert new user record into database
                    using (
                        SQLiteCommand com =
                            new SQLiteCommand(
                                $"INSERT INTO users VALUES ({id}, '{c.Nickname}', '{c.Realname}', 3, '{messageTime}')",
                                V.Db))
                        com.ExecuteNonQuery();

                    // add new User instance to the list of users
                    V.Users.Add(new User {
                        Id = id,
                        Nickname = c.Nickname.ToLower(),
                        Realname = c.Realname,
                        Access = 2,
                        Seen = messageTime
                    });
                }

                // if current user doesn't exist in userAttempts, add it
                if (!V.UserAttempts.ContainsKey(c.Realname))
                    V.UserAttempts.Add(c.Realname, 0);

                // add new channel to the channel list if not contained
                if (!V.UserChannelList.ContainsKey(c.Recipient)
                    && c.Recipient.StartsWith("#"))
                    V.UserChannelList.Add(c.Recipient, new List<string>());

                // set currentUser to the current user
                V.CurrentUser = V.Users.FirstOrDefault(e => e.Realname == c.Realname);

                // Write data to console in a more readable format
                Console.WriteLine($"[{c.Recipient}]({messageTime.ToString("hh:mm:ss")}){c.Nickname}: {c.Args}");

                // queue OnChannelMessage into threadpool
                ThreadPool.QueueUserWorkItem(e => OnChannelMessage(c));
            }
            else if (pingRegex.IsMatch(data))
                SendData("PONG", pingRegex.Match(data).Groups["Message"].Value);
        }

        /// <summary>
        ///     Loads all Type assemblies in ./modules/ into memory
        /// </summary>
        /// <returns>void</returns>
        public static Dictionary<string, Type> LoadModules() {
            var modules = new Dictionary<string, Type>();

            if (!Directory.Exists("modules")) {
                Console.WriteLine("||| Modules directory not found. Creating directory.");
                Directory.CreateDirectory("modules");
            }

            try {
                foreach (KeyValuePair<String, Type> kvp in Assembly.LoadFrom(Path.GetFullPath("modules/Eve.Core.dll")).GetTypes()
                    .Select(t => TypeCheckAndDo(t, modules)).Where(kvp => !kvp.Equals(default(KeyValuePair<string, Type>))))
                    modules.Add(kvp.Key, kvp.Value);

                foreach (KeyValuePair<String, Type> kvp in from f in Directory.EnumerateFiles("modules", "Eve.*.dll")
                    let r = new RecursiveAssemblyLoader()
                    select r.GetAssembly(Path.GetFullPath(f))
                    into file
                    from t in file.GetTypes()
                    select TypeCheckAndDo(t, modules)
                    into kvp
                    where !kvp.Equals(default(KeyValuePair<string, Type>))
                    select kvp)
                    modules.Add(kvp.Key, kvp.Value);
            }
            catch (InvalidCastException e) {
                Console.WriteLine(e.ToString());
            }
            catch (NullReferenceException e) {
                Console.WriteLine(e.ToString());
            }
            catch (ReflectionTypeLoadException ex) {
                StringBuilder sb = new StringBuilder();
                foreach (Exception exSub in ex.LoaderExceptions) {
                    sb.AppendLine(exSub.Message);
                    FileNotFoundException exFileNotFound = exSub as FileNotFoundException;
                    if (!string.IsNullOrEmpty(exFileNotFound?.FusionLog)) {
                        sb.AppendLine("Fusion Log:");
                        sb.AppendLine(exFileNotFound.FusionLog);
                    }
                    sb.AppendLine();
                }
                string errorMessage = sb.ToString();
                Console.WriteLine(errorMessage);
            }

            Console.WriteLine($"||| Loaded modules: {String.Join(", ", modules.Keys)}");
            return modules;
        }

        /// <summary>
        ///     Handles interface checks on the Types and adds them to the module list.
        ///     Commands are also added to list.
        /// </summary>
        /// <param name="type">Type to be checked against IModule interface</param>
        /// <param name="checker">Dictionary to be checked against</param>
        private static KeyValuePair<string, Type> TypeCheckAndDo(Type type, Dictionary<string, Type> checker) {
            if (type.GetInterface("IModule") == null) return new KeyValuePair<string, Type>();

            if (!type.GetInterface("IModule").IsEquivalentTo(typeof(IModule)))
                return new KeyValuePair<string, Type>();

            if (checker.ContainsValue(type)) return new KeyValuePair<string, Type>();
            // instance the current type and set it's def clause equal to def
            Dictionary<String, String> def = ((IModule) Activator.CreateInstance(type)).Def;

            if (def == null) return new KeyValuePair<string, Type>(type.Name.ToLower(), type);

            foreach (KeyValuePair<String, String> s in def.Where(s => !V.Commands.Contains(s)))
                V.Commands.Add(s.Key, s.Value);

            return new KeyValuePair<string, Type>(type.Name.ToLower(), type);
        }
    }

Eve class, the main space for the program

internal class Eve {
    public static bool ShouldRun { get; set; } = true;

    private static void Main() {
        Config conf = new Config {
            Name = "SemiViral",
            Nick = "Eve",
            Port = 6667,
            Server = "irc6.foonetic.net",
            Channels = new[] {"#testgrounds2" },//, "#ministryofsillywalks" },
            Joined = false,
            Identified = false
        };

        using (IrcBot bot = new IrcBot(conf)) {
            bot.InitializeConnections();

            while (ShouldRun)
                bot.Runtime();
        }

        Console.WriteLine("||| Bot has shutdown.");
        Console.ReadLine();
    }
}

Assembly loader class

public class RecursiveAssemblyLoader : MarshalByRefObject {
    public Assembly GetAssembly(string path) {
        return Assembly.LoadFrom(path);
    }
}

CaseEquals extension method

public static class Extentions {
    /// <summary>
    ///     Compares the object to a string with default ignorance of casing
    /// </summary>
    /// <param name="query">string to compare</param>
    /// <param name="ignoreCase">whether or not to ignore case</param>
    /// <returns>true: strings equal; false: strings unequal</returns>
    public static bool CaseEquals(this string obj, string query, bool ignoreCase = true) {
        return obj.Equals(query, ignoreCase ? StringComparison.InvariantCultureIgnoreCase : StringComparison.InvariantCulture);
    }
}

IModule.cs, the interface

public interface IModule {
    Dictionary<string, string> Def { get; }
    ChannelMessage OnChannelMessage(ChannelMessage c);
}

public class ChannelMessage {
    public List<string> _Args = new List<string>();
    public DateTime Time;

    public int ExitType = Int32.MaxValue;
    public string
        Nickname,
        Realname,
        Hostname,
        Type,
        Recipient,
        Args;

    public List<string> MultiMessage = new List<String>(); 
    public string
        Target,
        Message;

    /// <summary>
    /// Resets this ChannelMessage's output variables
    /// </summary>
    /// <param name="newTarget">new string for this.Target to equate to</param>
    public void Reset(string newTarget = null) {
        MultiMessage = new List<string>();
        ExitType = Int32.MaxValue;
        Target = Message = String.Empty;

        Target = newTarget ?? newTarget;

    }
}

Core.cs, home of core module types

(This is a snippet, as the full file is about 500 lines)

public class Core : Utils, IModule {
        public Dictionary<string, string> Def => null;

        public ChannelMessage OnChannelMessage(ChannelMessage c) {
            switch (c.Type) {
                case "NICK":
                    c.ExitType = 0;

                    using (
                        SQLiteCommand com =
                            new SQLiteCommand($"UPDATE users SET nickname='{c.Recipient}' WHERE realname='{c.Realname}'", IrcBot.V.Db))
                        com.ExecuteNonQuery();
                    break;
                case "JOIN":
                    c.ExitType = 0;
                    if (c.Realname == "Eve") return c;

                    if (IrcBot.V.QueryName(c.Realname) != null
                        && IrcBot.V.CurrentUser.Messages != null) {
                        c.Target = c.Nickname;

                        foreach (Eve.Message m in IrcBot.V.CurrentUser.Messages)
                            c.MultiMessage.Add($"({m.Date}) {m.Sender}: {Regex.Unescape(m.Contents)}");

                        IrcBot.V.Users.First(e => e.Realname == c.Realname).Messages = null;

                        using (SQLiteCommand x = new SQLiteCommand($"DELETE FROM messages WHERE id={IrcBot.V.CurrentUser.Id}", IrcBot.V.Db))
                            x.ExecuteNonQuery();
                    }

                    if (!IrcBot.V.UserChannelList.ContainsKey(c.Recipient))
                        IrcBot.V.UserChannelList.Add(c.Recipient, new List<string>());

                    IrcBot.V.UserChannelList[c.Recipient].Add(c.Realname);
                    break;
                case "PART":
                    c.ExitType = 0;
                    IrcBot.V.UserChannelList[c.Recipient].Remove(c.Realname);
                    break;
                case "353":
                    c.ExitType = 0;
                    if (!IrcBot.V.UserChannelList.ContainsKey(c.Recipient))
                        IrcBot.V.UserChannelList.Add(c.Recipient, new List<string>());

                    // splits the channel user list in half by the :, then splits each user into an array object to iterated
                    foreach (string s in c.Args.Split(':')[1].Split(' '))
                        IrcBot.V.UserChannelList[c.Recipient].Add(s);
                    break;
                default:
                    if (!c._Args[0].Replace(",", String.Empty).CaseEquals("eve")
                        || IrcBot.V.IgnoreList.Contains(c.Realname)
                        || GetUserTimeout(c.Realname)) {
                        c.ExitType = 0;
                        return c;
                    }

                    if (c._Args.Count < 2) {
                        c.ExitType = 1;
                        c.Message = "Please provide a command. Type 'eve help' to view my command list.";
                    }
                    else if (!IrcBot.V.Commands.ContainsKey(c._Args[1].ToLower())) {
                        c.ExitType = 1;
                        c.Message = "Invalid command. Type 'eve help' to view my command list.";
                    }

                    break;
            }

            return c;
        }
    }

    public class Join : IModule {
        public Dictionary<string, string> Def => new Dictionary<string, string> {
            {"join", "(<channel>) — joins specified channel."}
        };

        public ChannelMessage OnChannelMessage(ChannelMessage c) {
            if (!c._Args[1].CaseEquals("join"))
                return null;

            if (IrcBot.V.CurrentUser.Access > 1)
                c.Message = "Insufficient permissions.";
            else if (String.IsNullOrEmpty(c._Args[2]))
                c.Message = "Insufficient parameters. Type 'eve help join' to view ccommand's help index.";
            else if (!c._Args[2].StartsWith("#"))
                c.Message = "Channel c._Argsument must be a proper channel name (i.e. starts with '#').";
            else if (IrcBot.V.Channels.Contains(c._Args[2].ToLower()))
                c.Message = "I'm already in that channel.";

            if (!String.IsNullOrEmpty(c.Message)) {
                c.Type = "PRIVMSG";
                return c;
            }

            IrcBot.V.Channels.Add(c._Args[2].ToLower());

            c.Message = c._Args[2];
            c.Type = "JOIN";
            return c;
        }
    }

    public class Part : IModule {
        public Dictionary<string, string> Def => new Dictionary<string, string> {
            ["part"] = "(<channel> *<message>) — parts from specified channel."
        };

        public ChannelMessage OnChannelMessage(ChannelMessage c) {
            if (!c._Args[1].CaseEquals("part"))
                return null;

            if (IrcBot.V.CurrentUser.Access > 1)
                c.Message = "Insufficient permissions.";
            else if (String.IsNullOrEmpty(c._Args[2]))
                c.Message = "Insufficient parameters. Type 'eve help part' to view ccommand's help index.";
            else if (!c._Args[2].StartsWith("#"))
                c.Message = "Channel c._Argsument must be a proper channel name (i.e. starts with '#').";
            else if (!IrcBot.V.Channels.Contains(c._Args[2].ToLower()))
                c.Message = "I'm not in that channel.";

            if (!String.IsNullOrEmpty(c.Message)) {
                c.Type = "PRIVMSG";
                return c;
            }

            c.Message = c._Args.Count > 3 ? $"{c._Args[2]} {c._Args[3]}" : c._Args[2];

            IrcBot.V.Channels.Remove(c._Args[2]);
            IrcBot.V.UserChannelList.Remove(c._Args[2]);

            c.Type = "PART";
            return c;
        }
    }

Utilities.cs

public class Utils {
    public static string HttpGet(string url) {
        HttpWebRequest request = (HttpWebRequest) WebRequest.Create(url);
        request.Method = "GET";

        using (HttpWebResponse httpr = (HttpWebResponse) request.GetResponse())
            return new StreamReader(httpr.GetResponseStream()).ReadToEnd();
    }

    public static IEnumerable<string> SplitStr(string str, int maxLength) {
        for (var i = 0; i < str.Length; i += maxLength)
            yield return str.Substring(i, Math.Min(maxLength, str.Length - i));
    }

    public static bool GetUserTimeout(string who) {
        Variables v = IrcBot.V;
        var doTimeout = false;

        if (v.QueryName(who) == null)
            return false;

        if (v.UserAttempts[who] == 4)
            // Check if user's last message happened more than 1 minute ago
            if (v.QueryName(who).Seen.AddMinutes(1) < DateTime.UtcNow)
                v.UserAttempts[who] = 0; // if so, reset their attempts to 0
            else doTimeout = true; // if not, timeout is true
        else if (v.QueryName(who).Access > 1)
            // if user isn't admin/op, increment their attempts
            v.UserAttempts[who]++;

        IrcBot.V = v;
        return doTimeout;
    }
}

Concerns are:

  • Code readability
  • Consistency and efficiency in commenting
  • Upwards scalability. As far as I can tell this shouldn't be an issue, but the program speed could be very impacted by how cluttered the main body class is
  • General code-efficiency
  • Typos

I do appreciate the use of an already-existing source, so those are of course welcomed also.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Variables class

Usually boolean flags does not help readability and, in this case, a boolean flag to perform an active action in object constructor (which should, in theory, just...construct the object) is even more astonishing. Fortunately you don't even call constructor with false (which BTW may be useless) then you can simply remove it (otherwise you should introduce another public method to call).

sealed class Variables {
    public Variables() {
        CreateDbIfNotExistAndReadData();
    }

    private void CreateDbIfNotExistAndReadData() {
        // Things happen here
    }
}

Note I also marked the class with sealed, if you do not inherit then you should make it explicit (it may enable some optimizations but more than that it's a matter of intent).

You check for if (!File.Exists("users.sqlite")) hardcoding database file name and without specifying any path. For sure file name must not stay in code like this but in a constant (or in configuration), it's even used more than once. Moreover unless you really want to use current directory then you should explicitly say where you expect file to be. This also applies for all your other strings (also and especially for your connection string).

You will then have:

private void CreateDbIfNotExistAndReadData() {
    if (!File.Exists(ResolveDatabaseFilePath()))
        CreateDatabase();

    ReadDatabaseData();
}

Note that this method may be (IMO) even directly merge in constructor (unless you have to make it public).

In my opinion your exception handling is little bit weird. First of all you often catch NullReferenceException. NullReferenceException should never happen, if an object may be null then you have to check before you use it. Second problem is that you catch exceptions just for logging purposes also leaving program running, for example when DB connection fails you still try to read data. If you can't do anything with an exception than don't catch it.

You may consider to implement IDisposable pattern in a more traditional way with Dispose(bool) method and a destructor.

Code to create database may be hard to maintain if things become more complicate. First of all split that code in several methods: ReadUsers(), ReadMessagesForUser(), CreateDefaultUser() and so on. Each method should be short (ideally less than, let's say, 20 lines of code) and easy to understand (following called methods only when you need more details).

After this you will have a lot of SQL code inside your C# code, that's hard to read and it's easy to write incorrect SQL code (because strings aren't validated at compile time). If possible I'd use an ORM or something to abstract this away. Did you consider to use, for example, Dapper? See also what suggested in this post.

All your fields are public. They must be private and you must expose specific methods for each operation you want to allow. If you don't then your classes will be too coupled and a small change in internal implementation will force changes in all your code (what if you want to use an hashset instead of a list?)

Dictionary<string, List<string>> is sincerely near my personal maximum type name complexity. You may consider to create a class for this (also abstracting all the complexity of its Add() method).

IrcBot

Many things said for Variables also apply to IrcBot. Exception management, method complexity and hard-coded string constants and IDisposable implementation. For example let's pick OnChannelMessage method where you're using a boolean flag stoploop. You felt you need to add comments because cm.ExitType tells nothing about its content. An enum and a better name may help a lot (even a boolean flag may be better than an integer). Ideally you should have something like (better names still required):

if (message.ExitType == LoopExitType.SendMessageAndExit)
   ...
else if (message.ExitType == LoopExitType.SendMessageAndExit)
   ...

Without switch/case you may also skip that boolean flag. Even better if you move that code into a separate function. More than that: what's the meaning of ExitType inside a ChannelMessage class?

Runtime() method is waaaaaaaaaays too complex. Split split split. Move constants outside, ideally you will see you won't need comments because code MUST be self-explicative. Parse message, build object, do things, insert into database, log data. All of these in one method.

This is a God class, split code into several classes.

Eve

You may not need a while loop in main thread. Create and call your bot in a secondary background thread (where you can loop parsing input) and simply add a Console.ReadLine() in your Main() method.

CaseEquals

It's useless, IMO. There already is a String.Equals() method and there is a reason for that...it's clear at calling point what you're comparing (booleans are terrible for this). Note that if things are well-done then you will have just one or two calling points.

Core.cs

I can't comment on invisible code but I'd guess it's another God class. Split things! Do you need a command line parser? Create a class. Do you need an user input parser? Create a class for it. Also I didn't see any MessageParser.

Answers

To aswer your direct questions:

The code readability

Low, in my opinion. Methods are too long and classes do too many things. Imagine to put this code in a box and take it out after six months. You will need to re-read almost everything to understand its logic.

Upwards scalability.

Sub-optimal. To handle a new command you will need to touch multiple (already convoluted) methods. God classes are hard to fix, understand and to test. The fact you can talk about a main body class should be a big red flag for you.

General code-efficiency

I do not think you will ever have any performance issue for this application (unless you're running an IRC server). Yes things may (should) be improved (lazy loading from DB only of required data, background loading/processing and blocking network I/O) but you may not have this needing soon.

Let me stress again about two important topics: readability and tests. Your code now is hard to test. You can't test your IRC protocol parser separated from your logic separated from your network connection handling. For obvious reasons it's bad. Single-responsibility classes will be easier to test because you can mock world around them. Moreover you can completely rewrite your logic and you won't need to also rewrite your parser (and vice-versa).

Commenting

After refactoring you will probably see you will need less comments (BTW in your actual code I think you should add much more comments to explain what's going on and why, also referring to IRC standard). For example:

// Check if user's last message happened more than 1 minute ago
if (v.QueryName(who).Seen.AddMinutes(1) < DateTime.UtcNow)

Your comment just repeat what you can already read in code. It adds nothing but confusion (last seen one minute ago or last message sent one minute ago?) You should explain WHY you wait one minute and what it is for. Also (about GetUserTimeout()) you should consider to rename this method. GetTimeout() that returns bool is pretty weird.

Instead, for example, I can't find anything (especially a reference to IRC protocol) to explain what messageRegex regex is supposed to parse.

One very small note: I wouldn't put more than one class in each file (naming file with class name). It's easier to find code when you need it.

\$\endgroup\$
10
  • \$\begingroup\$ Thank you for the input. The reason CaseEquals exists is because this is very long and is a pain in the ass to type:` if (c._Args[1].Equals("join", StringComparison.InvariantCultureIgnoreCase). Also, if I make Variables sealed` then it's data is inaccessible; is this what should be happening, and I should just change the way the commands handle changing of the values in the Variables class? \$\endgroup\$ Jan 12, 2016 at 18:10
  • 1
    \$\begingroup\$ I understand but it's a pain for a reason. I'd ban == for string comparison, more often than not we must exactly think about what we are doing with strings. Sealed stops inheritance. To make data inaccessible you gave to make them private (at least setters) ideally exposing methods (or, if properties, at least hiding implementation details). \$\endgroup\$ Jan 12, 2016 at 18:14
  • \$\begingroup\$ Curious why Adriano would want to make the class sealed...in my experience that's usually very short-sighted and the cause of much cursing by future developers who are now unable to extend your code. \$\endgroup\$ Jan 12, 2016 at 18:29
  • \$\begingroup\$ Well, first because it has not (currently) any virtual method then there is nothing to extend. Secondly because it's a private class, he is not developing a library (in that case I'd suggest a completely different approach with virtual methods and base classes). If in future he will need anything different then he will be free to change it. No unneeded complexity now. I like to write code when needed, not before (when I do not have to deal with compatibility). \$\endgroup\$ Jan 12, 2016 at 19:15
  • \$\begingroup\$ I'm on board with not over-engineering, but specifically making it sealed? Why would you default to something so restricting? You can't extend, inherit, or mock it if you're going to do unit testing... \$\endgroup\$ Jan 12, 2016 at 20:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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