Simple IRC Bot in C#

Question

This was a simple IRC bot I threw together a long time ago, found recently, and was curious as to if there were any kind of significant improvements that could be made.

using System;
using System.Net;
using System.Net.Sockets;
using System.IO;
using System.Threading;

public class IRCbot
{
    // server to connect to (edit at will)
    public static string SERVER = "irc.changeme.com";
    // server port (6667 by default)
    private static int PORT = 6667;
    // user information defined in RFC 2812 (IRC: Client Protocol) is sent to the IRC server 
    private static string USER = "USER IRCbot 0 * :IRCbot";
    // the bot's nickname
    private static string NICK = "IRCbot";
    // channel to join
    private static string CHANNEL = "#opers";

    static void Main(string[] args)
    {
        NetworkStream stream;
        TcpClient irc;
        string inputLine;
        StreamReader reader;
        StreamWriter writer;

        try
        {
            irc = new TcpClient(SERVER, PORT);
            stream = irc.GetStream();
            reader = new StreamReader(stream);
            writer = new StreamWriter(stream);
            writer.WriteLine("NICK " + NICK);
            writer.Flush();
            writer.WriteLine(USER);
            writer.Flush();

            while (true)
            {
                while ((inputLine = reader.ReadLine()) != null)
                {
                    Console.WriteLine("<- " + inputLine);

                    // split the lines sent from the server by spaces (seems to be the easiest way to parse them)
                    string[] splitInput = inputLine.Split(new Char[] {
                        ' '
                    });

                    if (splitInput[0] == "PING")
                    {
                        string PongReply = splitInput[1];
                        //Console.WriteLine("->PONG " + PongReply);
                        writer.WriteLine("PONG " + PongReply);
                        writer.Flush();
                        //continue;
                    }

                    switch (splitInput[1])
                    {
                        case "001":
                            writer.WriteLine("JOIN " + CHANNEL);
                            writer.Flush();
                            break;
                        default:
                            break;
                    }
                }

                // close all streams (to preserve memory)
                writer.Close();
                reader.Close();
                irc.Close();
            }
        }
        catch (Exception e)
        {
            // shows the exception, sleeps for a little while and then tries to establish a new connection to the IRC server
            Console.WriteLine(e.ToString());
            Thread.Sleep(5000);
            string[] argv = {};
            Main(argv);
        }
    }
}

Answer 1

• you should separate the IRCBot from the Main into its own class with it's own fields/properties
• you should specify all required parameters via the construtor so that it is not possible to create an invalid ircbot
• I don't think it's a good idea to make the retry recursive - IMHO it would be better to create a loop and limit the retries so that you don't have an infite loop (ok, unless it was desired)
• you really should use the using statement. In your current implementation the resources will be closed only if there was no execption - if an exception occurs the Closees won't be called. You also don't free the resources because you don't call the Dispose, you just close them and then create new resources on retry, this is a memory leak.

Here's an example:

public class IRCbot
{
    // server to connect to (edit at will)
    private readonly string _server;
    // server port (6667 by default)
    private readonly int _port;
    // user information defined in RFC 2812 (IRC: Client Protocol) is sent to the IRC server 
    private readonly string _user;

    // the bot's nickname
    private readonly string _nick;
    // channel to join
    private readonly string _channel;

    private readonly int _maxRetries;

    public IRCbot(string server, int port, string user, string nick, string channel, int maxRetries = 3)
    {
        _server = server;
        _port = port;
        _user = user;
        _nick = nick;
        _channel = channel;
        _maxRetries = maxRetries;
    }

    public void Start()
    {
        var retry = false;
        var retryCount = 0;
        do
        {
            try
            {
                using (var irc = new TcpClient(_server, _port))
                using (var stream = irc.GetStream())
                using (var reader = new StreamReader(stream))
                using (var writer = new StreamWriter(stream))
                {
                    writer.WriteLine("NICK " + _nick);
                    writer.Flush();
                    writer.WriteLine(_user);
                    writer.Flush();

                    while (true)
                    {
                        string inputLine;
                        while ((inputLine = reader.ReadLine()) != null)
                        {
                            Console.WriteLine("<- " + inputLine);

                            // split the lines sent from the server by spaces (seems to be the easiest way to parse them)
                            string[] splitInput = inputLine.Split(new Char[] { ' ' });

                            if (splitInput[0] == "PING")
                            {
                                string PongReply = splitInput[1];
                                //Console.WriteLine("->PONG " + PongReply);
                                writer.WriteLine("PONG " + PongReply);
                                writer.Flush();
                                //continue;
                            }

                            switch (splitInput[1])
                            {
                                case "001":
                                    writer.WriteLine("JOIN " + _channel);
                                    writer.Flush();
                                    break;
                                default:
                                    break;
                            }
                        }
                    }
                }
            }
            catch (Exception e)
            {
                // shows the exception, sleeps for a little while and then tries to establish a new connection to the IRC server
                Console.WriteLine(e.ToString());
                Thread.Sleep(5000);
                retry = ++retryCount <= _maxRetries;
            }
        } while (retry);
    }
}

