patterncsharpMinor
TCP async socket server client communication
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
```
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
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
1: Use
This declares intent and keeps other code from accidentally modifying an invariant. Plus, the runtime can sometimes perform certain optimizations knowing a field is
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:
StateObject:
IAsyncSocketListener:
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 :
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.