4
\$\begingroup\$

This is the second part of a multi-part review. The first part, the server for this client, can be found here.

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

In this second part, I'd like to get some feedback on my client. It feels leaner and more efficient than the server, but I don't doubt that there are plenty of problems in here.

Program.cs

using System;
using System.Collections.Generic;
using System.Threading;
using System.Runtime.InteropServices;
using System.Windows.Forms;

namespace MessengerClient
{
    class Program
    {
        private static Thread receiverThread;

        private static bool FirstRun = true;

        static void Main(string[] args)
        {
            if (FirstRun)
            {
                Console.BackgroundColor = ConsoleColor.White;
                Console.ForegroundColor = ConsoleColor.Black;
                Console.Clear();
                Application.ApplicationExit += new EventHandler(QuitClient);
                FirstRun = false;
            }

            if (args.Length == 1 && args[0] == "--debug")
            {
                Console.WriteLine("<DEBUG> Setting debug mode ON...");
                Output.DebugMode = true;
            }

            Console.WriteLine("Enter the IP to connect to, including the port:");
            string address = Console.ReadLine();
            try
            {
                string[] parts = address.Split(':');
                receiverThread = new Thread(new ParameterizedThreadStart(Receiver.Start));
                receiverThread.Start(address);
                Client.Start(parts[0], Int32.Parse(parts[1]));
            }
            catch (Exception e)
            {
                Console.Clear();
                Output.Message(ConsoleColor.DarkRed, "Could not connect: " + e.Message);
                Main(new string[1]);
            }
        }

        private static void QuitClient(object sender, EventArgs e)
        {
            Client.Disconnect();
            while (!Commands.ExitHandlingFinished)
            {
                Thread.Sleep(100);
            }
        }
    }
}

Client.cs

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

namespace MessengerClient
{
    class Client
    {
        private static ASCIIEncoding encoder = new ASCIIEncoding();

        private static int clientId = 0;

        public static int GetClientId()
        {
            return clientId;
        }

        public static TcpClient client = new TcpClient();

        private static IPEndPoint serverEndPoint;

        public static void Start(string ip, int port)
        {
            serverEndPoint = new IPEndPoint(IPAddress.Parse(ip), port);

            try
            {
                client.Connect(serverEndPoint);
            }
            catch (Exception e)
            {
                throw new Exception("No connection was made: " + e.Message);
            }

            while (true)
            {
                Output.Write(ConsoleColor.DarkBlue, "Me: ");
                Console.ForegroundColor = ConsoleColor.DarkBlue;
                string message = Console.ReadLine();
                Console.ForegroundColor = ConsoleColor.Black;

                if (Commands.IsCommand(message))
                {
                    Commands.HandleCommand(client, message);
                    continue;
                }

                SendMessage(message);
            }
        }

        public static void SendMessage(string message)
        {
            NetworkStream clientStream = client.GetStream();
            byte[] buffer;
            if (message.StartsWith("[Disconnect]") || message.StartsWith("[Command]"))
            {
                buffer = encoder.GetBytes(message);
            }
            else
            {
                buffer = encoder.GetBytes("[Send]" + message);
            }

            clientStream.Write(buffer, 0, buffer.Length);
            clientStream.Flush();
        }

        public static void HandleResponse(ResponseCode code)
        {
            switch (code)
            {
                case ResponseCode.Success:
                    return;
                case ResponseCode.ServerError:
                    Output.Message(ConsoleColor.DarkRed, "The server could not process your message. (100)");
                    break;
                case ResponseCode.NoDateFound:
                    Output.Message(ConsoleColor.DarkRed, "Could not retrieve messages from the server. (200)");
                    break;
                case ResponseCode.BadDateFormat:
                    Output.Message(ConsoleColor.DarkRed, "Could not retrieve messages from the server. (201)");
                    break;
                case ResponseCode.NoMessageFound:
                    Output.Message(ConsoleColor.DarkRed, "The server could not process your message. (300)");
                    break;
                case ResponseCode.NoHandlingProtocol:
                    Output.Message(ConsoleColor.DarkRed, "The server could not process your message. (400)");
                    break;
                case ResponseCode.NoCode:
                    Output.Message(ConsoleColor.DarkRed, "Could not process the server's response. (NoCode)");
                    break;
                default:
                    return;
            }
        }

        public static void ParseClientId(string id)
        {
            clientId = Int32.Parse(id);
        }

