C# Chat - Part 1: Server

Question

I've been building a simple C# server-client chat-style app as a test of my C#. I've picked up code from a few tutorials, and extended what's there to come up with my own server spec.

In this post (the second will be the client), I'd like to get some feedback on the server. To me, the code seems bulky and as if it could be brought down by some judicious use of functions or a utility class (I've spotted a doubled-up function (SendToClient) that I guess I could just make public to save on LOC, but what else is there?)

1 - Program.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace MessengerServer
{
    class Program
    {
        static void Main(string[] args)
        {
            int port = 1100;

            if (args.Length == 2 && args[0] == "--port")
            {
                try
                {
                    port = Int32.Parse(args[1]);
                }
                catch (Exception e)
                {
                    Output.Log("Not a valid port number. Defaulting to 1100.", LogType.Error);
                }
            }

            Output.Log("Starting server on port " + port, LogType.Info);

            new Server(port);
        }
    }
}

2 - Server.cs

using System;
using System.Collections.Generic;
using System.Net;
using System.Net.Sockets;
using System.Threading;
using System.Text;

namespace MessengerServer
{
    class Server
    {
        private TcpListener tcpListener;
        private Thread listenThread;
        private ASCIIEncoding encoder = new ASCIIEncoding();

        public Dictionary<DateTime, string> Messages = new Dictionary<DateTime, string>();
        private List<string> MessagesAfter(string time)
        {
            List<string> matches = new List<string>();
            DateTime sinceTime = DateTime.Parse(time);
            foreach (KeyValuePair<DateTime, string> pair in Messages)
            {
                if (pair.Key.CompareTo(sinceTime) > 0)
                {
                    matches.Add(pair.Value);
                }
            }
            return matches;
        }

        private List<TcpClient> connectedClients = new List<TcpClient>();

        public Server(int port)
        {
            Output.Log("Starting server: all network interfaces, port " + port, LogType.Info);
            this.tcpListener = new TcpListener(IPAddress.Any, port);
            this.listenThread = new Thread(new ThreadStart(ListenForClients));
            this.listenThread.Start();
        }

        private void ListenForClients()
        {
            Output.Log("Listener thread spawned", LogType.Info);

            this.tcpListener.Start();
            Output.Log("TCP listener started, ready to accept clients.", LogType.Info);

            while (true)
            {
                TcpClient client = this.tcpListener.AcceptTcpClient();
                Output.Log("Client connected, starting thread...", LogType.Info);

                Thread clientThread = new Thread(new ParameterizedThreadStart(HandleClientComms));
                clientThread.Start(client);
                connectedClients.Add((TcpClient) client);
            }
        }

        private void HandleClientComms(object client)
        {
            int clientId = new Random().Next(0, int.MaxValue - 1);
            TcpClient tcpClient = (TcpClient) client;
            NetworkStream clientStream = tcpClient.GetStream();
            Output.Log("Communication thread started with client at " + tcpClient.Client.RemoteEndPoint.ToString(), LogType.Info);
            Output.Log("Client identifier is " + clientId, LogType.Info);

            SendToClient(tcpClient, clientId.ToString());

            byte[] message = new byte[4096];
            int bytesRead;

            string data = "";

            while (true)
            {
                bytesRead = 0;
                data = "";

                try
                {
                    bytesRead = clientStream.Read(message, 0, 4096);
                    Output.Log("Read " + bytesRead + " bytes from " + clientId, LogType.Info);
                }
                catch(Exception e)
                {
                    Output.Log("Could not read from client: " + e.Message, LogType.Error);
                    if (e.GetType() == Type.GetType("System.IO.IOException"))
                    {
                        connectedClients.Remove(tcpClient);
                        Thread.CurrentThread.Abort();
                    }
                    break;
                }

                if (bytesRead == 0)
                {
                    Output.Log("Client " + clientId + " disconnected", LogType.Info);
                    connectedClients.Remove(tcpClient);
                    Thread.CurrentThread.Abort();
                    break;
                }

                string received = encoder.GetString(message, 0, bytesRead);
                data += received;
                Output.Log("Message: " + received, LogType.Info);

                try
                {
                    HandleMessage(tcpClient, clientId, data);
                }
                catch (ThreadAbortException tae)
                {
                    Output.Log("Client thread disconnect complete: " + tae.Message, LogType.Info);
                }
                catch (Exception e)
                {
                    Output.Log("Could not handle message: " + e.Message, LogType.Warn);
                }
            }
        }

