HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Implementing an IRC Bot

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementingircbot

Problem

This is a (fairly) simple IRC bot. The whole idea of it was similar to Duga, except that it will post to an IRC server instead.

The bot takes messages from a SQL table and posts them in the IRC server.

It supports a few (basic) commands which are sent as private messages to the user who requested the command.

The bot is designed to pull messages from an SQL server, which means that messages are not likely to be lost. I haven't included any DDL for the SQL tables, Stored Procedures, or Views, but they all work exactly as expected.

Repository on GitHub

The Program class:

```
class Program
{
private static BotConfiguration configuration;
private static CommandDispatcher commandDispatcher;
public static IrcClient irc = new IrcClient();

#if DEBUG
private static string configFile = "BotConfiguration.debug.xml";
#else
private static string configFile = "BotConfiguration.release.xml";
#endif

static void Main(string[] args)
{
configuration = BotConfiguration.LoadConfig(configFile);
_Main(args);
Console.ReadLine();
}

public static void OnQueryMessage(object sender, IrcEventArgs e)
{
switch (e.Data.MessageArray[0])
{
case "dump_channel":
string requested_channel = e.Data.MessageArray[1];
Channel channel = irc.GetChannel(requested_channel);

irc.SendMessage(SendType.Message, e.Data.Nick, "");

irc.SendMessage(SendType.Message, e.Data.Nick, "Name: '" + channel.Name + "'");
irc.SendMessage(SendType.Message, e.Data.Nick, "Topic: '" + channel.Topic + "'");
irc.SendMessage(SendType.Message, e.Data.Nick, "Mode: '" + channel.Mode + "'");
irc.SendMessage(SendType.Message, e.Data.Nick, "Key: '" + channel.Key + "'");
irc.SendMessage(SendType.Message, e.Data.Nick, "UserLimit: '" + channel.UserLimit + "'");

string nickname_list = "";

Solution

Lots of code we got. This from bottom to top we review.

VersionMessageHandler

#if DEBUG
    public string ProgramMode = "Debug";
#else
    public string ProgramMode = "Release";
#endif


Why is this field public accessible ? Any caller/user of this class can adjust it to its needs. If this is set to null by its caller, the call to ProcessMethod() will lead to unexpected results.

public VersionMessageHandler()
{ 
    Version = typeof(Program).Assembly.GetName().Version.ToString();
}


this will couple the VersionMessageHandler class tigthly to the Program class. It would be better to pass the version as a constructor argument.

public void ProcessMethod(IrcClient ircClient, BotConfiguration configuration, string message, string user, params string[] args)
{
    if (user != "")
    {
        ircClient.RfcPrivmsg($"{user}", $"Sara Version: {Version} {ProgramMode}");
    }

    if (args.Length > 0 && args[0] == "force")
    {
        ircClient.SendMessage(SendType.Notice, configuration.DefaultChannel, $"Sara Version: {Version} {ProgramMode}");
    }
}


The check for if (user != "") is superflous because you don't check for user != null. Using string interpolation with a argument which is null behaves the same as it wouldn't be present. So $"{user}" would evaluate for user == null or user == "" to just "".

Now you know what I meant by unexpected results for setting ProgramMode = null. The second if would send Sara Version: {Version} where Version would be replaced by the current version, but the ProgramMode would be skipped.

DateMessageHandler

Using expression bodied member for assigning a value to a string like private string _defaultDateTime => "O"; is a little bit overkill. In addition because it won't be changed anywhere in the code, you should just make it private readonly.

public void ProcessMethod(IrcClient ircClient, BotConfiguration configuration, string message, string user, params string[] args)
{
    if (args.Length >= 1)
    {
        if (args[0].ToUpper() == "UTC")
        {
            SendMessage(new DateTimeOffset(DateTime.UtcNow), ircClient, configuration, message, user, args);
        }
        else if (args[0].ToUpper() == "LOCAL")
        {
            SendMessage(new DateTimeOffset(DateTime.Now), ircClient, configuration, message, user, args);
        }
    }
    else
    {
        SendMessage(new DateTimeOffset(DateTime.UtcNow), ircClient, configuration, message, user, args);
    }
}


here you have small code duplication wich can be removed by changing the method like so

public void ProcessMethod(IrcClient ircClient, BotConfiguration configuration, string message, string user, params string[] args)
{
    if (args.Length == 0 || args[0].ToUpper() == "UTC")
    {
        SendMessage(new DateTimeOffset(DateTime.UtcNow), ircClient, configuration, message, user, args);
    }
    else if (args[0].ToUpper() == "LOCAL")
    {
        SendMessage(new DateTimeOffset(DateTime.Now), ircClient, configuration, message, user, args);
    }
}


For using string.ToUpper() for comparising please read here: using-invariantcultureignorecase-instead-of-toupper-for-case-insensitive-string

Quoting the qoute:


As discussed by lots and lots of people, the "I" in Turkish behaves differently than in most languages. Per the Unicode standard, our lowercase "i" becomes "İ" (U+0130 "Latin Capital Letter I With Dot Above") when it moves to uppercase. Similarly, our uppercase "I" becomes "ı" (U+0131 "Latin Small Letter Dotless I") when it moves to lowercase.


Fix: Again, use an ordinal (raw byte) comparer, or invariant culture for comparisons unless you absolutely need culturally based linguistic comparisons (which give you uppercase I's with dots in Turkey)

private string GetFormat(params string[] args)
{
    string format = "";

    for (int i = 1; i  0)
        {
            format += " ";
        }

        format += args[i];
    }

    return format;
}


this method can be reduced to a one liner and therfor could just be removed completely.

Just replace the call of the method like so

string format = string.Join(" ", args);


CountMessageHandler

This if construct

```
if (args.Length >= 1 && args[0].ToUpper() == "IRC")
{
if (args.Length >= 2 && args[1].ToUpper() == "TOTAL")
{
using (SqlCommand command = new SqlCommand(configuration.SqlScripts["Operations\\GetAllIrcMessagesCount"], connection))
{
//ircClient.SendMessage(SendType.Notice, configuration.DefaultChannel, $"{messagePrefix}Total messages: {command.ExecuteScalar()}");
ircClient.RfcPrivmsg($"{user}", $"Total messages: {command.ExecuteScalar()}");
}
}
else if (args.Length >= 2 && args[1].ToUpper() == "TODAY")
{
using (SqlCommand command = new SqlCommand(configuration.SqlSc

Code Snippets

#if DEBUG
    public string ProgramMode = "Debug";
#else
    public string ProgramMode = "Release";
#endif
public VersionMessageHandler()
{ 
    Version = typeof(Program).Assembly.GetName().Version.ToString();
}
public void ProcessMethod(IrcClient ircClient, BotConfiguration configuration, string message, string user, params string[] args)
{
    if (user != "")
    {
        ircClient.RfcPrivmsg($"{user}", $"Sara Version: {Version} {ProgramMode}");
    }

    if (args.Length > 0 && args[0] == "force")
    {
        ircClient.SendMessage(SendType.Notice, configuration.DefaultChannel, $"Sara Version: {Version} {ProgramMode}");
    }
}
public void ProcessMethod(IrcClient ircClient, BotConfiguration configuration, string message, string user, params string[] args)
{
    if (args.Length >= 1)
    {
        if (args[0].ToUpper() == "UTC")
        {
            SendMessage(new DateTimeOffset(DateTime.UtcNow), ircClient, configuration, message, user, args);
        }
        else if (args[0].ToUpper() == "LOCAL")
        {
            SendMessage(new DateTimeOffset(DateTime.Now), ircClient, configuration, message, user, args);
        }
    }
    else
    {
        SendMessage(new DateTimeOffset(DateTime.UtcNow), ircClient, configuration, message, user, args);
    }
}
public void ProcessMethod(IrcClient ircClient, BotConfiguration configuration, string message, string user, params string[] args)
{
    if (args.Length == 0 || args[0].ToUpper() == "UTC")
    {
        SendMessage(new DateTimeOffset(DateTime.UtcNow), ircClient, configuration, message, user, args);
    }
    else if (args[0].ToUpper() == "LOCAL")
    {
        SendMessage(new DateTimeOffset(DateTime.Now), ircClient, configuration, message, user, args);
    }
}

Context

StackExchange Code Review Q#105400, answer score: 6

Revisions (0)

No revisions yet.