        public static void Disconnect()
        {
            SendMessage("[Disconnect]");
            Commands.EndRcvThread = true;
            Output.Debug("Requested receive thread termination.");
            Output.Message(ConsoleColor.DarkGreen, "Shutting down...");
        }
    }
}

Receiver.cs

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

namespace MessengerClient
{
    class Receiver
    {
        private static TcpClient client = new TcpClient();

        private static IPEndPoint serverEndPoint;

        public static void Start(object address)
        {
            string[] parts = ((string) address).Split(':');

            try
            {
                serverEndPoint = new IPEndPoint(IPAddress.Parse(parts[0]), Int32.Parse(parts[1]));
            }
            catch (Exception e)
            {
                Output.Message(ConsoleColor.DarkRed, "Could not connect: " + e.Message);
                return;
            }

            try
            {
                client.Connect(serverEndPoint);
                client.ReceiveTimeout = 500;
            }
            catch (Exception e)
            {
                Output.Message(ConsoleColor.DarkRed, "Could not connect: " + e.Message);
                return;
            }

            NetworkStream stream = client.GetStream();
            string data = "";
            byte[] received = new byte[4096];

            while (true)
            {
                if (Commands.EndRcvThread)
                {
                    Output.Debug("Ending receiver thread");
                    client.Close();
                    Output.Debug("Cleaned up receive client");
                    Commands.RcvThreadEnded = true;
                    Commands.HandleResponse("[DisconnectAcknowledge]");
                    Output.Debug("Notified Commands handler of thread abortion");
                    Thread.CurrentThread.Abort();
                    return;
                }

                data = "";
                received = new byte[4096];

                int bytesRead = 0;
                try
                {
                    bytesRead = stream.Read(received, 0, 4096);
                }
                catch (Exception e)
                {
                    continue;
                }

                if (bytesRead == 0)
                {
                    break;
                }

                int endIndex = received.Length - 1;
                while (endIndex >= 0 && received[endIndex] == 0)
                {
                    endIndex--;
                }

                byte[] finalMessage = new byte[endIndex + 1];
                Array.Copy(received, 0, finalMessage, 0, endIndex + 1);

                data = Encoding.ASCII.GetString(finalMessage);
                Output.Debug("Server message: " + data);

                try
                {
                    ProcessMessage(data);
                }
                catch (Exception e)
                {
                    Output.Message(ConsoleColor.DarkRed, "Could not process the server's response (" + data + "): " + e.Message);
                }
            }
        }

        public static void ProcessMessage(string response)
        {
            Output.Debug("Processing message: " + response);
            response = response.Trim();

            if (response.StartsWith("[Message]"))
            {
                Output.Debug("Starts with [Message], trying to find ID");
                response = response.Substring(9);

                int openIndex = response.IndexOf("<");
                int closeIndex = response.IndexOf(">");

                if (openIndex < 0 || closeIndex < 0 || closeIndex < openIndex)
                {
                    Output.Debug("No ID tag? ( <ID-#-HERE> )");
                    throw new FormatException("Could not find ID tag in message");
                }

                int diff = closeIndex - openIndex;
                int id = Int32.Parse(response.Substring(openIndex + 1, diff - 1));
                if (id != Client.GetClientId())
                {
                    string message = response.Substring(closeIndex + 1);
                    Console.WriteLine();
                    Output.Message(ConsoleColor.DarkYellow, "<Stranger> " + message);
                    Output.Write(ConsoleColor.DarkBlue, "Me: ");
                }
                else
                {
                    Output.Debug("ID is client ID, not displaying.");
                }
            }
            else if (response == "[DisconnectAcknowledge]" || response == "[CommandInvalid]")
            {
                Output.Debug("Sending response to Commands handler: " + response);
                Commands.HandleResponse(response);
            }
            else if (response.Length == 5 && response.StartsWith("[") && response.EndsWith("]"))
            {
                Client.HandleResponse(ResponseCodes.GetResponse(response));
            }
            else
            {
                Output.Debug("Figuring out what to do with server message: " + response);
                try
                {
                    Int32.Parse(response);
                    Output.Debug("Int32.Parse has not failed, assume client ID sent.");
                    Client.ParseClientId(response);
                    return;
                }
                catch (Exception e) {
                    Output.Debug("Could not process client ID: " + e.Message);
                }
                Output.Debug("Could not identify what to do with message.");
                Output.Message(ConsoleColor.DarkCyan, "<Server> " + response);
            }
        }
    }
}

