HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

TCP async socket server client communication

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
asynccommunicationtcpclientserversocket

Problem

I develop my first async TCP socket server and client program in c# and would like to review the first parts of it. I like to get some information’s about smelly code that I missed and what I could improve in the program. I know it's a little bit much code, but it would awesome if you help me to improve my skills!

I have to classes AsyncSocketListener and AsyncClient. Both use events to tell the main program if messages sent or received. Here are the classes:

AsyncSocketListener

```
static class AsyncSocketListener
{
private static ushort port = 8080;
private static ushort limit = 250;

private static ManualResetEvent mre = new ManualResetEvent(false);
private static Dictionary clients = new Dictionary();

#region Event handler
public delegate void MessageReceivedHandler(int id, String msg);
public static event MessageReceivedHandler MessageReceived;

public delegate void MessageSubmittedHandler(int id, bool close);
public static event MessageSubmittedHandler MessageSubmitted;
#endregion

/ Starts the AsyncSocketListener /
public static void StartListening()
{
IPHostEntry host = Dns.GetHostEntry(String.Empty);
IPAddress ip = host.AddressList[3];
IPEndPoint socket = new IPEndPoint(ip, port);

try
{
using (Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
{
listener.Bind(socket);
listener.Listen(limit);
while (true)
{
mre.Reset();
listener.BeginAccept(new AsyncCallback(OnClientConnect), listener);
mre.WaitOne();
}
}
}
catch (SocketException)
{
// TODO:
}
}

/ Gets a socket from the clients dictionary by his Id. /
private static StateObject getClient(int id)
{
StateObject state = new

Solution

0: Keep class members private unless there is a darn good reason to expose them. And then, if you have to, use properties.

1: Use readonly on class declarations which are considered unmodifiable after construction. Example:

public readonly ManualResetEvent connected = new ManualResetEvent(false);


This declares intent and keeps other code from accidentally modifying an invariant. Plus, the runtime can sometimes perform certain optimizations knowing a field is readonly.

2: Develop to interfaces. This allows for decoupling of implementation plus ease of testing/mocking.

So this being said, here's how I refactored it:


IStateObject:

public interface IStateObject
{
    int BufferSize { get; }

    int Id { get; }

    bool Close { get; set; }

    byte[] Buffer { get; }

    Socket Listener { get; }

    string Text { get; }

    void Append(string text);

    void Reset();
}



StateObject:

public sealed class StateObject : IStateObject
{
    /* Contains the state information. */

    private const int Buffer_Size = 1024;
    private readonly byte[] buffer = new byte[Buffer_Size];
    private readonly Socket listener;
    private readonly int id;
    private StringBuilder sb;

    public StateObject(Socket listener, int id = -1)
    {
        this.listener = listener;
        this.id = id;
        this.Close = false;
        this.Reset();
    }

    public int Id
    {
        get
        {
            return this.id;
        }
    }

    public bool Close { get; set; }

    public int BufferSize
    {
        get
        {
            return Buffer_Size;
        }
    }

    public byte[] Buffer
    {
        get
        {
            return this.buffer;
        }
    }

    public Socket Listener
    {
        get
        {
            return this.listener;
        }
    }

    public string Text
    {
        get
        {
            return this.sb.ToString();
        }
    }

    public void Append(string text)
    {
        this.sb.Append(text);
    }

    public void Reset()
    {
        this.sb = new StringBuilder();
    }
}



IAsyncSocketListener:

public interface IAsyncSocketListener : IDisposable
{
    event MessageReceivedHandler MessageReceived;

    event MessageSubmittedHandler MessageSubmitted;

    void StartListening();

    bool IsConnected(int id);

    void OnClientConnect(IAsyncResult result);

    void ReceiveCallback(IAsyncResult result);

    void Send(int id, string msg, bool close);

    void Close(int id);
}



AsyncSocketListener:

```
public delegate void MessageReceivedHandler(int id, string msg);
public delegate void MessageSubmittedHandler(int id, bool close);

