13
\$\begingroup\$

I've only been coding C# a few weeks and was just hoping for a little constructive criticism of a socket server I've been working on:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Net.Sockets;
using System.Net;

namespace NetworkCommunication
{
    public class TCPSocketServer : IDisposable
    {
        private int portNumber;
        private int connectionsLimit;
        private Socket connectionSocket;
        private List<StateObject> connectedClients = new List<StateObject>();

        public event SocketConnectedHandler ClientConnected;
        public delegate void SocketConnectedHandler(TCPSocketServer socketServer, SocketConnectArgs e);
        public event SocketMessageReceivedHandler MessageReceived;
        public delegate void SocketMessageReceivedHandler(TCPSocketServer socketServer, SocketMessageReceivedArgs e);
        public event SocketClosedHandler ClientDisconnected;
        public delegate void SocketClosedHandler(TCPSocketServer socketServer, SocketEventArgs e);

        #region Constructors
        public TCPSocketServer(int PortNumber) : this(PortNumber, 0) { }

        public TCPSocketServer(int PortNumber, int ConnectionsLimit)
        {
            this.portNumber = PortNumber;
            this.connectionsLimit = ConnectionsLimit;
            startListening();
        }
        #endregion

        #region Send Messages
        public void SendMessage(string MessageToSend, int clientID)
        {
            try
            {
                byte[] byData = System.Text.Encoding.UTF8.GetBytes(MessageToSend + "\0");

                foreach (StateObject client in connectedClients)
                {
                    if (clientID == client.id)
                    {
                        // Send message on correct client
                        if (client.socket.Connected)
                        {
                            client.socket.Send(byData);
                        }
                        break;
                    }
                }
            }
            catch (SocketException) { }
        }

        public void SendMessage(byte[] MessageToSend, int clientID)
        {
            try
            {
                foreach (StateObject client in connectedClients)
                {
                    if (clientID == client.id)
                    {
                        // Send message on correct client
                        if (client.socket.Connected)
                        {
                            client.socket.Send(MessageToSend);
                        }
                        break;
                    }
                }
            }
            catch (SocketException) { }
        }
        #endregion

        #region Connection and Listening
        private void startListening()
        {
            try
            {
                // Create listening socket
                connectionSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
                connectionSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true);
                IPEndPoint ipLocal = new IPEndPoint(IPAddress.Any, this.portNumber);
                // Bind to local IP Address
                connectionSocket.Bind(ipLocal);
                // Start Listening
                connectionSocket.Listen(1000);
                // Creat callback to handle client connections
                connectionSocket.BeginAccept(new AsyncCallback(onClientConnect), null);
            }
            catch (SocketException) { }
        }

        private void onClientConnect(IAsyncResult asyn)
        {
            try
            {
                // Create a new StateObject to hold the connected client
                StateObject connectedClient = new StateObject();
                connectedClient.socket = connectionSocket.EndAccept(asyn);
                if (connectedClients.Count == 0)
                {
                    connectedClient.id = 1;
                }
                else
                {
                    connectedClient.id = connectedClients[connectedClients.Count - 1].id + 1;
                }
                connectedClients.Add(connectedClient);

                // Check against limit
                if (connectedClients.Count > connectionsLimit)
                {
                    // No connection event is sent so close socket silently
                    closeSocketSilent(connectedClient.id);
                    return;
                }

                // Dispatch Event
                if (ClientConnected != null)
                {
                    SocketConnectArgs args = new SocketConnectArgs();
                    args.ConnectedIP = IPAddress.Parse(((IPEndPoint)connectedClient.socket.RemoteEndPoint).Address.ToString());
                    args.clientID = connectedClient.id;
                    ClientConnected(this, args);
                }

                // Release connectionSocket to keep listening if limit is not reached
                connectionSocket.BeginAccept(new AsyncCallback(onClientConnect), null);

                // Allow connected client to receive data and designate a callback method
                connectedClient.socket.BeginReceive(connectedClient.buffer, 0, StateObject.BufferSize, 0, new AsyncCallback(onReceivedClientData), connectedClient);
            }
            catch (SocketException) { }
            catch (ObjectDisposedException) { }
        }