ResponseCodes.cs

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

namespace MessengerClient
{
    public enum ResponseCode
    {
        Success,
        ServerError,
        NoDateFound,
        BadDateFormat,
        NoMessageFound,
        NoHandlingProtocol,
        NoCode,
        NoResponse
    }

    class ResponseCodes
    {
        public static Dictionary<string, ResponseCode> CodeStrings = new Dictionary<string, ResponseCode>
        {
            {"[600]", ResponseCode.Success},
            {"[100]", ResponseCode.ServerError},
            {"[200]", ResponseCode.NoDateFound},
            {"[201]", ResponseCode.BadDateFormat},
            {"[300]", ResponseCode.NoMessageFound},
            {"[400]", ResponseCode.NoHandlingProtocol},
        };

        public static ResponseCode GetResponse(string code)
        {
            if (CodeStrings.ContainsKey(code))
            {
                return CodeStrings[code];
            }
            else
            {
                return ResponseCode.NoCode;
            }
        }
    }
}

Commands.cs

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

namespace MessengerClient
{
    class Commands
    {
        public static volatile bool EndRcvThread = false;

        public static volatile bool RcvThreadEnded = false;

        public static bool ExitHandlingFinished = false;

        public static bool IsCommand(string command)
        {
            if (command.StartsWith("/"))
            {
                return true;
            }
            else
            {
                return false;
            }
        }

        public static void HandleCommand(TcpClient client, string command)
        {
            string[] args = command.Split(' ');
            switch (args[0].ToLower())
            {
                case "/server":
                    if (args.Length >= 2)
                    {
                        int startIndex = args[0].Length;
                        string commandArgs = command.Substring(startIndex + 1);
                        Client.SendMessage("[Command]" + commandArgs);
                    }
                    else
                    {
                        Output.Message(ConsoleColor.DarkRed, "Not enough arguments");
                        return;
                    }
                    break;
                case "/exit":
                    Client.Disconnect();
                    break;
                default:
                    Output.Message(ConsoleColor.DarkRed, "Unknown command.");
                    return;
            }
        }

        public static void HandleResponse(string response)
        {
            // Command was sent; server did not recognise
            if (response == "[CommandInvalid]")
            {
                Output.Message(ConsoleColor.DarkRed, "The command was not recognised by the server.");
                return;
            }

            // Disconnect was sent; server acknowledges
            if (response == "[DisconnectAcknowledge]")
            {
                EndRcvThread = true;
                Output.Debug("Waiting for thread termination");
                while (!RcvThreadEnded)
                {
                    Thread.Sleep(100);
                }
                Output.Debug("Thread terminated, cleaning send client");
                Client.SendMessage("");
                Client.client.Close();
                Output.Debug("Cleaned up send client");
                if (Output.DebugMode)
                {
                    Console.WriteLine();
                    Output.Debug("Press any key to exit");
                    Console.ReadKey();
                }
                Environment.Exit(0);
            }

            // Fallback for neither case: pass it off to the client
            ResponseCode code = ResponseCodes.GetResponse(response);
            Client.HandleResponse(code);
        }
    }
}

The final class, Output.cs, is the same class as in the last post, and I'm still happy with it so am not putting it up for review. Please also note, I do have XML documentation comments in the code but to save characters have excluded them here.

\$\endgroup\$
2
\$\begingroup\$
private static bool FirstRun = true;

private fields are lowerCamelCase or _lowerCamelCase, the latter receiving much preference.


if (args.Length == 1 && args[0] == "--debug")

This is fine if you only have one argument but as soon as you want multiple you'll have issues: people expect arguments to be swappable so you might want to look into making this more generic if you go in that direction.

I would also use args.Any() to make it more expressive.


Output.DebugMode = true;

I don't like the supposedly singleton instance of Output. You might just as well create a normal instance and pass it along to your client, no? Perhaps some dependency injection?


I would parse the Uri before you pass it to the client. A trick to do it with IP + Port could be this:

Uri uri = new Uri("http://" + "192.168.11.11:8080");
Console.WriteLine (uri.Host);
Console.WriteLine (uri.Port);

Keep it in a try-catch though because it will throw an exception if it's badly formatted.

