5
\$\begingroup\$

Here is my simple chat program. I'm wondering whether it has an acceptable design (both object oriented design and network design).

If you want to run it, I can provide the solution folder.

Server

public class Server
{
    private TcpListener _tcpListener;
    private readonly int _packetSize = 64;

    private int _clientCount;
    private readonly int _maxClientCount;
    private Dictionary<IPAddress, Client> _clients;
    private readonly object _token = new object();

    public Action<byte[], Client> OnDataReceive;

    public bool Running { get; set; }

    public int Port
    {
        get { return ((IPEndPoint)_tcpListener.Server.LocalEndPoint).Port; }
    }

    public Dictionary<IPAddress, Client> Clients
    {
        get { return _clients; }
    }

    public Server(int port, int maxClientCount)
    {
        _clientCount = 0;
        _maxClientCount = maxClientCount;
        _clients = new Dictionary<IPAddress, Client>(_maxClientCount);

        try
        {
            _tcpListener = new TcpListener(IPAddress.Any, port);
        }
        catch (Exception e)
        {
            CommandLine.Write(e.Message);
        }
    }

    public void StartListen()
    {
        try
        {
            Running = true;
            _tcpListener.Start();
            CommandLine.Write("Started listening at port " + Port + ".");
        }
        catch (Exception e)
        {
            CommandLine.Write(e.Message);
        }

        while (Running)
        {
            lock (_token)
            {
                if (_clientCount >= _maxClientCount) { continue; }
            }

            var newClient = _tcpListener.AcceptTcpClient();
            AddClient(newClient);
        }
    }

    public void StopListen()
    {
        try
        {
            Running = false;
            _tcpListener.Stop();
            CommandLine.Write("Stopped listening at port " + Port + ".");
        }
        catch (Exception e)
        {
            CommandLine.Write(e.Message);
        }
    }

    public void Send(Client client, string data)
    {
        if (client == null || !client.Connected) return;

        var msg = new Message(data);

        try
        {
            client.Stream.Write(msg.Data, 0, msg.Data.Length);
        }
        catch (Exception e)
        {
            CommandLine.Write(e.Message);
        }
    }

    public void SendAll(string data)
    {
        foreach (var entry in _clients) { Send(entry.Value, data); }
    }

    private void AddClient(TcpClient newClient)
    {
        if (newClient == null) return;

        var client = new Client(newClient);
        _clients.Add(client.IP, client);
        IncreaseClientCount();

        var clientThread = new Thread(HandleClient) { IsBackground = true };
        clientThread.Start(client);

        CommandLine.Write("A new client connected. Client count is " + _clientCount + ".");
    }

    private void RemoveClient(Client client)
    {
        if (client == null) return;

        _clients.Remove(client.IP);
        DecreaseClientCount();

        client.Close();
    }

    private void HandleClient(object newClient)
    {
        var client = (Client)newClient;
        var currentMessage = new List<byte>();

        while (true)
        {
            var readMessage = new byte[_packetSize];
            int readMessageSize;

            try
            {
                readMessageSize = client.Stream.Read(readMessage, 0, _packetSize);
            }
            catch (Exception e)
            {
                CommandLine.Write(e.Message);
                break;
            }

            if (readMessageSize <= 0)
            {
                CommandLine.Write("The client [" + client.IP + "] has closed the connection.");
                break;
            }

            foreach (var b in readMessage)
            {
                if (b == 0) break;

                if (b == 4)
                {
                    OnDataReceive(currentMessage.ToArray(), client);
                    currentMessage.Clear();
                }
                else
                {
                    currentMessage.Add(b);
                }
            }
        }

        CommandLine.Write("Communication ended with client [" + client.IP + "].");
        RemoveClient(client);
    }

    private void IncreaseClientCount()
    {
        lock (_token) { _clientCount++; }
    }

    private void DecreaseClientCount()
    {
        lock (_token) { _clientCount--; }
    }
}

Client

public class Client
{
    private TcpClient _client;
    private readonly int _packetSize = 64;

    public NetworkStream Stream
    {
        get { return _client.GetStream(); }
    }

    public IPAddress IP
    {
        get { return ((IPEndPoint)_client.Client.RemoteEndPoint).Address; }
    }

    public bool Connected
    {
        get { return _client.Connected; }
    }

    public Client(TcpClient client)
    {
        _client = client;
    }

    public void Send(string data)
    {
        var msg = new Message(data);

        try
        {
            Stream.Write(msg.Data, 0, msg.Data.Length);
        }
        catch (Exception e)
        {
            CommandLine.Write(e.Message);
        }
    }

    public void Receive()
    {
        var currentMessage = new List<byte>();

        while (true)
        {
            var readMessage = new byte[_packetSize];
            int readMessageSize;

            try
            {
                readMessageSize = Stream.Read(readMessage, 0, _packetSize);
            }
            catch (Exception e)
            {
                CommandLine.Write(e.Message);
                break;
            }

            if (readMessageSize <= 0) break;

            foreach (var b in readMessage)
            {
                if (b == 0) break;

                if (b == 4)
                {
                    CommandLine.Write("[SRV] : " + new ASCIIEncoding().GetString(currentMessage.ToArray()));
                    currentMessage.Clear();
                }
                else
                {
                    currentMessage.Add(b);
                }
            }
        }
    }