        private void onReceivedClientData(IAsyncResult asyn)
        {
            String content = String.Empty;
            // Receive stateobject of the client that sent data
            StateObject dataSender = (StateObject)asyn.AsyncState;

            try
            {
                // Complete aysnc receive method and read data length
                int bytesRead = dataSender.socket.EndReceive(asyn);

                if (bytesRead > 0)
                {
                    // More data could be sent so append data received so far
                    dataSender.sb.Append(Encoding.UTF8.GetString(dataSender.buffer, 0, bytesRead));
                    content = dataSender.sb.ToString();
                    if ((content.Length > 0) || (content.IndexOf("") > -1))
                    {
                        String formattedMessage = String.Empty;
                        formattedMessage += content.Replace("\0", "");

                        // Dispatch Event
                        if (MessageReceived != null)
                        {
                            SocketMessageReceivedArgs args = new SocketMessageReceivedArgs();
                            args.MessageContent = formattedMessage;
                            args.clientID = dataSender.id;
                            MessageReceived(this, args);
                        }

                        dataSender.sb.Length = 0;
                    }
                    try
                    {
                        dataSender.socket.BeginReceive(dataSender.buffer, 0, StateObject.BufferSize, 0, new AsyncCallback(this.onReceivedClientData), dataSender);
                    }
                    catch (SocketException) { }
                }
                else
                {
                    closeSocket(dataSender.id);
                }
            }
            catch (SocketException) { }
            catch (ObjectDisposedException) { }
        }
        #endregion

        #region Socket Closing
        public void closeSocket(int SocketID)
        {
            foreach (StateObject client in connectedClients.ToList())
            {
                try
                {
                    if (SocketID == client.id)
                    {
                        client.socket.Close();
                        client.socket.Dispose();

                        // Dispatch Event
                        if (ClientDisconnected != null)
                        {
                            SocketEventArgs args = new SocketEventArgs();
                            args.clientID = client.id;
                            ClientDisconnected(this, args);
                        }

                        connectedClients.Remove(client);
                        break;
                    }
                }
                catch (SocketException) { }
            }
        }

        // This does not dispatch an event, this task is to be used when rejecting connections past the limit.
        // No connection event is sent so no disconnection event should be sent.
        private void closeSocketSilent(int SocketID)
        {
            foreach (StateObject client in connectedClients.ToList())
            {
                if (SocketID == client.id)
                {
                    try
                    {
                        client.socket.Close();
                        client.socket.Dispose();
                        connectedClients.Remove(client);
                        break;
                    }
                    catch (SocketException) { }
                }
            }
        }

        public void closeAllSockets()
        {
            foreach (StateObject client in connectedClients.ToList())
            {
                closeSocket(client.id);
            }
        }
        #endregion

        public void Dispose()
        {
            this.ClientConnected = null;
            this.ClientDisconnected = null;
            this.MessageReceived = null;

            connectionSocket.Close();
        }
    }
}

Here's a new updated version with the help I was given kindly below. I've added error handling and moved some parts around:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Net.Sockets;
using System.Net;

namespace NetworkCommunication
{
    public class TCPSocketServer : IDisposable
    {
        private const int MaxLengthOfPendingConnectionsQueue = 1000;

        private int portNumber;
        private int connectionsLimit;
        private Socket connectionSocket;
        private Dictionary<int, StateObject> connectedClients = new Dictionary<int, StateObject>();

        public event SocketConnectedHandler ClientConnected;
        public delegate void SocketConnectedHandler(TCPSocketServer socketServer, SocketConnectArgs e);
        public event SocketMessageReceivedHandler MessageReceived;
        public delegate void SocketMessageReceivedHandler(TCPSocketServer socketServer, SocketMessageReceivedArgs e);
        public event SocketClosedHandler ClientDisconnected;
        public delegate void SocketClosedHandler(TCPSocketServer socketServer, SocketEventArgs e);

        #region Constructor
        public TCPSocketServer(int PortNumber, int ConnectionsLimit = 0)
        {
            // Validate Port Number
            if (PortNumber > 0 && PortNumber < 65536)
            {
                this.portNumber = PortNumber;
            }
            else
            {
                throw new InvalidPortNumberException("Ports number must be in the 1-65535 range. Note: 256 and bellow are normally reserved.");
            }

            this.connectionsLimit = ConnectionsLimit;
            startListening();
        }
        #endregion

        private StateObject GetClient(int clientId)
        {
            StateObject client;
            if (!connectedClients.TryGetValue(clientId, out client))
            {
                return null;
            }
            return client;
        }

        #region Send Messages
        public void SendMessage(string MessageToSend, int clientID)
        {
            byte[] data = System.Text.Encoding.UTF8.GetBytes(MessageToSend + "\0");
            SendMessage(data, clientID);
        }