        public void SendToClient(TcpClient client, string message)
        {
            try
            {
                NetworkStream stream = client.GetStream();
                byte[] msg = new byte[message.Length];
                msg = encoder.GetBytes(message);
                stream.Write(msg, 0, message.Length);
                stream.Flush();
                Output.Log("Sent to client: " + message, LogType.Info);
            }
            catch (Exception e)
            {
                Output.Log("Unexpected exception sending: " + e.Message, LogType.Error);
            }
        }

        private void HandleMessage(TcpClient client, int clientId, string message)
        {
            Output.Log("Attempting to handle message " + message, LogType.Info);
            if (message.StartsWith("[AllSince]"))
            {
                try
                {
                    string date = message.Split(']')[1];
                    List<string> messages = MessagesAfter(date);
                    string data = "";
                    foreach (string msg in messages)
                    {
                        data += msg + "|&|";
                    }
                    SendToClient(client, data);
                }
                catch (IndexOutOfRangeException e)
                {
                    SendToClient(client, "[200]");
                    throw new Exception("No date was found for [AllSince]: " + e.Message);
                }
                catch (FormatException e)
                {
                    SendToClient(client, "[201]");
                    throw new Exception("Date was not formatted correctly: " + e.Message);
                }
                catch (Exception e)
                {
                    SendToClient(client, "[100]");
                    throw new Exception("Unexpected exception: " + e.Message);
                }
            }
            else if (message.StartsWith("[Send]"))
            {
                try
                {
                    string text = message.Split(']')[1];
                    Messages.Add(DateTime.Now, "<" + clientId + ">" + text);
                    Output.Log("Added to message list: " + text, LogType.Info);
                    NotifyAllClients("<" + clientId + ">" + text);
                    SendToClient(client, "[600]");
                }
                catch (Exception e)
                {
                    SendToClient(client, "[300]");
                    throw new Exception("No message could be found for [Send]: " + e.Message);
                }
            }
            else if (message.StartsWith("[Disconnect]"))
            {
                Output.Log("Client " + clientId + "'s client thread disconnected", LogType.Warn);
                connectedClients.Remove(client);
                Thread.CurrentThread.Abort();
            }
            else if (message.StartsWith("[Command]"))
            {
                string command = message.Substring(9);
                Commands.HandleCommand(client, command);
            }
            else
            {
                SendToClient(client, "[400]");
                throw new Exception("No handling protocol specified.");
            }
        }

        public void NotifyAllClients(string message)
        {
            Output.Log("Notifying all clients of message " + message, LogType.Info);
            foreach (TcpClient client in connectedClients)
            {
                Output.Log("Notify: client " + client.Client.RemoteEndPoint.ToString(), LogType.Info);
                SendToClient(client, "[Message]" + message);
            }
        }
    }
}

3 - Commands.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Net;
using System.Net.Sockets;

namespace MessengerServer
{
    class Commands
    {
        public static void HandleCommand(TcpClient client, string command)
        {
            string[] args = command.Split(' ');
            switch (args[0].ToLower().Trim())
            {
                case "force":
                    if (args.Length == 2)
                    {
                        SendToClient(client, "[" + args[1] + "]");
                    }
                    else
                    {
                        SendToClient(client, "[CommandInvalid]");
                    }
                    break;
                default:
                    SendToClient(client, "[CommandInvalid]");
                    break;
            }
        }