This also solves the problem that you might have in case no port is specified (ArrayIndexOutOfRangeException) or that the IP can't be parsed into an IPEndPoint (FormatException).

On top of that it also keeps the responsibility of validation inside your main() block instead of passing exceptions through threads and all that stuff.


catch (Exception e)
   Console.Clear();

Don't clear my console! I use that to retract my steps and perhaps contact support.


Main(new string[1]);

What's the point of setting its length to 1? I do like the approach you used here to re-call the Main method.


Client.Disconnect();
while (!Commands.ExitHandlingFinished)
{
    Thread.Sleep(100);
}

This sort of polling should have a timeout in case something isn't going as expected. An indication to the user that the program is quitting is advised as well.


I'd advise you to group your members by their type so you know exactly where you can find something. group private fields, private static fields, public static fields, methods, etc.


public static TcpClient client = new TcpClient();

We don't do public fields in C#. This should be a property (why does the outside world even need to know about this inner detail?)


Too much static. This is very hard to test and limits scalability.


catch (Exception e)
{
    throw new Exception("No connection was made: " + e.Message);
}

Pass in the original exception as the new inner exception.


The only way to interrupt your chat program is by exiting the application. That isn't very nice -- I might want to keep the program open! Perhaps provide interruptability?


if (message.StartsWith("[Disconnect]") || message.StartsWith("[Command]"))

This is a simple approach and its purposes are clear but I would consider a custom object that holds a property Message and something like MessageKind which could be an enum of Message and ConnectionStatus, that sort of stuff. It allows you to add other variations more easily and doesn't restrict you to an exact string to work with.


public static void ParseClientId(string id)
{
    clientId = Int32.Parse(id);
}

Seems a little pointless -- You're even adding characters. I would also just use int instead of Int32 to retain conformity with the rest of the code.


Thread.CurrentThread.Abort();

You shouldn't have to abort the thread since that is considered unreliable. Just using return; should do the trick.


int endIndex = received.Length - 1;
while (endIndex >= 0 && received[endIndex] == 0)
{
    endIndex--;
}

This is a curious piece of code to me. Maybe I'm misinterpreting it so perhaps you can clarify: are you expecting 0 values being sent? Why would it do this?

You should probably add a comment to specify what you're doing (e.g.: // trimming useless data).


response = response.Substring(9);

Make clear why you're using 9. The above debug statement isn't adequate documentation (it's not explicitly linked to the line of code so people might remove it). It also strikes me more as a comment than debug output, really.


int openIndex = response.IndexOf("<");

This is the first I see of these fishbrackets. What are they used for? Commentate it!


Output.Debug("ID is client ID, not displaying.");

Should a client be able to talk to himself? If yes: you're not doing that. If no: this indicates something is fundamentally wrong! Throw an exception and let the user know -- don't just hide it in the logs.


Int32.Parse(response);
Output.Debug("Int32.Parse has not failed, assume client ID sent.");
Client.ParseClientId(response);

Double work for no reason. Consider using int.TryParse() instead.


EndRcvThread
RcvThreadEnded

Only a very select few abbreviations are recommended (db, app, etc). These aren't amongst them.

Again note that these are fields and not properties!


if (command.StartsWith("/"))
{
    return true;
}
else
{
    return false;
}

Also known as

return command.StartsWith("/")

args[0].ToLower()

String comparison should never be done like this for two main reasons:

  • Performance impact -- you create a new string. What if that first string barely fitted in your memory?

  • It's not a correct comparison. This comment makes it clear but I suggest reading the entire post as well.


Overall the code can be followed pretty well.
Two things I would definitely look into if I were you: threading and static-ness. I'm not versed enough in threading to give a meaningful review but certain static fields and thread handling raised some eyebrows.

The static-ness of your code is something you really should address though: It's very hard to test and your classes are very tightly coupled. I'd rather see instances being passed around where needed.

While on the note of testing: all your external dependencies are hardcoded in it -- look into dependency injection if you want to start unit-testing some things!

\$\endgroup\$
  • \$\begingroup\$ That's great thanks. I have to admit to being lazy when it comes to testing and prefer to just debug without automated tests... \$\endgroup\$ – ArtOfCode Apr 6 '15 at 18:21
  • \$\begingroup\$ static fields are fine in PascalCase... at least per R# default settings ;) \$\endgroup\$ – Mathieu Guindon Apr 6 '15 at 19:04

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.