3
\$\begingroup\$

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
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

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.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ camelCase vs PascalCase :-) \$\endgroup\$ – t3chb0t May 1 '17 at 16:32
  • \$\begingroup\$ 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? \$\endgroup\$ – wants-to-learn May 2 '17 at 1:40
  • \$\begingroup\$ 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. \$\endgroup\$ – urmeli May 2 '17 at 18:18
  • \$\begingroup\$ ... 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. \$\endgroup\$ – urmeli May 2 '17 at 18:25
  • \$\begingroup\$ 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. \$\endgroup\$ – urmeli May 2 '17 at 18:33

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.