        public static void SendToClient(TcpClient client, string message)
        {
            try
            {
                NetworkStream stream = client.GetStream();
                byte[] msg = new byte[message.Length];
                msg = Encoding.ASCII.GetBytes(message);
                stream.Write(msg, 0, message.Length);
                stream.Flush();
                Output.Log("Sent to client: " + message, LogType.Info);
            }
            catch (Exception e)
            {
                Output.Log("Unexpected exception sending: " + e.Message, LogType.Error);
            }
        }
    }
}

The other class, Output (in Output.cs) is one I'm happy with - I use it widely and it's pretty solid, so I've decided not to put it up for review.

Note: I've also got XML documentation comments in my code, but have excluded them here for succinctness.

Answer 1

I would start with looking at the private methods. Looks like we have quite a few of them. Based on the work it's doing my first stab would be to extract them as their own classes and expose the methods as public methods used by the server. This way you can actually add unit tests around your classes.

e.g. I would take HandleClientComms() as my first candidate to be extracted out. Once we have it out the other private methods that are called by `HandleClientComms() would be something that I would consider as the next candidates to be their own classes.

 public interface IHandleClientCommunications
{
    void Handle(TcpClient client);
}

The Server Class that has the method below would look something like below :

private void ListenForClients()
    {
        //Output.Log("Listener thread spawned", EntityConstants.LogType.Info);

        this.tcpListener.Start();
        //Output.Log("TCP listener started, ready to accept clients.", EntityConstants.LogType.Info);

        while (true)
        {
            TcpClient client = this.tcpListener.AcceptTcpClient();

            Thread clientThread = new Thread(o => _handleClientCommunications.Handle(client));
            clientThread.Start(client);
            connectedClients.Add((TcpClient)client);
        }
    }

Once you do this you will see that a lot of your private methods move out of the Server class as they are not needed there. They will be moved under the new class that we introduced above. This is ok. Next thing to look at it is to Extract HandleMessage() (I smell Strategy Pattern once its extracted. I can see we have quite a few if...else where we do different algorithms based on a certain criteria. This is a good candidate to refactor)

Extract out NotifyAllClients()(This should take care of Client and Clients). Extract out RemoveClient().

Note as you start extracting out things you want to ensure that you are adding tests.

Usually I stick with

1. the Single Responsibility
2. Dependency Inversion principle
3. Ability to write unit tests for the piece of software.

as my yardstick to decide if a certain object needs to be broken down or not. This guideline helps to me to avoid the urge of over- or under-engineering a certain piece of software.

There are a lot of good books out there that have helped me immensely to grow as a software Engineer. I list a few below:

• http://martinfowler.com/books/refactoring.html
• http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

Answer 2

Program.cs

• int port = 1100; This is the default port number, right? What if you need to change the default? You should have the default declared elsewhere (that is, not inside the code of a function) as a const (or readonly). Don't use unexplained magic numbers. Especially since it's repeated a few lines down in the Defaulting to message. Don't repeat yourself. There should be one and only one instance of 1100.
• Why do you only allow 2 arguments? Why don't you display some error message if the command-line arguments are invalid (not just 2, or the param is not "--port")? The caller should know that their command-line switches failed and why, rather than just using defaults.
• "--port" is also a magic string; it shouldn't be just there in the code.
• Int32.Parse(args[1]); Why not use if (TryParse())? Code would be simpler and not rely on exceptions.

Server.cs

• Dictionary<DateTime, string> should be SortedDictionary<> and you should be able to use LINQ to pick out the strings in a date range.
• int clientId = new Random().Next(0, int.MaxValue - 1); no guarantee you won't get the same int twice in a row. Does it serve any purpose at all?
• bytesRead = clientStream.Read(message, 0, 4096); There's no guarantee you'll get a complete message. You have to keep reading until you actually have one, so that way you don't pass incomplete messages to your handler. TCP is stream oriented, not message oriented.
• stream.Flush(); Why are you doing this? It's almost never necessary.

Commands.cs