patterncsharpMinor
Writing an IRC Bot from scratch
Viewed 0 times
scratchwritingbotircfrom
Problem
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:
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(@"^:(?[^\s]+)\s(?[^\s]+)\s(?[^\s]+)\s?:?(?.*)", RegexOptions.None);
Regex senderRegex = new Regex(@"^(?[^\s]+)!(?[^\s]+)@(?[^\s]+)", RegexOptions.None);
Regex pingRegex = new Regex(@"^PING :(?.+)", RegexOptions.None);
Regex pongRegex = new Regex(@"^PONG :(?.+)", RegexOptions.None);
Regex errorRegex = new Regex(@"^ERROR :(?.+)", RegexOptions.None);
Regex CTCPRegex = new Regex(@"^\u0001(?[^\s]+)\s?(?.*)\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;
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(@"^:(?[^\s]+)\s(?[^\s]+)\s(?[^\s]+)\s?:?(?.*)", RegexOptions.None);
Regex senderRegex = new Regex(@"^(?[^\s]+)!(?[^\s]+)@(?[^\s]+)", RegexOptions.None);
Regex pingRegex = new Regex(@"^PING :(?.+)", RegexOptions.None);
Regex pongRegex = new Regex(@"^PONG :(?.+)", RegexOptions.None);
Regex errorRegex = new Regex(@"^ERROR :(?.+)", RegexOptions.None);
Regex CTCPRegex = new Regex(@"^\u0001(?[^\s]+)\s?(?.*)\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;
Solution
The most obvious improvement would be to first extract methods out of each
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:
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.
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.
Context
StackExchange Code Review Q#88150, answer score: 4
Revisions (0)
No revisions yet.