    public void Close()
    {
        try
        {
            _client.Close();
            _client = null;
        }
        catch (Exception e)
        {
            CommandLine.Write(e.Message);
        }
    }
}

Main

public class Program
{
    private static Server _server;
    private static Client _client;

    public static void Main()
    {
        while (true)
        {
            string input = Console.ReadLine();

            switch (input)
            {
                case "-srv":
                    {
                        if (_client != null) return;

                        Console.Title = "Server";

                        _server = new Server(15150, 3);
                        _server.OnDataReceive += OnReceive;
                        new Thread(_server.StartListen).Start();
                    }
                    break;
                case "-clients":
                    {
                        if (_server == null) return;

                        int counter = 0;

                        foreach (var entry in _server.Clients)
                        {
                            if (entry.Value == null) return;
                            CommandLine.Write(++counter + "- " + entry.Key + "\n");
                        }
                    }
                    break;
                case "-connect":
                    {
                        if (_server != null) return;

                        Console.Title = "Client";

                        var client = new TcpClient();
                        //ip adress below will be taken by user input after tests.
                        var serverEndPoint = new IPEndPoint(IPAddress.Parse("192.168.1.10"), 15150);
                        client.Connect(serverEndPoint);
                        _client = new Client(client);
                        new Thread(_client.Receive).Start();
                    }
                    break;
                default:
                    {
                        if (_client != null) // if user is a client.
                        {
                            _client.Send(input);
                        }
                        else if (_server != null) // if user is the server.
                        {
                            _server.SendAll(input);
                        }
                    }
                    break;
            }
        }
    }

    private static void OnReceive(byte[] data, Client client)
    {
        CommandLine.Write("[" + client.IP + "] : " + new ASCIIEncoding().GetString(data));
    }
}

Other classes

public class Message
{
    public byte[] Data { get; private set; }

    public Message(byte[] data)
    {
        var wrappedData = new LinkedList<byte>(data);
        wrappedData.AddLast(4);
        Data = wrappedData.ToArray();
    }

    public Message(string data)
    {
        data += (char)4;
        Data = new ASCIIEncoding().GetBytes(data);
    }
}

public static class CommandLine
{
    private static readonly object _token = new object();

    public static void Write(string text)
    {
        lock (_token)
        {
            Console.WriteLine(text);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ can you provide the link to the solution please? \$\endgroup\$ – user118432 Sep 24 '16 at 14:27
3
\$\begingroup\$

Grouping you member variables will make it easier for a reader. I prefer the order to be constants, set once, mutable.

If a value is known at compile time, it can be const instead of readonly. If a value is set and instance initialization and never changed, it should be readonly.

For example, all of the member variables in Server can be const or readonly. _clientCount can be replaced with _clients.Count. This has the added benefit of not needing to manually increment and decrement the count.


StartListen() has a spin loop in it. This is bad because it will cause the CPU to continually perform operations when it should just be silently waiting. You can use an EventWaitHandle pause the thread when no more clients can be added and trigger it when a client leaves. There are a number of specific implementations based on common patterns.


OnDataReceive assumes that an event listener has been registered, but there is no guarantee that one exists.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ In StartListen(), AcceptTcpClient() stops the loop when there is not any connection request. I don't understand why I have to use EventWaitHandle. Also, there are loops in Server's HandleClient() and Client's Receive(). Do they have to be treated in the same way? \$\endgroup\$ – Cem Feb 1 '15 at 22:06
  • 1
    \$\begingroup\$ Once you have maxClientCount clients, the while(Running) loop will keep looping as fast as it can. The only thing it will do is check if a new client can be accepted, then restart the loop. Over and over again. This is not productive work. The fact that it is running, means there is some other thread that can't be executing at that moment. An EventWaitHandle will allow you to stop the thread from executing any code until you explicitly tell it to wake up (when some client disconnects). As for the other loops, you should check to see if the socket reads block until there is data or not. \$\endgroup\$ – unholysampler Feb 2 '15 at 4:43
2
\$\begingroup\$

Adding to unholysampler's answer

The StartListen() method can lead to problems in its current state. If the call of _tcpListener.Start() throws an exception, Running will still be true.


You should use braces {} for single if statements to make your code less errorprone. If you decide to not use braces in these cases, you should stick to a choosen style. Right now yo are mixing them. Sometimes you use braces sometimes you don't.


The Server's constructor shouldn't use a try..catch like it does. Just printing the Exception.Message without rethrowing the exception can lead to serious problems. Any call to _tcpListener which isn't wrapped in a try..catch will cause the whole application to crash.


You can get rid of the IncreaseClientCount() and DecreaseClientCount() methods by using the Dictionary.Count property.


In its current state, your application will crash by calling AddClient() with a client which is already in the Dictionary.


| improve this answer | |
\$\endgroup\$

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.