6
\$\begingroup\$

I have been learning C# for a couple years now, and using small projects to help myself learn. My most recent project is a complete rewrite of my first C# project that just got out of hand (the old one).

I would really appreciate it if you could take the time to take a look at it and provide some critiques.

Some notes about the project:

  • I wanted to write most everything from scratch instead of using libraries in order to better learn.
  • I am thinking of using a different database, possibly one that does not need to be installed that I can distribute with the code.

Here is a link to the source code: Github

Here are some of the more important/ugliest portions of code:

ParseTCPMessage() in Messages.cs

internal async void ParseTCPMessage(string tcpMessage)
{
    DateTime messageTime = DateTime.Now;
    Regex messageRegex = new Regex(@"^:(?<Sender>[^\s]+)\s(?<Type>[^\s]+)\s(?<Recipient>[^\s]+)\s?:?(?<Args>.*)", RegexOptions.None);
    Regex senderRegex = new Regex(@"^(?<Nick>[^\s]+)!(?<Realname>[^\s]+)@(?<Host>[^\s]+)", RegexOptions.None);
    Regex pingRegex = new Regex(@"^PING :(?<Message>.+)", RegexOptions.None);
    Regex pongRegex = new Regex(@"^PONG :(?<Message>.+)", RegexOptions.None);
    Regex errorRegex = new Regex(@"^ERROR :(?<Message>.+)", RegexOptions.None);
    Regex CTCPRegex = new Regex(@"^\u0001(?<Command>[^\s]+)\s?(?<Args>.*)\u0001", RegexOptions.None);

    string[] messages = tcpMessage.Split(new string[] { Environment.NewLine }, StringSplitOptions.RemoveEmptyEntries);

    foreach (string message in messages)
    {
        if (messageRegex.IsMatch(message))
        {
            Match match = messageRegex.Match(message);
            string type = match.Groups["Type"].Value;
            string sender = match.Groups["Sender"].Value;
            string recipient = match.Groups["Recipient"].Value;
            string args = match.Groups["Args"].Value;
            Match senderMatch = senderRegex.Match(sender);
            string senderNick = sender;
            string senderRealname = sender;
            string senderHost = sender;
            if (senderMatch.Success)
            {
                senderNick = senderMatch.Groups["Nick"].Value;
                senderRealname = senderMatch.Groups["Realname"].Value;
                senderHost = senderMatch.Groups["Host"].Value;
            }

            int replyCode;
            if (int.TryParse(type, out replyCode))
            {
                // The message was a reply to a command sent
                if (Enum.IsDefined(typeof(IRCReplyCode), replyCode))
                {
                    await Task.Run(() =>
                    {
                        if (ServerReplyEvent != null)
                        {
                            ServerReplyEvent(this,
                                new ServerReplyMessage()
                                {
                                    TimeStamp = messageTime,
                                    ReplyCode = (IRCReplyCode) replyCode,
                                    Message = args
                                });
                        }
                    });
                }
                else if (Enum.IsDefined(typeof(IRCErrorCode), replyCode))
                {
                    await Task.Run(() =>
                    {
                        if (ServerReplyEvent != null)
                        {
                            ServerReplyEvent(this,
                                new ServerErrorMessage()
                                {
                                    TimeStamp = messageTime,
                                    ErrorCode = (IRCErrorCode) replyCode,
                                    Message = args
                                });
                        }
                    });
                }
            }
            else
            {
                switch (type)
                {
                    // The message was a private message to a channel or nick
                    case "PRIVMSG":
                        if (CTCPRegex.IsMatch(args))
                        {
                            Match ctcpMatch = CTCPRegex.Match(args);
                            CTCPMessage ctcpMessage = new CTCPMessage();
                            ctcpMessage.Sender = new Nick()
                            {
                                Nickname = senderNick,
                                Realname = senderRealname,
                                Host = senderHost
                            };
                            ctcpMessage.Command = ctcpMatch.Groups["Command"].Value;
                            ctcpMessage.Arguments = ctcpMatch.Groups["Args"].Value;

                            await Task.Run(() =>
                            {
                                if (CTCPMessageReceivedEvent != null)
                                {
                                    CTCPMessageReceivedEvent(this, ctcpMessage);
                                }
                            });
                        }
                        else
                        {
                            if (recipient.StartsWith("&") || recipient.StartsWith("#"))
                            {
                                ChannelMessage msg = new ChannelMessage();
                                msg.Channel = recipient;
                                msg.Sender = new Nick()
                                {
                                    Nickname = senderNick,
                                    Realname = senderRealname,
                                    Host = senderHost
                                };
                                msg.Message = args;

                                await Task.Run(() =>
                                {
                                    if (ChannelMessageReceivedEvent != null)
                                    {
                                        ChannelMessageReceivedEvent(this, msg);
                                    }
                                });
                            }
                            else
                            {
                                PrivateMessage msg = new PrivateMessage();
                                msg.Sender = new Nick()
                                {
                                    Nickname = senderNick,
                                    Realname = senderRealname,
                                    Host = senderHost
                                };
                                msg.Message = args;

                                await Task.Run(() =>
                                {
                                    if (PrivateMessageReceivedEvent != null)
                                    {
                                        PrivateMessageReceivedEvent(this, msg);
                                    }
                                });
                            }
                        }
                        break;
                    // The message was a notice to a channel or nick
                    case "NOTICE":
                        if (CTCPRegex.IsMatch(args))
                        {
                            Match ctcpMatch = CTCPRegex.Match(args);
                            CTCPMessage ctcpMessage = new CTCPMessage();
                            ctcpMessage.Sender = new Nick()
                            {
                                Nickname = senderNick,
                                Realname = senderRealname,
                                Host = senderHost
                            };
                            ctcpMessage.Command = ctcpMatch.Groups["Command"].Value;
                            ctcpMessage.Arguments = ctcpMatch.Groups["Args"].Value;

                            await Task.Run(() =>
                            {
                                if (CTCPNoticeReceivedEvent != null)
                                {
                                    CTCPNoticeReceivedEvent(this, ctcpMessage);
                                }
                            });
                        }
                        if (recipient.StartsWith("&") || recipient.StartsWith("#"))
                        {
                            ChannelNotice msg = new ChannelNotice();
                            msg.Channel = recipient;
                            msg.Sender = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };
                            msg.Message = args;

                            await Task.Run(() =>
                            {
                                if (ChannelNoticeReceivedEvent != null)
                                {
                                    ChannelNoticeReceivedEvent(this, msg);
                                }
                            });
                        }
                        else
                        {
                            PrivateNotice msg = new PrivateNotice();
                            msg.Sender = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };
                            msg.Message = args;

                            await Task.Run(() =>
                            {
                                if (PrivateNoticeReceivedEvent != null)
                                {
                                    PrivateNoticeReceivedEvent(this, msg);
                                }
                            });
                        }
                        break;
                    // The message was a mode change message for a channel or nick
                    case "MODE":
                        if (recipient.StartsWith("&") || recipient.StartsWith("#"))
                        {
                            ChannelModeChangeInfo modeMsg = new ChannelModeChangeInfo();
                            modeMsg.Modes = new List<ChannelModeInfo>();
                            modeMsg.Channel = recipient;
                            modeMsg.Nick = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };

                            string[] modeArgs = args.Split(new char[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
                            List<string> argList = modeArgs.ToList();
                            argList.RemoveAt(0);
                            modeMsg.Modes.AddRange(_IRC.ParseChannelModeString(modeArgs[0].TrimStart(':'), string.Join(" ", argList)));

                            await Task.Run(() =>
                            {
                                if (ChannelModeChangeEvent != null)
                                {
                                    ChannelModeChangeEvent(this, modeMsg);
                                }
                            });
                        }
                        else
                        {
                            UserModeChangeInfo modeMsg = new UserModeChangeInfo();
                            modeMsg.Modes = new List<UserModeInfo>();
                            modeMsg.Nick = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };

                            string[] modeArgs = args.Split(new char[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
                            char[] modeInfo = modeArgs[0].TrimStart(':').ToCharArray();
                            bool set = true;
                            foreach (char mode in modeInfo)
                            {
                                if (mode.Equals('-'))
                                {
                                    set = false;
                                }
                                else if (mode.Equals('+'))
                                {
                                    set = true;
                                }
                                else if (mode.ToString() != string.Empty)
                                {
                                    UserModeInfo newMode = new UserModeInfo();
                                    newMode.Set = set;
                                    newMode.Mode = (UserMode)Enum.Parse(typeof(UserMode), mode.ToString());
                                    modeMsg.Modes.Add(newMode);
                                }
                            }

                            await Task.Run(() =>
                            {
                                if (UserModeChangeEvent != null)
                                {
                                    UserModeChangeEvent(this, modeMsg);
                                }
                            });
                        }
                        break;
                    // The message was a topic change for a channel
                    case "TOPIC":
                        TopicChangeInfo topicMsg = new TopicChangeInfo();
                        topicMsg.Channel = recipient;
                        topicMsg.Nick = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };
                        topicMsg.Topic = args;

                        await Task.Run(() =>
                        {
                            if (TopicChangeEvent != null)
                            {
                                TopicChangeEvent(this, topicMsg);
                            }
                        });
                        break;
                    // The message was a nick change
                    case "NICK":
                        NickChangeInfo nickMsg = new NickChangeInfo();
                        nickMsg.OldNick = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };
                        nickMsg.NewNick = new Nick() { Nickname = recipient.TrimStart(':') };

                        await Task.Run(() =>
                        {
                            if (NickChangeEvent != null)
                            {
                                NickChangeEvent(this, nickMsg);
                            }
                        });
                        break;
                    // The message was an invite to a channel
                    case "INVITE":
                        InviteChannelInfo inviteMsg = new InviteChannelInfo();
                        inviteMsg.Requester = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };
                        inviteMsg.Recipient = new Nick() { Nickname = recipient };
                        inviteMsg.Channel = args;

                        await Task.Run(() =>
                        {
                            if (InviteChannelEvent != null)
                            {
                                InviteChannelEvent(this, inviteMsg);
                            }
                        });
                        break;
                    // The message was a nick joining a channel
                    case "JOIN":
                        JoinChannelInfo joinMsg = new JoinChannelInfo();
                        joinMsg.Channel = recipient.TrimStart(':');
                        joinMsg.Nick = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };

                        await Task.Run(() =>
                        {
                            if (JoinChannelEvent != null)
                            {
                                JoinChannelEvent(this, joinMsg);
                            }
                        });
                        break;
                    // The message was a nick parting a channel
                    case "PART":
                        PartChannelInfo partMsg = new PartChannelInfo();
                        partMsg.Channel = recipient;
                        partMsg.Nick = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };

                        await Task.Run(() =>
                        {
                            if (PartChannelEvent != null)
                            {
                                PartChannelEvent(this, partMsg);
                            }
                        });
                        break;
                    // The message was a nick being kicked from a channel
                    case "KICK":
                        KickInfo kickMsg = new KickInfo();
                        kickMsg.Channel = recipient;
                        kickMsg.Nick = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };
                        string[] argSplit = args.Split(new char[] { ' ' }, StringSplitOptions.None);

                        kickMsg.KickedNick = new Nick() { Nickname = argSplit[0], Realname = argSplit[0], Host = argSplit[0] };

                        List<string> reasonArgs = argSplit.ToList<string>();
                        reasonArgs.RemoveAt(0);
                        kickMsg.Reason = string.Join(" ", reasonArgs.ToArray()).Remove(0, 1);

                        await Task.Run(() =>
                        {
                            if (KickEvent != null)
                            {
                                KickEvent(this, kickMsg);
                            }
                        });
                        break;
                    // The message was a nick quiting the irc network
                    case "QUIT":
                        QuitInfo quitMsg = new QuitInfo();
                        quitMsg.Nick = new Nick() { Nickname = senderNick, Realname = senderRealname, Host = senderHost };
                        quitMsg.Message = string.Join(" ", recipient.Remove(0, 1), args);

                        await Task.Run(() =>
                        {
                            if (QuitEvent != null)
                            {
                                QuitEvent(this, quitMsg);
                            }
                        });
                        break;
                    default:
                        break;
                }
            }
        }
        else if (pingRegex.IsMatch(message)) // The message was a PING
        {
            Match match = pingRegex.Match(message);
            PingInfo ping = new PingInfo();
            ping.Message = match.Groups["Message"].Value;

            await Task.Run(() =>
            {
                if (PingEvent != null)
                {
                    PingEvent(this, ping);
                }
            });
        }
        else if (pongRegex.IsMatch(message)) // The message was a PONG
        {
            Match match = pongRegex.Match(message);
            PongInfo pong = new PongInfo();
            pong.Message = match.Groups["Message"].Value;

            await Task.Run(() =>
            {
                if (PongEvent != null)
                {
                    PongEvent(this, pong);
                }
            });
        }
        else if (errorRegex.IsMatch(message)) // The message was a server error
        {
            Match match = errorRegex.Match(message);
            ErrorMessage error = new ErrorMessage();
            error.Message = match.Groups["Message"].Value;

            if (ErrorMessageEvent != null)
            {
                ErrorMessageEvent(this, error);
            }
        }

        string rawMessage = message;
        await Task.Run(() =>
        {
            if (RawMessageEvent != null)
            {
                RawMessageEvent(this, rawMessage);
            }
        });
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ For performance's sake, and for declaring intent, I'd move those Regexes out of the method and as static readonly class-level member variables. And in place of RegexOptions.None I'd use RegexOptions.Compiled. \$\endgroup\$ Commented Apr 27, 2015 at 20:46
  • 1
    \$\begingroup\$ Also, your event raising pattern could run afoul of race conditions in which the event handler is disconnected between checking if it is null and raising it. The pattern should be: var rawMessageEvent = RawMessageEvent; if (rawMessageEvent != null) { rawMessageEvent(this, rawMessage); }. Assigning to a local variable keeps the view of its value consistent. \$\endgroup\$ Commented Apr 27, 2015 at 20:59

