10
\$\begingroup\$

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);
        }
    }
}
\$\endgroup\$
13
\$\begingroup\$
  • 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();
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Yeach, and extract connection info in separate data structure. Then We should separate responsibility - connection, looping and answer strategy -> move this to separate classes. Ideally would be using(var client= clientFactory.Create(Clients.IRC, clientCredentials)){ await botStrategyFactory.Create(BotStrategy.PingPong, client.StreamToRead, client.StreamToWrite).RunAsync(); } :D \$\endgroup\$ – tym32167 Sep 28 '16 at 10:01
  • \$\begingroup\$ @tym32167 I agree, usually I'd separate everything but this is so tiny that I'd probably wouldn't bother. \$\endgroup\$ – t3chb0t Sep 28 '16 at 10:35
  • 2
    \$\begingroup\$ oh, that was just joke, I agree with KISS (Keep it simple and stupid), we don want to overcomplicate here, you answer is pretty enough. \$\endgroup\$ – tym32167 Sep 28 '16 at 10:42
12
\$\begingroup\$

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...

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @tym32167 is this what you mean? ;-D Good job Adriano +1 ;-) but for this micro app perhaps a little bit over engineered. \$\endgroup\$ – t3chb0t Sep 28 '16 at 13:38
  • 1
    \$\begingroup\$ @t3chb0t I agree, of course OP code has to be "reduced" to fit question size then I'm always in doubt about how big his application is. For a true IRC bot I'd start immediately with a more complex implementation, overhead is small (IMO) compared to time you will save just few days later ;) \$\endgroup\$ – Adriano Repetti Sep 28 '16 at 14:09
  • 1
    \$\begingroup\$ I have to admit, that this general idea of separating all of the small tasks is what I've personally wanted to do with my project, and I love the way this looks. But wow would I be in over my head with how you were describing all that... I can never find good practical code like this in tutorials or what not. \$\endgroup\$ – Tony Arnold May 5 '17 at 5:34

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.