TCP/IP multithread chat

Question

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<Connection> client_list = new List<Connection>();
 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("<SERVER ON>");
 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 = "<OK>" + 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);
 broadcast_status(connection.nickname, broadcastStatus.disconnect);
 connection.socket.Shutdown(SocketShutdown.Both);
 connection.socket.Close();
 count--;
 }
 catch (Exception ex)
 {
 lock (_lock) client_list.Remove(connection);
 broadcast_status(connection.nickname, broadcastStatus.disconnect);
 connection.socket.Close();
 count--;
 }
 }
 else
 {
 //send error to the client 
 string error = "<ERROR>";
 send_data(connection, error);
 connection.socket.Close();
 }
 }
 static string receive_data(Connection connection)
 {
 byte[] buffer = new byte[1024];
 bool found = false;
 string data = "";
 while (found == false)
 {
 int byte_count = 0;
 byte_count += connection.socket.Receive(buffer, 0, buffer.Length, SocketFlags.None);
 if (byte_count > 0)
 {
 foreach (byte b in buffer)
 {
 if (b == (int)EoM)
 {
 found = true;
 break;
 }
 }
 }
 else
 {
 throw new ConnectionEndException(); //friendly disconnect exception
 }
 data += Encoding.ASCII.GetString(buffer, 0, byte_count);
 }
 return data.TrimEnd(EoM); // remove the EoM character
 }
 static void send_data(Connection connection, string data)
 {
 data += EoM; // add the EoM character
 byte[] buffer = Encoding.ASCII.GetBytes(data);
 connection.socket.Send(buffer, 0, buffer.Length, SocketFlags.None);
 }
 static bool authenticate(string nick)
 {
 foreach (Connection c in client_list)
 {
 if (nick == c.nickname || nick == "")
 {
 return false;
 }
 }
 return true;
 }
 static void broadcast_msg(string nick, string msg)
 {
 string data = "<MSG>" + separator + nick + separator + msg;
 lock (_lock)
 {
 foreach (Connection connection in client_list)
 {
 send_data(connection, data);
 }
 }
 Console.WriteLine(data);
 }
 static void broadcast_status(string nick, broadcastStatus status)
 {
 if (status == broadcastStatus.connect)
 {
 string data = "<STATUS>" + separator + nick + separator + "<CONNECTED>";
 if (client_list.Count > 0)
 {
 lock (_lock)
 {
 foreach (Connection connection in client_list)
 {
 send_data(connection, data);
 }
 }
 }
 Console.WriteLine(data);
 }
 if (status == broadcastStatus.disconnect)
 {
 string data = "<STATUS>" + separator + nick + separator + "<DISCONNECTED>";
 lock (_lock)
 {
 foreach (Connection connection in client_list)
 {
 send_data(connection, data);
 }
 }
 Console.WriteLine(data);
 }
 }
 static string client_list_to_string()
 {
 string data = "";
 lock (_lock)
 {
 foreach (Connection c in client_list)
 {
 data += c.nickname + clientSeparator;
 }
 }
 return data;
 }
}
public class Connection
{
 public Socket socket;
 public string nickname;
 public Connection()
 {
 socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
 }
}
public class ConnectionEndException : Exception
{
}

Client WPF: MainWindow.xaml.cs