public sealed class AsyncSocketListener : IAsyncSocketListener
{
private const ushort Port = 8080;
private const ushort Limit = 250;

private static readonly IAsyncSocketListener instance = new AsyncSocketListener();

private readonly ManualResetEvent mre = new ManualResetEvent(false);
private readonly IDictionary clients = new Dictionary();

public event MessageReceivedHandler MessageReceived;

public event MessageSubmittedHandler MessageSubmitted;

private AsyncSocketListener()
{
}

public static IAsyncSocketListener Instance
{
get
{
return instance;
}
}

/ Starts the AsyncSocketListener /
public void StartListening()
{
var host = Dns.GetHostEntry(string.Empty);
var ip = host.AddressList[3];
var endpoint = new IPEndPoint(ip, Port);

try
{
using (var listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
{
listener.Bind(endpoint);
listener.Listen(Limit);
while (true)
{
this.mre.Reset();
listener.BeginAccept(this.OnClientConnect, listener);
this.mre.WaitOne();
}
}
}
catch (SocketException)
{
// TODO:
}
}

/ Gets a socket from the clients dictionary by his Id. /
private IStateObject GetClient(int id)
{
IStateObject state;

return this.clients.TryGetValue(id, out state) ? state : null;
}

/ Checks if the socket is connected. /
public bool IsConnected(int id)
{
var state = this.GetClient(id);

return !(state.Listener.Poll(1000, SelectMode.SelectRead) && state.Listener.Available == 0);
}

/* Add a socket to the clients dictionary. Lock clients temporary to handle multiple access.
ReceiveCallback raise a event, after the message receive complete. /
#region Receive data
public void OnClientConnect(IAsyncResult result)
{
this.mre.Set();

try
{
IStateObject state;

lock (this.clients)
{
var id = !this.clients.Any() ? 1 :

Code Snippets

public readonly ManualResetEvent connected = new ManualResetEvent(false);
public interface IStateObject
{
    int BufferSize { get; }

    int Id { get; }

    bool Close { get; set; }

    byte[] Buffer { get; }

    Socket Listener { get; }

    string Text { get; }

    void Append(string text);

    void Reset();
}
public sealed class StateObject : IStateObject
{
    /* Contains the state information. */

    private const int Buffer_Size = 1024;
    private readonly byte[] buffer = new byte[Buffer_Size];
    private readonly Socket listener;
    private readonly int id;
    private StringBuilder sb;

    public StateObject(Socket listener, int id = -1)
    {
        this.listener = listener;
        this.id = id;
        this.Close = false;
        this.Reset();
    }

    public int Id
    {
        get
        {
            return this.id;
        }
    }

    public bool Close { get; set; }

    public int BufferSize
    {
        get
        {
            return Buffer_Size;
        }
    }

    public byte[] Buffer
    {
        get
        {
            return this.buffer;
        }
    }

    public Socket Listener
    {
        get
        {
            return this.listener;
        }
    }

    public string Text
    {
        get
        {
            return this.sb.ToString();
        }
    }

    public void Append(string text)
    {
        this.sb.Append(text);
    }

    public void Reset()
    {
        this.sb = new StringBuilder();
    }
}
public interface IAsyncSocketListener : IDisposable
{
    event MessageReceivedHandler MessageReceived;

    event MessageSubmittedHandler MessageSubmitted;

    void StartListening();

    bool IsConnected(int id);

    void OnClientConnect(IAsyncResult result);

    void ReceiveCallback(IAsyncResult result);

    void Send(int id, string msg, bool close);

    void Close(int id);
}
public delegate void MessageReceivedHandler(int id, string msg);
public delegate void MessageSubmittedHandler(int id, bool close);

public sealed class AsyncSocketListener : IAsyncSocketListener
{
    private const ushort Port = 8080;
    private const ushort Limit = 250;

    private static readonly IAsyncSocketListener instance = new AsyncSocketListener();

    private readonly ManualResetEvent mre = new ManualResetEvent(false);
    private readonly IDictionary<int, IStateObject> clients = new Dictionary<int, IStateObject>();

    public event MessageReceivedHandler MessageReceived;

    public event MessageSubmittedHandler MessageSubmitted;

    private AsyncSocketListener()
    {
    }

    public static IAsyncSocketListener Instance
    {
        get
        {
            return instance;
        }
    }

    /* Starts the AsyncSocketListener */
    public void StartListening()
    {
        var host = Dns.GetHostEntry(string.Empty);
        var ip = host.AddressList[3];
        var endpoint = new IPEndPoint(ip, Port);

        try
        {
            using (var listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
            {
                listener.Bind(endpoint);
                listener.Listen(Limit);
                while (true)
                {
                    this.mre.Reset();
                    listener.BeginAccept(this.OnClientConnect, listener);
                    this.mre.WaitOne();
                }
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    /* Gets a socket from the clients dictionary by his Id. */
    private IStateObject GetClient(int id)
    {
        IStateObject state;

        return this.clients.TryGetValue(id, out state) ? state : null;
    }

    /* Checks if the socket is connected. */
    public bool IsConnected(int id)
    {
        var state = this.GetClient(id);

        return !(state.Listener.Poll(1000, SelectMode.SelectRead) && state.Listener.Available == 0);
    }

    /* Add a socket to the clients dictionary. Lock clients temporary to handle multiple access.
     * ReceiveCallback raise a event, after the message receive complete. */
    #region Receive data
    public void OnClientConnect(IAsyncResult result)
    {
        this.mre.Set();

        try
        {
            IStateObject state;

            lock (this.clients)
            {
                var id = !this.clients.Any() ? 1 : this.clients.Keys.Max() + 1;

                state = new StateObject(((Socket)result.AsyncState).EndAccept(result), id);
                this.clients.Add(id, state);
                Console.WriteLine("Client connected. Get Id " + id);
            }

            state.Listener.BeginReceive(state.Buffer, 0, state.BufferSize, SocketFlags.None, this.ReceiveCallback, state);
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public void ReceiveCallback(IAsyncResult result)
   

Context

StackExchange Code Review Q#24758, answer score: 9

Revisions (0)

No revisions yet.