2 Answers 2

4
\$\begingroup\$

The most obvious improvement would be to first extract methods out of each case in that disastrous switch(type) block... and then to break each if...else block into its own methods too - the idea being to reduce nesting, and come up with small methods that do little.

Actually the first thing to extract is near the beginning of the loop: each match possibility belongs in its own method.

And then once you've extracted them into their own method, you can extract each one into a strategy pattern, because what you're doing can be stated as follows:

  • Find a regex pattern that matches the input
  • Use the matching regex to parse the input
  • Fire up an event that's specific to the type of pattern we matched

Your code is very procedural, which is rather unusual in the object-oriented paradigm C# is offering.

I would think more in terms of objects here: there's a strategy to adopt depending on the type of message you're receiving - each strategy belongs in its own class. And since all these strategies are really doing the same thing, they should share an interface, so that your main loop can call any of them, without actually caring which one it's calling.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks, yea, there are some duplicate actions within the nesting I can pull out to a function. Then I could keep the case statement for only determining what kind of message it is, and call the respective function. \$\endgroup\$ Commented Apr 27, 2015 at 20:32
4
\$\begingroup\$

The very length of your code, and the complexity of the routines, makes it hard to read. After you refactor it into shorter routines as @Mat'sMug suggested, post it again and you'll get more reviews.

As it stands, I can only make a few observations:

  • Did you check to see if it might be worthwhile to create long-lived Regex objects with RegexOptions.Compiled? Regex does do automatic caching, but that only works inside static methods.
  • To save on memory usage, you could write a string splitter that returns an IEnumerable<string> using yield return, since you're only iterating over it using foreach.
  • in the Regexes where you're only matching one thing, you don't need to name the capturing group.
  • does the check for if (ServerReplyEvent != null) need to be inside the await? (and similarly for others)
\$\endgroup\$

Your Answer

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

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