TCP Socket Server

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 connectedClients = new List();

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;

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 connectedClients = new Dictionary();

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;
}

StackExchange Code Review Q#5306, answer score: 13