Semi-primitive IRC bot serving as learning project

I'd say currently my understanding of C# is within the range of (1Beginner-10Intermediate) a solid 4-5. Thus I am seeking to broaden my understanding of C#; I've started coding an IRC bot to improve. This is the code:

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Text.RegularExpressions;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

namespace IRCBot
{

User and Message classes for JSON deserialization

internal class User
    {
        public User()
        {
            Messages = new List();
        }

        public string Name { get; set; }
        public int Access { get; set; }
        public DateTime Seen { get; set; }
        public List Messages { get; set; }
    }

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

Config struct

internal struct Config
    {
        public bool Joined;
        public string Server;
        public string[] Channels;
        public string Nick;
        public string Name;
        public int Port;
    }

IRCBot class, where everything happens

```
internal class IRCBot : IDisposable, IModule {
private readonly Dictionary> users = new Dictionary>();
private readonly Dictionary userAttempts = new Dictionary();

private readonly Dictionary commands = new Dictionary
{
{"join", "(join ) - joins specified channel."},
{"part", "(part ) - parts specified channel."},
{"leave", "(part ) - parts specified channel."},
{"say", "(say ) - sends privmsg to target channel."},
{"chanlist", "Provides a list of channels joined."},
{"channels", "Provides a list of channels joined."},
{"userlist", "List all users currently in database and their access number."},

One of the key points in having readable and maintainable code is to have consistence of the choosen style which isn't the case in your code.

• You have different indention styles for methods. Sometimes you are using K&R and sometimes you are using Allman style.

• sometimes you are using underscore prefixed method variables (which isn't the preffered way) and sometimes you use camelCase casing to name your method variables.

• sometimes you are using braces {} for single instruction else or for each and sometimes you don't use them.

While we are talking about braces and the use of them although they might be optional, I would like to encourage you to always use them to make your code less error prone.

Naming is another important task for having readable and understandable code. This goal will be defeated if you use abbreviation for naming variables like

private StreamWriter _log;
private NetworkStream _ns;
private StreamReader _sr;
private StreamWriter _sw;

Seeing this variables in the code will not tell the reader of the code what they are about. Using meaningful names allows Sam the maintainer to grasp the purpose of these at first glance.

The OnChannelMessage() method is doing too many things. It is querying and adding to multiple lists, writing to file and parsing the message. You should extract parts of this method to separate methods.

For instance this

if (QueryName(c.Realname) == null
    && !nameDenyList.Contains(c.Realname)) {
    userList.Add(new User {
        Name = c.Realname,
        Access = 2,
        Seen = DateTime.UtcNow
    });
} else if (QueryName(c.Realname) != null)
    userList.First(e => e.Name == c.Realname).Seen = DateTime.UtcNow;

if (!userAttempts.ContainsKey(c.Realname))
    userAttempts.Add(c.Realname, 0);
if (!users.ContainsKey(c.Recipient)
    && c.Recipient.StartsWith("#"))
    users.Add(c.Recipient, new List());

should be in a separate method, because it doesn't has anything to do with the parsing of the message.

This magic string

case "353":

doesn't tell what it is. Why is this not "777" ? Having magic numbers and strings in your code make it harder to understand the code. You should extract such strings (and numbers) to meaningful constants.

Based on the NET naming guidelines methods should be made out of verbs or verb phrases. You have a lot of methods which aren't following this guidelines like UserList, Message, Seen, UserTimeout and many more.

Following guidelines makes it easier for future readers of that code to understand it faster.

Instead of string concatenation like

SendMessage(u.Name + " was last seen on: " + u.Seen + " (UTC)");

you should either use string.Format() like so

SendMessage(string.Format("{0} was last seen on: {1} (UTC)", u.Name, u.Seen));

or if you are using C# 6 (VS 2015) you can use string interpolation by using the $ operator like so

SendMessage($"{u.Name} was last seen on: {u.Seen} (UTC)");

Because regex expressions mostly aren't understandable at first glance it is important to name them good.

var mRegex = new Regex(@"^:(?[^\s]+)\s(?[^\s]+)\s(?[^\s]+)\s?:?(?.*)", RegexOptions.Compiled);
var sRegex = new Regex(@"^(?[^\s]+)!(?[^\s]+)@(?[^\s]+)", RegexOptions.Compiled);
var aRegex = new Regex(@"^:(?[^\s]+)\s(?[^\s]+)\s(?[^\s]+)\s?:?(?.*)", RegexOptions.Compiled);
var pRegex = new Regex(@"^PING :(?.+)", RegexOptions.None);

so this isn't helping.

Declaring multiple variables on the "same line" like in the ChannelMessage class doesn't buy you anything but costs readability.

internal class ChannelMessage
{
    public DateTime Time;

    public string
        Nickname,
        Realname,
        Hostname,
        Type,
        Recipient,
        Args;
}

Parents should talk to their children by using methods and properties but a child should talk to its parent by using events. So this

case "shutdown":
    if (access > 1)
    {
        SendMessage("Goodybe.");
        Eve.Run = false; // <- this
    }

is not good.

StackExchange Code Review Q#113707, answer score: 11