/// <summary>
/// Interação lógica para MainWindow.xam
/// </summary>
public partial class MainWindow : Window
{
 const char EoM = (char)3; //End of Messege mark
 const char separator = (char)29; // main delimiter 
 const char clientSeparator = (char)31; //client delimiter
 Socket client;
 public MainWindow()
 {
 InitializeComponent();
 }
 private void bt_connect_Click(object sender, RoutedEventArgs e)
 {
 try
 {
 //connect to the socket
 client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
 client.Connect(IPAddress.Parse(txt_IP.Text), Int32.Parse(txt_Port.Text));
 //starting the connection thread
 Thread clientThread = new Thread(() => connectionThread());
 clientThread.IsBackground = true;
 clientThread.Start();
 }
 catch (Exception ex)
 {
 MessageBox.Show(ex.Message);
 }
 }
 private void bt_send_Click(object sender, RoutedEventArgs e)
 {
 send_data(txt_send.Text);
 txt_send.Text = "";
 }
 private void bt_disconnect_Click(object sender, RoutedEventArgs e)
 {
 client.Shutdown(SocketShutdown.Send); //send 0 byte signal to the server 
 }
 private void connectionThread()
 {
 List<string> client_list = new List<string>();
 try
 {
 //send NickName
 string nickName = this.Dispatcher.Invoke(() => txt_Nick.Text);
 send_data(nickName);
 //get server authenticate response and client list
 string response_and_client_list = receive_data();
 //format the server response 
 string[] formated = response_and_client_list.Split(separator);
 //get authenticate response
 string response = formated[0];
 //this method will throw an exception(InvalidNickNameException) if the nickname is already being used
 authenticate_server_response(response);
 //get client list
 string[] clients = formated[1].Split(new char[] { clientSeparator }, StringSplitOptions.RemoveEmptyEntries);
 //enable connected interface
 this.Dispatcher.Invoke(() => connected_ui());
 this.Dispatcher.Invoke(() => update_chat("<" + nickName + "> Connected!"));
 //add clients online to the list 
 foreach (string s in clients)
 {
 client_list.Add(s);
 }
 //Invoke UI (txtClientList) UPDATE
 this.Dispatcher.Invoke(() => update_client_list(client_list));
 //listen to server DATA
 while (true)
 {
 string data = receive_data();
 interpret_data(data, client_list);
 }
 }
 catch (InvalidNickNameException ex)
 {
 MessageBox.Show(ex.Message);
 }
 catch (ConnectionEndException) //friendly disconnect 
 {
 client.Close();
 this.Dispatcher.Invoke(() => disconnected_ui());
 }
 catch (Exception e)
 {
 MessageBox.Show(e.Message);
 }
 }
 private string receive_data()
 {
 byte[] buffer = new byte[1024];
 bool found = false;
 string data = "";
 while (found == false)
 {
 int byte_count = 0;
 byte_count += client.Receive(buffer, 0, buffer.Length, SocketFlags.None);
 if (byte_count > 0)
 {
 foreach (byte b in buffer)
 {
 if (b == (int)EoM)
 {
 found = true;
 break;
 }
 }
 }
 else
 {
 throw new ConnectionEndException(); //friendly disconnect 
 }
 data += Encoding.ASCII.GetString(buffer, 0, byte_count);
 }
 return data.TrimEnd(EoM);
 }
 private void send_data(string data)
 {
 data += EoM; //add the EoM character
 byte[] buffer = Encoding.ASCII.GetBytes(data);
 client.Send(buffer, 0, buffer.Length, SocketFlags.None);
 }
 private void authenticate_server_response(string response)
 {
 if (response == "<ERROR>")
 {
 throw new InvalidNickNameException();
 }
 }
 private void interpret_data(string data, List<string> client_list)
 {
 string[] formated_data = data.Split(separator);
 if (formated_data[0] == "<STATUS>") // RESPONSE FOR DISCONNECT/CONNECT
 {
 if (formated_data[2] == "<CONNECTED>")
 {
 client_list.Add(formated_data[1]);
 this.Dispatcher.Invoke(() => update_chat("<" + formated_data[1] + "> Connected!"));
 this.Dispatcher.Invoke(() => update_client_list(client_list));
 }
 else
 {
 this.Dispatcher.Invoke(() => update_chat("<" + formated_data[1] + "> Disconnected!"));
 client_list.Remove(formated_data[1]);
 this.Dispatcher.Invoke(() => update_client_list(client_list));
 }
 }
 else // <MSG> MSG FROM CLIENTS
 {
 string msg = formated_data[1] + ": " + formated_data[2];
 this.Dispatcher.Invoke(() => update_chat(msg));
 }
 }
 private void update_chat(string data)
 {
 txt_chat.Text += data + Environment.NewLine;
 }
 private void update_client_list(List<string> list)
 {
 txtClientList.Clear();
 foreach (string s in list)
 {
 txtClientList.Text += s + '\n';
 }
 }
 private void connected_ui()
 {
 txtClientList.Text = "";
 txt_chat.Text = "";
 txt_chat.IsEnabled = false;
 txt_IP.IsEnabled = false;
 txt_Nick.IsEnabled = false;
 txt_Port.IsEnabled = false;
 txt_chat.IsEnabled = true;
 txt_send.IsEnabled = true;
 txtClientList.IsEnabled = true;
 bt_send.IsEnabled = true;
 bt_connect.IsEnabled = false;
 bt_disconnect.IsEnabled = true;
 }
 private void disconnected_ui()
 {
 txtClientList.Text = "";
 txt_chat.Text = "";
 txt_chat.IsEnabled = false;
 txt_send.IsEnabled = false;
 txtClientList.IsEnabled = false;
 bt_send.IsEnabled = false;
 bt_disconnect.IsEnabled = false;
 bt_connect.IsEnabled = true;
 txt_IP.IsEnabled = true;
 txt_Nick.IsEnabled = true;
 txt_Port.IsEnabled = true;
 }
}
class ConnectionEndException : Exception
{
}
class InvalidNickNameException : Exception
{
 public override string Message
 {
 get
 {
 return "Erro: Nick já existente"; // this nicks already exists
 }
 }
}

Answer 1

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.

• t3chb0t: camelCase vs PascalCase :-)
• Paulo Afonso Pinhero: Thanks for your time!! what about this connection per thread thing? some people told me that its usually bad idea to to use raw threads, also how should i implement this data-length thing,do i need a separator between the length and the data?
• urmeli: Concerning the "connection per thread": it's true, in a real-life application it's generally considered a bad idea to do this. Threads are quite expensive OS-Objects and you can't start millions of them without running out of resources. Besides that these threads most of the time do nothing because they are blocked in the Socket.Receive-Method.
• urmeli: ... you can get rid of threads when you use asynchronous APIs (e. g. Socket.ReceiveAsync). There you don't wait for data to arrive but the OS notifies you when data arrives. C# greatly supports this with the keywords async and await.
• urmeli: Concerning the "data-length thing": no, you don't need a separator. You first send a Int32 (4 bytes). On the receiver side you first read 4 bytes, convert them to an Int32 and then read on using the length.