        public void SendMessage(byte[] MessageToSend, int clientID)
        {
            StateObject client = GetClient(clientID);
            if (client != null)
            {
                try
                {
                    if (client.socket.Connected)
                    {
                        client.socket.Send(MessageToSend);
                    }
                }
                catch (SocketException)
                {
                    // Close socket
                    closeSocket(clientID);
                }
            }
        }
        #endregion

        #region Connection and Listening
        private void startListening()
        {
            try
            {
                // Create listening socket
                connectionSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
                connectionSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true);
                IPEndPoint ipLocal = new IPEndPoint(IPAddress.Any, this.portNumber);
                // Bind to local IP Address
                connectionSocket.Bind(ipLocal);
                // Start Listening
                connectionSocket.Listen(MaxLengthOfPendingConnectionsQueue);
                // Create callback to handle client connections
                connectionSocket.BeginAccept(new AsyncCallback(onClientConnect), null);
            }
            catch (SocketException) 
            {
                throw new SocketCannotListenException("Cannot listen on this socket. Fatal Error");
            }
        }

        private void onClientConnect(IAsyncResult asyn)
        {
            // Create a new StateObject to hold the connected client
            StateObject connectedClient = new StateObject();
            try
            {
                connectedClient.socket = connectionSocket.EndAccept(asyn);
                connectedClient.id = !connectedClients.Any() ? 1 : connectedClients.Keys.Max() + 1;

                connectedClients.Add(connectedClient.id, connectedClient);

                // Check against limit
                if (connectedClients.Count > connectionsLimit && connectionsLimit != 0)
                {
                    // No connection event is sent so close connection quietly
                    closeSocket(connectedClient.id, true);
                    return;
                }

                // Allow connected client to receive data and designate a callback method
                connectedClient.socket.BeginReceive(connectedClient.buffer, 0, StateObject.BufferSize, 0, new AsyncCallback(onReceivedClientData), connectedClient);
            }
            catch (Exception)
            {
                closeSocket(connectedClient.id, true);
            }
            finally
            {
                // Perfomed here to not get any exceptions on the main socket caught up in client connection errors.
                ReleaseConnectionSocket();
            }

            // Dispatch Event at the end as any errors in socket dispatch silent diconnections
            if (ClientConnected != null)
            {
                SocketConnectArgs args = new SocketConnectArgs()
                {
                    ConnectedIP = IPAddress.Parse(((IPEndPoint)connectedClient.socket.RemoteEndPoint).Address.ToString()),
                clientID = connectedClient.id
                };
                ClientConnected(this, args);
            }
        }

        private void ReleaseConnectionSocket()
        {
            try
            {
                // Release connectionSocket to keep listening
                connectionSocket.BeginAccept(new AsyncCallback(onClientConnect), null);
            }
            catch (SocketException)
            {
                throw new SocketCannotListenException("Cannot listen on the main socket. Fatal Error");
            }
        }

        private void onReceivedClientData(IAsyncResult asyn)
        {
            // Receive stateobject of the client that sent data
            StateObject dataSender = (StateObject)asyn.AsyncState;

            try
            {
                // Complete aysnc receive method and read data length
                int bytesRead = dataSender.socket.EndReceive(asyn);

                if (bytesRead > 0)
                {
                    // More data could be sent so append data received so far
                    dataSender.sb.Append(Encoding.UTF8.GetString(dataSender.buffer, 0, bytesRead));
                    String content = dataSender.sb.ToString();
                    if (!string.IsNullOrEmpty(content))
                    {
                        String formattedMessage = content.Replace("\0", "");

                        // Dispatch Event
                        if (MessageReceived != null)
                        {
                            SocketMessageReceivedArgs args = new SocketMessageReceivedArgs() {
                            MessageContent = formattedMessage,
                            clientID = dataSender.id
                        };
                            MessageReceived(this, args);
                        }

                        dataSender.sb.Clear();
                    }
                    try
                    {
                        dataSender.socket.BeginReceive(dataSender.buffer, 0, StateObject.BufferSize, 0, new AsyncCallback(this.onReceivedClientData), dataSender);
                    }
                    catch (SocketException) {
                        closeSocket(dataSender.id);
                    }
                }
                else
                {
                    closeSocket(dataSender.id);
                }
            }
            catch (SocketException ex) {
                // Socket closed at other end
                if (ex.ErrorCode == 10054)
                {
                    closeSocket(dataSender.id);
                }
                else
                {
                    closeSocket(dataSender.id);
                }
            }
        }
        #endregion