Usage:

void Main()
{
    var ircBot = new IRCbot(
        server: "irc.changeme.com",
        port: 6667,
        user: "USER IRCbot 0 * :IRCbot",
        nick: "IRCbot",
        channel: "#opers"
    );

    ircBot.Start();
}

Answer 2

In addition to t3schb0t's answer I'd also change your parsing logic. Your code is just few lines and it's already too complicate to read. After you moved code into a separate class then it's time for refactoring!

Abstract away read/write/log logic. You already have StreamReader, StreamWriter, Console and TcpClient. Current implementation is hard to test (because you need a TCP server and you must read data to/from streams.) More than that: it's almost impossible to test in isolation...let's introduce few more classes.

Abstract IrcServerConnection to handle connection (with its implementation TcpIrcServerConnection and probably a mock to test all the other logic without a true remote IRC server).

Now you have to abstract input stream, let's introduce a IrcDataPipe (here I'd avoid to call it channel because it may cause confusion with IRC channel but a better name is required...) It may expose ReadLines() (which returns IEnumerable<string>, an infinite loop until connection is closed may be appropriate), WriteLine() for generic write operations and Send() to explicitly write a command (see later.) If required you may want to introduce two views of this pipe accessible through two properties like Request and Response (but I'd not go too far at the beginning.)

Also note that NetworkStream is not buffered then you do not need to call Flush() after each write.

You may want to introduce a Log class with its LogWriter (which may be console by default). In this way you may also differentiate between errors and informative messages. Console output may be, for example, colored and you may want a different format for file log (note that command line redirection is always possible.)

inputLine.Split(new Char[] {' '}); may be simplified to inputLine.Split(' ');

Be consistent with variable naming, PongReply should be pongReply.

I'd suggest to remove all those magic constants. If you do not want to introduce an IrcCommand class you may at least use constants. Here is how I'd like to see it (at first step):

pipe.Send(new PongCommand(splitLine[1]));

I'd simplify parsing. For commands for which you have a mandatory response you may use a dictionary (or Reflection...) For example a generic IrcCommand derived class may be something like this:

[InResponseTo(IrcServerMessages.Ping)]
sealed class PongCommand : IrcCommand {
    public override string GetResponse(IrcServerMessage message) {
        return message.Parameters.Single();
    }
}

As you can see I introduced an IrcServerMessage class which both hide parsing and encapsulate parameters. Your processing loop will be as simple as:

using (var connection = CreateConnection().Open())
using (var pipe = connection.OpenPipe())
{
    LogIn(pipe);

    foreach (var message in pipe.ReadLines().Select(x => new IrcServerMessage(x))
        ProcessKnownMessages(message);
}

Where LogIn(pipe) is something like this:

private static void LogIn(IrcPipe pipe) {
    pipe.Send(
        new NickCommand(_nick),
        new UserCommand(_nick)
    );
}

Note multiple commands on the same line (Send(params IrcCommand[] commands)) to denote a batch which I want to fail/succeed all together.

Now let's move connection into a separate method to use your class like this:

using (var connection = new IrcConnection(CreateConnection))
using (var bot = new IrcBot(connection) {
    connection.Open();
    bot.StartChatting();
}

You may automatically start bot when connection is open (just add a ConnectionStateChanged event in your IrcConnection class and automatically call LogIn() there) Now your chat loop will simply be:

using (var pipe = Connection.OpenPipe()) {
    foreach (var message in pipe.ReadLines().Select(x => new IrcServerMessage(x))
        ProcessKnownMessages(message);
}

Now that your bot has state you may want to encapsulate those status into an IrcClient class (to handle basic log-in, keep track of open channels and so on.) In this way you will even be able to operate on multiple channels (or with multiple bots). Again a proof of concept:

using (var connection = new IrcConnection(CreateConnection))
using (var client = new IrcClient(connection) {
using (var bot = new IrcBot(client) {
    client.Open(); // Client will open connection if required
    bot.StartChatting();
}

As you can see if you name things your code will be more clear, easy to extend and feature-rich...

Also note that you should never catch Exception if you want to perform any recovery logic. Will you attempt a new server connection if you get OutOfMemoryException? What if you have AccessViolationException? Note that for logging purposes you may also want to flatten aggregate exceptions (and some others with an InnerExceptions - note it's plural - property.)

To retry at this outer level may be useful but you should carefully choose which exceptions you may ignore. For example:

try {
}
catch (Exception e) when (IsNotCriticalException(e)) {
}

Also note that in some circumstances you may want to ignore some exceptions (let's also introduce parsing exceptions like ServerMessageFormatException) but you have to know them otherwise you will not have a reliable software but a software that reliably repeatedly fails. Having specific exceptions will also help you to test failure scenarios: if your test will pass with [ExpectedException(typeof(Exception))] you will never be sure if it passed because you correctly handled that scenario or because your code is faulty and it generated NullReferenceException...