C# Chat - Part 2: Client

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(" 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 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
{
ret

StackExchange Code Review Q#86028, answer score: 2