        #region Socket Closing
        public void closeSocket(int SocketID)
        {
            closeSocket(SocketID, false);
        }

        // This does not dispatch an event, this task is to be used when rejecting connections past the limit.
        // No connection event is sent so no disconnection event should be sent.
        private void closeSocket(int SocketId, bool silent)
        {
            StateObject client = GetClient(SocketId);
            if (client == null)
            {
                return;
            }
            try
            {
                client.socket.Close();
                client.socket.Dispose();

                if (!silent)
                {
                    // Dispatch event
                    if (ClientDisconnected != null)
                    {
                        SocketEventArgs args = new SocketEventArgs()
                        {
                            clientID = client.id
                        };
                        ClientDisconnected(this, args);
                    }
                }
            }
            catch (SocketException)
            {
                // Socket is being removed anyway.
            }
            finally
            {
                connectedClients.Remove(client.id);
            }
        }

        public void closeAllSockets()
        {
            var keys = connectedClients.Keys;
            foreach (int key in keys)
            {
                var client = connectedClients[key];
                closeSocket(client.id);
            }
        }
        #endregion

        public void Dispose()
        {
            this.ClientConnected = null;
            this.ClientDisconnected = null;
            this.MessageReceived = null;

            connectionSocket.Close();
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ I'm interested in your async implementation, can you share the client code and eventArgs code? \$\endgroup\$
    – Smith
    Apr 29, 2015 at 21:24
  • \$\begingroup\$ When EndReceive returns 0 and you have no more data to send, perhaps you should execute Shutdown as part of the graceful termination of the socket connection (msdn.microsoft.com/en-us/library/windows/desktop/…). \$\endgroup\$
    – Pooven
    Sep 4, 2015 at 7:57

4 Answers 4

13
\$\begingroup\$

I will not comment on the actual TCP functionality itself.
I am not competent for that.

Stopping broadcast - on purpose?

When you are broadcasting to the clients in SendMessage(), it seems like you stop broadcasting if you get problems reaching one of the clients. Is this intended?
I would move the try/catch inside the foreach, and around the if. Then, in the catch, I would either explicitly break; out of the foreach or leave a comment like // Don't care, ignore and proceed.

Empty try-catches

As @ChaosPandion suggested, I would not leave empty try-catches.
I would either handle them, try to refactor the code as not to throw them, or leave a comment with a quick explanation.

Succinct code

If you are using C#4.0, I would write the default values like this:

    #region Constructors
    // This behaves the same as the other version with two constructors.
    public TCPSocketServer(int PortNumber, int ConnectionsLimit = 0)
    {

Parameters - case

You use camelCase for local variables, which agrees to what I think is the de facto convention.
It seems you use PascalCase for parameters. Shouldn't that be camelCase instead, same as local variables?

Hungarian notation

I would avoid Hungarian notation.
I have no real suggestion for byte[] byData though. Maybe just data? encodedData?

Code duplication

Code duplication is recipe for disaster. You will update one of the code sections and not the other, and so on...

So I would suggest that the first SendMessage just reuses the second:

public void SendMessage(string MessageToSend, int clientID)
{
    byte[] data = System.Text.Encoding.UTF8.GetBytes(MessageToSend + "\0");
    SendMessage(data, clientID);
}

Magic numbers

I would extract this number to a constant somewhere, and name it appropriately. SocketListenTimeoutMillisec?

// Start Listening
connectionSocket.Listen(1000);

Black magic

People got dunked for much less than this. :)

content.IndexOf("") > -1

Does it do anything, considering that the string has Length > 0? Context:

content = dataSender.sb.ToString();
if ((content.Length > 0) || (content.IndexOf("") > -1))

Succint code

String formattedMessage = String.Empty;
formattedMessage += content.Replace("\0", "");

could be

String formattedMessage = content.Replace("\0", "");

Var declarations closer to usage

String content = String.Empty;

could move next to content = dataSender.sb.ToString();, and even be integrated into that line:

string content = dataSender.sb.ToString();

Use StringBuilder better

// instead of dataSender.sb.Length = 0;
dataSender.sb.Clear();

useless .ToList()

    foreach (StateObject client in connectedClients.ToList())

Linq makes code succint

foreach (StateObject client in connectedClients.ToList()) {
    if (SocketID == client.id) {

can be replaced with

StateObject client = connectedClients.Where( conClient => conClient.id == SocketID );
if( client != null ) {

Object constructor

// Parentheses are optional, if empty.
SocketMessageReceivedArgs args = new SocketMessageReceivedArgs() {
    MessageContent = formattedMessage,
    clientID = dataSender.id
};

And same for other similar property assignments immediately after the respective constructor.


My proposal of cleaner code

using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Text;

namespace NetworkCommunication
{
    class StateObject { }
    class SocketConnectArgs { }
    class SocketMessageReceivedArgs { }
    class SocketEventArgs { }

    public class TCPSocketServer : IDisposable
    {
        // Or whatever the name.
        const int MaxLengthOfPendingConnectionsQueue = 1000;

        private int portNumber;
        private int connectionsLimit;
        private Socket connectionSocket;
        private Dictionary<int, StateObject> connectedClients = new Dictionary<int, StateObject>();

        public event SocketConnectedHandler ClientConnected;
        public delegate void SocketConnectedHandler(TCPSocketServer socketServer, SocketConnectArgs e);
        public event SocketMessageReceivedHandler MessageReceived;
        public delegate void SocketMessageReceivedHandler(TCPSocketServer socketServer, SocketMessageReceivedArgs e);
        public event SocketClosedHandler ClientDisconnected;
        public delegate void SocketClosedHandler(TCPSocketServer socketServer, SocketEventArgs e);

        #region Constructors
        public TCPSocketServer(int portNumber, int connectionsLimit = 0) {
            this.portNumber = portNumber;
            this.connectionsLimit = connectionsLimit;
            startListening();
        }
        #endregion

        private StateObject GetClient(int clientId) {
            StateObject client;
            if(!connectedClients.TryGetValue(clientId, out client)) {
                return null;
            }
            return client;
        }

        #region Send Messages
        public void SendMessage(string messageToSend, int clientID) {
            byte[] data = System.Text.Encoding.UTF8.GetBytes(messageToSend + "\0");
            SendMessage(data, clientID);
        }

        public void SendMessage(byte[] messageToSend, int clientID) {
            StateObject client = GetClient(clientID);
            if (client != null) {
                try {
                    if (client.socket.Connected) {
                        client.socket.Send(messageToSend);
                    }
                } catch (SocketException) {
                    // TODO: sending failed; disconnect from client, or?
                }
            }
        }
        #endregion

        #region Connection and Listening
        private void startListening() {
            try {
                // Create listening socket
                connectionSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
                connectionSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true);
                IPEndPoint ipLocal = new IPEndPoint(IPAddress.Any, this.portNumber);
                // Bind to local IP Address
                connectionSocket.Bind(ipLocal);
                // Start Listening
                connectionSocket.Listen(MaxLengthOfPendingConnectionsQueue);
                // Creat callback to handle client connections
                connectionSocket.BeginAccept(new AsyncCallback(onClientConnect), null);
            } catch (SocketException) {
                // TODO: if we fail to start listening, is it even ok to continue?
                // Consider that some of the bootstrapping actions might not even have been done.
                // Thus execution will likely crash on next step.
            }
        }

        private void onClientConnect(IAsyncResult asyn) {
            try {
                // Create a new StateObject to hold the connected client
                StateObject connectedClient = new StateObject() {
                    socket = connectionSocket.EndAccept(asyn),
                    id = !connectedClients.Any() ? 1 : connectedClients.Keys.Max() + 1
                };

                connectedClients.Add(connectedClient);

                // TODO: consider if we can instead do this at the beginning of the method.
                // Check against limit
                if (connectedClients.Count > connectionsLimit) {
                    // No connection event is sent so close socket silently
                    closeSocket(connectedClient.id, true);
                    return;
                }

                // Dispatch Event
                if (ClientConnected != null) {
                    SocketConnectArgs args = new SocketConnectArgs() {
                        ConnectedIP = IPAddress.Parse(((IPEndPoint)connectedClient.socket.RemoteEndPoint).Address.ToString()),
                        clientID = connectedClient.id
                    };
                    ClientConnected(this, args);
                }

                // Release connectionSocket to keep listening if limit is not reached
                connectionSocket.BeginAccept(new AsyncCallback(onClientConnect), null);

                // Allow connected client to receive data and designate a callback method
                connectedClient.socket.BeginReceive(connectedClient.buffer, 0, StateObject.BufferSize, 0, new AsyncCallback(onReceivedClientData), connectedClient);
            } catch (SocketException) {
                // TODO: should we closeSocketSilent()? Or?
            } catch (ObjectDisposedException) {
                // TODO: should we closeSocketSilent()? Or?
            }
        }

        private void onReceivedClientData(IAsyncResult asyn) {
            // Receive stateobject of the client that sent data
            StateObject dataSender = (StateObject)asyn.AsyncState;

            try {
                // Complete aysnc receive method and read data length
                int bytesRead = dataSender.socket.EndReceive(asyn);

                if (bytesRead > 0) {
                    // More data could be sent so append data received so far
                    dataSender.sb.Append(Encoding.UTF8.GetString(dataSender.buffer, 0, bytesRead));
                    if ( dataSender.sb.Length != 0
                        && MessageReceived != null
                        ) {
                        // TODO: is it possible that multiple messages are in the sb?
                        // Consider whether it's necessary to replace with newline.
                        dataSender.sb.Replace("\0", null); // Removes them.

                        // Dispatch Event
                        SocketMessageReceivedArgs args = new SocketMessageReceivedArgs();
                        args.MessageContent = dataSender.sb.ToString();
                        args.clientID = dataSender.id;
                        MessageReceived(this, args);

                        dataSender.sb.Clear();
                    }
                    try {
                        dataSender.socket.BeginReceive(dataSender.buffer, 0, StateObject.BufferSize, 0, new AsyncCallback(this.onReceivedClientData), dataSender);
                    } catch (SocketException) { }
                } else {
                    closeSocket(dataSender.id);
                }
            } catch (SocketException) {
                // TODO: should we closeSocketSilent()? Or?
            } catch (ObjectDisposedException) {
                // TODO: should we closeSocketSilent()? Or?
            }
        }
        #endregion

        #region Socket Closing
        public void closeSocket(int socketID) {
            closeSocket(socketID, false);
        }

        /// <param name="silent">Whether to skip dispatching the disconnection event. Used to cancel the bootstrapping of the client-server connection.</param>
        private void closeSocket(int socketID, bool silent) {
            StateObject client = GetClient(socketID);
            if (client == null) {
                return;
            }
            try {
                client.socket.Close();
                client.socket.Dispose();

                if(!silent) {
                    // Dispatch Event
                    if (ClientDisconnected != null) {
                        SocketEventArgs args = new SocketEventArgs();
                        args.clientID = client.id;
                        ClientDisconnected(this, args);
                    }
                }
                // Moved to finnaly block: connectedClients.Remove(client.id);
            } catch (SocketException) {
                // Don't care. Or?
            } finally {
                connectedClients.Remove(client.id);
            }
        }

        public void closeAllSockets() {
            var keys = connectedClients.Keys;
            foreach( int key in keys ) {
                var client = connectedClients[key];
                closeSocket(client.id);
            }
        }
        #endregion

        public void Dispose() {
            ClientConnected = null;
            ClientDisconnected = null;
            MessageReceived = null;

            connectionSocket.Close();
        }
    }
}

PS: you owe me "a beer". :)
(This was fun to do, though!)

edit: camelCase for parameters; reduced code duplication; Dictionary<int, StateObject> instead of List; more object constructors.
edit2: StringBuilder methods as suggested by @pstrjds.

\$\endgroup\$
8
  • \$\begingroup\$ Nice job, there is still duplicate code in the close and closesilent, I feel like that could be handled with either adding a default parameter bool silent = false to the close method, or adding a private method that they both call and then the param just needs to be checked whether to raise the event and what to do with an exception. \$\endgroup\$
    – pstrjds
    Nov 8, 2011 at 17:28
  • \$\begingroup\$ Thanks. I've improved the answer a bit, using your comment and the great approaches from other answers. (I wonder if it would be polite, or constructive, to make the answer community wiki?) \$\endgroup\$
    – ANeves
    Nov 9, 2011 at 12:17
  • \$\begingroup\$ 2 things - 1. I think you should call dataSender.sb.Clear() instead of setting length to 0, clear is a convenience method that is equivalent but I feel is clearer and makes reading the code easier later. 2. The StringBuilder class has a replace on it. It seems like the if statement that is checking the ToString() result (called content) could just check the SB and also check if there is an event to raise and only do the replace in the SB if it needs to raise the event. Then clearing the SB can be outside the if since it should be trivial to clear an empty string builder. \$\endgroup\$
    – pstrjds
    Nov 9, 2011 at 13:00
  • \$\begingroup\$ @BenGale - is dataSender.sb a StringBuilder? @pstrjds - without the code for that class I do not know if it is, and would be assuming; thus I left the correction as a comment. \$\endgroup\$
    – ANeves
    Nov 9, 2011 at 13:59
  • 1
    \$\begingroup\$ sure, I was making the assumption as it is called sb and both Append and ToString are being called on it as well as setting the Length property, based on that I figure it is most likely a string builder and thus things like Replace and others could be called on it. \$\endgroup\$
    – pstrjds
    Nov 9, 2011 at 15:10
6
\$\begingroup\$

Your event handling is a bit non-standard. Rather than:

public event SocketConnectedHandler ClientConnected;
public delegate void SocketConnectedHandler(TCPSocketServer socketServer, SocketConnectArgs e);

it would be more usual (and hence less surprising) to have:

public event EventHandler<SocketConnectArgs> ClientConnected;

The downside of the more usual approach, of course, is that you have to cast the object sender to TCPSocketServer, so there's a tradeoff.


The way you're using connectedClients indicates that you've got the type wrong. In almost all accesses you're iterating through it checking the elements to see whether their id matches a value. That suggests that it should be an IDictionary<int, StateObject>, or that you should pass the StateObject around rather than the id. You might also need to synchronise access to it - I haven't nailed down whether it's possible for it to be accessed by more than one thread.

\$\endgroup\$
3
  • \$\begingroup\$ I was just commenting on the container choice myself. It seemed to me that the way that id is assigned makes it equivalent to the items index in the List + 1, so you should be able to get the item with just connectedClients[id-1]. \$\endgroup\$
    – pstrjds
    Nov 8, 2011 at 14:08
  • \$\begingroup\$ @pstrjds, no, because stuff gets deleted from the list. Although this does mean that it's possible for ids to be reused, which is probably a bug. \$\endgroup\$ Nov 8, 2011 at 14:11
  • \$\begingroup\$ that was my bad, I didn't notice the delete in the close call. I assumed they weren't being removed and missed that little line, and it would could lead to confusion if the ID was reused. \$\endgroup\$
    – pstrjds
    Nov 8, 2011 at 14:15
3
\$\begingroup\$

My biggest concern is your excessive use of empty try statements. You should either handle any SocketException that may be thrown or let them propagate. As it stands I would not consider this production ready code.

\$\endgroup\$
2
  • \$\begingroup\$ I have more thoughts which I will add at a convenient time. \$\endgroup\$ Oct 11, 2011 at 17:00
  • \$\begingroup\$ I'd love to hear them. I think handling errors is one of my weakest areas. \$\endgroup\$
    – Ben Gale
    Oct 13, 2011 at 10:10
3
\$\begingroup\$

I just noticed this question is a couple weeks old, but never-the-less I will add my two cents. I would have your SendMessage overload that takes a string perform the byte conversion that it is doing, but instead of each SendMessage duplicating the code that is walking a list looking for an open connection, I would have the SendMessage with the string just call into the SendMessage that takes a byte[]

public void SendMessage(string MessageToSend, int clientID)
{
    byte[] byData = System.Text.Encoding.UTF8.GetBytes(MessageToSend + "\0");
    SendMessage(byData, clientID);
}

public void SendMessage(byte[] MessageToSend, int clientID)
{
    try
    {
        foreach (StateObject client in connectedClients)
        {
            if (clientID == client.id)
            {
                // Send message on correct client
                if (client.socket.Connected)
                {
                    client.socket.Send(MessageToSend);
                }
                break;
            }
        }
    }
    catch (SocketException) { }
}

The code seems a little odd where the new connections are coming in. If you have reached the limit of connections you are going to handle you close the socket that you created and just return. It seems like you could check the limit first and return without doing any of the work to create the state object, add to the list, then check the limit and remove from the list.

Lastly, consider not looping through all connected clients in each method that takes a clientID. You could make your container a Dictionary<int, StateObject> so you can just use the clientID to get the object without walking the list.

\$\endgroup\$
1
  • \$\begingroup\$ Very nice suggestion, on the dictionary. \$\endgroup\$
    – ANeves
    Nov 8, 2011 at 17:00

Not the answer you're looking for? Browse other questions tagged or ask your own question.