TCP/IP multithread chat

I'm learning C# and would love to receive some feedback for my code.

Server:

```
class Program
{
const char EoM = (char)3; //End of Messege mark
const char separator = (char)29; // main delimiter
const char clientSeparator = (char)31; //client delimiter

enum broadcastStatus
{
connect,
disconnect
};
static readonly object _lock = new object();
static List client_list = new List();
static int count = 0;

static void Main(string[] args)
{
Socket serverSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
serverSocket.Bind(new IPEndPoint(IPAddress.Any, 7000));
serverSocket.Listen(1);
Console.WriteLine("");

while (true)
{
Connection connection = new Connection();
connection.socket = serverSocket.Accept();
Thread connectionThread = new Thread(() => handle_connection(connection));
connectionThread.Start();
}
}

static void handle_connection(Connection connection)
{

//get client nickname

connection.nickname = receive_data(connection);

if (authenticate(connection.nickname))
{
//adding to list
broadcast_status(connection.nickname, broadcastStatus.connect);
lock (_lock) client_list.Add(connection);
count++;

// send ok to client && send client list
string response = "" + separator + client_list_to_string();
send_data(connection, response);

try
{
//listening and broadcasting to others clients
while (true)
{
string data = receive_data(connection);
broadcast_msg(connection.nickname, data);
}

}
catch (ConnectionEndException)
{
lock (_lock) client_list.Remove(connection);

Welcome in C#-World :) You chose a nice use-case for the start. So here are my remarks (in random order):

1) Why do you need the count-Variable? It's the same as client_list.Count so no need to have this information twice. And BTW, the count-Variable needs to be protected by the _lock, too.

2) Instead of having all these static-Methods and a "Data-Only-Class" Connection I would wrap logic in a ChatConnection-Class. socket and nickname would then be private members. For broadcasting you still need methods that loop over all connections (a ChatConnectionManager-Class would be useful here).

while (true)
{
    var socket = serverSocket.Accept();
    // Factory-Method which internally creates a ChatConnection, starts the thread etc.
    var chatConnection = ChatConnection.Start(socket);
    _chatConnections.Add(chatConnection);
}

3) The naming style is not very C#-Standard. Instead of using underscores use PascalCase (HandleConnection instead of handle_connection)

4) I would replace foreach (byte b in buffer)... with the shorter Linq-Method found = buffer.Contains((byte)EoM). Basically every foreach-Loop can be replaced with a Linq-Oneliner.

5) Instead of

string data = "";
...
data += ...

I would use a StringBuilder which is more efficient

StringBuilder sb = new StringBuilder();
...
sb.Append(...)
data = sb.ToString();

6) bool authenticate(string nick) can also use Linq for shortness:

return client_list.Exists(c => c.nickname == nick || string.IsNullOrEmpty(nick));

7) Instead of a custom string protocol with separators why not use a little bit JSON to serialize/deserialize the data. Much easier, supports Unicode, extensible ... .

8) In receive_data you receive data in a buffer and then search for the EoM-Marker. The problem here is that the data coming out from a socket is a stream and the data sent by the client could be split in multiple chunks by the TCP-Layer if necessary. So the client could send the messages like this:

M1-EoM ... M2-EoM ... M3-EoM

and this data could arrive on the server-side in the Receive-Method as:

M1-Eom-M2a ... M2b-EoM-M3a ... M3b-EoM

where message M2 and M3 are split up in two chunks. In this case the existing EoM-Handling wouldn't be sufficient because you mix up M2 and M3 data. A usual way to fix this is to send the length of the data before the data instead of having an EoM-Marker. Then you first read the data-length and then call Receive until you got all the data.

9) Concerning MainWindow.xaml.cs:

This works but you have all the logic in the View (MainWindow) which is considered bad style. Please google "xaml mvvm" which will give you a lot of interesting reads about the Model-View-ViewModel pattern. Looks scary at first but is really a good way to separate UI and logic and is greatly supported in XAML through the Binding-Mechanism.

You also duplicated code in MainWindow (see receive_data and send_data) which is usually a sign for code that should be shared between client and server.

The connected_ui- and disconnected_ui-Methods should be reduced to one method with passing a bool isConnected parameter.

StackExchange Code Review Q#162210, answer score: 4