10
\$\begingroup\$

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<int, StateObject> clients = new Dictionary<int, StateObject>();

    #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 StateObject();
        return clients.TryGetValue(id, out state) ? state : null;
    }

    /* Checks if the socket is connected. */
    public static bool IsConnected(int id)
    {
        StateObject state = 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 static void OnClientConnect(IAsyncResult result)
    {
        mre.Set();
        StateObject state = new StateObject();
        try
        {
            lock (clients)
            {
                state.Id = !clients.Any() ? 1 : clients.Keys.Max() + 1;
                clients.Add(state.Id, state);
                Console.WriteLine("Client connected. Get Id " + state.Id);
            }
            state.listener = (Socket)result.AsyncState;
            state.listener = state.listener.EndAccept(result);
            state.listener.BeginReceive(state.buffer, 0, StateObject.BufferSize, SocketFlags.None, new AsyncCallback(ReceiveCallback), state);
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public static void ReceiveCallback(IAsyncResult result)
    {
        StateObject state = (StateObject)result.AsyncState;
        try
        {
            int receive = state.listener.EndReceive(result);
            if (receive > 0)
                state.sb.Append(Encoding.UTF8.GetString(state.buffer, 0, receive));
            if (receive == StateObject.BufferSize)
                state.listener.BeginReceive(state.buffer, 0, StateObject.BufferSize, SocketFlags.None, new AsyncCallback(ReceiveCallback), state);
            else
            {
                MessageReceived(state.Id, state.sb.ToString());
                state.sb = new StringBuilder();
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }
    #endregion

    /* Send(int id, String msg, bool close) use bool to close the connection after the message sent. */
    #region Send data
    public static void Send(int id, String msg, bool close)
    {
        StateObject state = getClient(id);
        if (state == null)
            throw new Exception("Client does not exist.");
        if (!IsConnected(state.Id))
            throw new Exception("Destination socket is not connected.");
        try
        {
            byte[] send = Encoding.UTF8.GetBytes(msg);
            state.Close = close;
            state.listener.BeginSend(send, 0, send.Length, SocketFlags.None, new AsyncCallback(SendCallback), state);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ArgumentException)
        {
            // TODO:
        }
    }

    private static void SendCallback(IAsyncResult result)
    {
        StateObject state = (StateObject)result.AsyncState;
        try
        {
            state.listener.EndSend(result);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ObjectDisposedException)
        {
            // TODO:
        }
        finally
        {
            MessageSubmitted(state.Id, state.Close);
        }
    }
    #endregion

    public static void Close(int id)
    {
        StateObject state = getClient(id);
        if (state == null)
            throw new Exception("Client does not exist.");
        try
        {
            state.listener.Shutdown(SocketShutdown.Both);
            state.listener.Close();
        }
        catch (SocketException)
        {
            // TODO:
        }
        finally
        {
            lock (clients)
            {
                clients.Remove(state.Id);
                Console.WriteLine("Client disconnected with Id {0}", state.Id);
            }
        }
    }
}

AsyncClient

class AsyncClient : IDisposable
{
    private const ushort port = 8080;

    private Socket listener = null;
    private bool close = false;

    public ManualResetEvent connected = new ManualResetEvent(false);
    public ManualResetEvent sent = new ManualResetEvent(false);
    public ManualResetEvent received = new ManualResetEvent(false);

    #region Event handler
    public delegate void ConnectedHandler(AsyncClient a);
    public static event ConnectedHandler Connected;

    public delegate void MessageReceivedHandler(AsyncClient a, String msg);
    public static event MessageReceivedHandler MessageReceived;

    public delegate void MessageSubmittedHandler(AsyncClient a, bool close);
    public static event MessageSubmittedHandler MessageSubmitted;
    #endregion

    /* Starts the AsyncClient */
    public AsyncClient()
    {
    }

    public void StartClient()
    {
        IPHostEntry host = Dns.GetHostEntry(String.Empty);
        IPAddress ip = host.AddressList[3];
        IPEndPoint socket = new IPEndPoint(ip, port);

        try
        {
            this.listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
            this.listener.BeginConnect(socket, new AsyncCallback(OnConnectCallback), listener);
            connected.WaitOne();
            Connected(this);
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public bool IsConnected()
    {
        return !(this.listener.Poll(1000, SelectMode.SelectRead) && this.listener.Available == 0);
    }

    private void OnConnectCallback(IAsyncResult result)
    {
        Socket server = (Socket)result.AsyncState;
        try
        {
            server.EndConnect(result);
            connected.Set();
        }
        catch (SocketException)
        {
        }
    }

    #region Receive data
    public void Receive()
    {
        StateObject state = new StateObject();
        state.listener = this.listener;
        state.listener.BeginReceive(state.buffer, 0, StateObject.BufferSize, SocketFlags.None, new AsyncCallback(ReceiveCallback), state);
    }

    private void ReceiveCallback(IAsyncResult result)
    {
        StateObject state = (StateObject)result.AsyncState;

        int receive = state.listener.EndReceive(result);
        if (receive > 0)
            state.sb.Append(Encoding.UTF8.GetString(state.buffer, 0, receive));
        if (receive == StateObject.BufferSize)
            state.listener.BeginReceive(state.buffer, 0, StateObject.BufferSize, SocketFlags.None, new AsyncCallback(ReceiveCallback), state);
        else
        {
            MessageReceived(this, state.sb.ToString());
            state.sb = new StringBuilder();
            received.Set();
        }
    }
    #endregion

    #region Send data
    public void Send(String msg, bool close)
    {
        if (!IsConnected())
            throw new Exception("Destination socket is not connected.");
        byte[] response = Encoding.UTF8.GetBytes(msg);
        this.close = close;
        this.listener.BeginSend(response, 0, response.Length, SocketFlags.None, new AsyncCallback(SendCallback), this.listener);
    }

    private void SendCallback(IAsyncResult result)
    {
        try
        {
            Socket resceiver = (Socket)result.AsyncState;
            resceiver.EndSend(result);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ObjectDisposedException)
        {
            // TODO;
        }
        MessageSubmitted(this, this.close);
        sent.Set();
    }
    #endregion

    private void Close()
    {
        try
        {
            if (IsConnected())
            {
                this.listener.Shutdown(SocketShutdown.Both);
                this.listener.Close();
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public void Dispose()
    {
        connected.Close();
        sent.Close();
        received.Close();
        Close();
    }
}

I save the connection information in a third class called StateObject.

StateObject

class StateObject
{
    /* Contains the state information. */
    private int id;
    private bool close = false; // Used to close the socket after the message sent.

    public Socket listener = null;
    public const int BufferSize = 1024;
    public byte[] buffer = new byte[BufferSize];
    public StringBuilder sb = new StringBuilder();

    public StateObject() { }

    public int Id
    {
        get { return this.id; }
        set { this.id = value; }
    }

    public bool Close
    {
        get { return this.close; }
        set { this.close = value; }
    }
}

With the following code I test the server and client:

public Server()
{
    InitializeComponent();
    /* I use this to test my code on one machine. */
    new Thread(new ThreadStart(AsyncSocketListener.StartListening)).Start();
    AsyncSocketListener.MessageReceived += new AsyncSocketListener.MessageReceivedHandler(ClientMessageReceived);
    AsyncSocketListener.MessageSubmitted += new AsyncSocketListener.MessageSubmittedHandler(ServerMessageSubmitted);

    AsyncClient.Connected += new AsyncClient.ConnectedHandler(ConnectedToServer);
    AsyncClient.MessageReceived += new AsyncClient.MessageReceivedHandler(ServerMessageReceived);
    AsyncClient.MessageSubmitted += new AsyncClient.MessageSubmittedHandler(ClientMessageSubmitted);

    for (int i = 0; i < 10; i++)
    {
        AsyncClient client = new AsyncClient();
        Thread thread = new Thread(new ThreadStart(client.StartClient));
        thread.Name = "Client" + i;
        thread.Start();
    }
}

/* Code to handle the events from AsyncSocketListener and AsyncClient. */
#region Server Code
private static void ClientMessageReceived(int id, String msg)
{
    AsyncSocketListener.Send(id, msg.Replace("client", "server"), true);
    Console.WriteLine("Server get Message from client. {0} ", msg);
}

private static void ServerMessageSubmitted(int id, bool close)
{
    if (close)
        AsyncSocketListener.Close(id);
}
#endregion

#region Client code
private static void ConnectedToServer(AsyncClient a)
{
    a.Send("Hello, I'm the client.", false);
    a.sent.WaitOne();
    a.Receive();
    a.received.WaitOne();
}

private static void ServerMessageReceived(AsyncClient a, String msg)
{
    Console.WriteLine("Client get Message from server. {0} ", msg);
}

private static void ClientMessageSubmitted(AsyncClient a, bool close)
{
    if (close)
        a.Dispose();
}
#endregion

If the code is fine, I will use polymorphism for the AsyncSocketListener and AsyncClient because some parts are very similar. What do you think?

Edit:

At the moment the client could connect and send a message to the server. The server could reply on it. But it is not possible to exchange messages after this. I'm trying to fix this, but I can't find a solution. Any idea?

\$\endgroup\$
9
\$\begingroup\$

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<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)
    {
        var state = (IStateObject)result.AsyncState;

        try
        {
            var receive = state.Listener.EndReceive(result);

            if (receive > 0)
            {
                state.Append(Encoding.UTF8.GetString(state.Buffer, 0, receive));
            }

            if (receive == state.BufferSize)
            {
                state.Listener.BeginReceive(state.Buffer, 0, state.BufferSize, SocketFlags.None, this.ReceiveCallback, state);
            }
            else
            {
                var messageReceived = this.MessageReceived;

                if (messageReceived != null)
                {
                    messageReceived(state.Id, state.Text);
                }

                state.Reset();
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }
    #endregion

    /* Send(int id, String msg, bool close) use bool to close the connection after the message sent. */
    #region Send data
    public void Send(int id, string msg, bool close)
    {
        var state = this.GetClient(id);

        if (state == null)
        {
            throw new Exception("Client does not exist.");
        }

        if (!this.IsConnected(state.Id))
        {
            throw new Exception("Destination socket is not connected.");
        }

        try
        {
            var send = Encoding.UTF8.GetBytes(msg);

            state.Close = close;
            state.Listener.BeginSend(send, 0, send.Length, SocketFlags.None, this.SendCallback, state);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ArgumentException)
        {
            // TODO:
        }
    }

    private void SendCallback(IAsyncResult result)
    {
        var state = (IStateObject)result.AsyncState;

        try
        {
            state.Listener.EndSend(result);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ObjectDisposedException)
        {
            // TODO:
        }
        finally
        {
            var messageSubmitted = this.MessageSubmitted;

            if (messageSubmitted != null)
            {
                messageSubmitted(state.Id, state.Close);
            }
        }
    }
    #endregion

    public void Close(int id)
    {
        var state = this.GetClient(id);

        if (state == null)
        {
            throw new Exception("Client does not exist.");
        }

        try
        {
            state.Listener.Shutdown(SocketShutdown.Both);
            state.Listener.Close();
        }
        catch (SocketException)
        {
            // TODO:
        }
        finally
        {
            lock (this.clients)
            {
                this.clients.Remove(state.Id);
                Console.WriteLine("Client disconnected with Id {0}", state.Id);
            }
        }
    }

    public void Dispose()
    {
        foreach (var id in this.clients.Keys)
        {
            this.Close(id);
        }

        this.mre.Dispose();
    }
}

IAsyncClient:

public interface IAsyncClient : IDisposable
{
    event ConnectedHandler Connected;

    event ClientMessageReceivedHandler MessageReceived;

    event ClientMessageSubmittedHandler MessageSubmitted;

    void StartClient();

    bool IsConnected();

    void Receive();

    void Send(string msg, bool close);
}

AsyncClient:

public delegate void ConnectedHandler(IAsyncClient a);
public delegate void ClientMessageReceivedHandler(IAsyncClient a, string msg);
public delegate void ClientMessageSubmittedHandler(IAsyncClient a, bool close);

public sealed class AsyncClient : IAsyncClient
{
    private const ushort Port = 8080;

    private Socket listener;
    private bool close;

    private readonly ManualResetEvent connected = new ManualResetEvent(false);
    private readonly ManualResetEvent sent = new ManualResetEvent(false);
    private readonly ManualResetEvent received = new ManualResetEvent(false);

    public event ConnectedHandler Connected;

    public event ClientMessageReceivedHandler MessageReceived;

    public event ClientMessageSubmittedHandler MessageSubmitted;

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

        try
        {
            this.listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
            this.listener.BeginConnect(endpoint, this.OnConnectCallback, this.listener);
            this.connected.WaitOne();

            var connectedHandler = this.Connected;

            if (connectedHandler != null)
            {
                connectedHandler(this);
            }
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public bool IsConnected()
    {
        return !(this.listener.Poll(1000, SelectMode.SelectRead) && this.listener.Available == 0);
    }

    private void OnConnectCallback(IAsyncResult result)
    {
        var server = (Socket)result.AsyncState;

        try
        {
            server.EndConnect(result);
            this.connected.Set();
        }
        catch (SocketException)
        {
        }
    }

    #region Receive data
    public void Receive()
    {
        var state = new StateObject(this.listener);

        state.Listener.BeginReceive(state.Buffer, 0, state.BufferSize, SocketFlags.None, this.ReceiveCallback, state);
    }

    private void ReceiveCallback(IAsyncResult result)
    {
        var state = (IStateObject)result.AsyncState;
        var receive = state.Listener.EndReceive(result);

        if (receive > 0)
        {
            state.Append(Encoding.UTF8.GetString(state.Buffer, 0, receive));
        }

        if (receive == state.BufferSize)
        {
            state.Listener.BeginReceive(state.Buffer, 0, state.BufferSize, SocketFlags.None, this.ReceiveCallback, state);
        }
        else
        {
            var messageReceived = this.MessageReceived;

            if (messageReceived != null)
            {
                messageReceived(this, state.Text);
            }

            state.Reset();
            this.received.Set();
        }
    }
    #endregion

    #region Send data
    public void Send(string msg, bool close)
    {
        if (!this.IsConnected())
        {
            throw new Exception("Destination socket is not connected.");
        }

        var response = Encoding.UTF8.GetBytes(msg);

        this.close = close;
        this.listener.BeginSend(response, 0, response.Length, SocketFlags.None, this.SendCallback, this.listener);
    }

    private void SendCallback(IAsyncResult result)
    {
        try
        {
            var resceiver = (Socket)result.AsyncState;

            resceiver.EndSend(result);
        }
        catch (SocketException)
        {
            // TODO:
        }
        catch (ObjectDisposedException)
        {
            // TODO;
        }

        var messageSubmitted = this.MessageSubmitted;

        if (messageSubmitted != null)
        {
            messageSubmitted(this, this.close);
        }

        this.sent.Set();
    }
    #endregion

    private void Close()
    {
        try
        {
            if (!this.IsConnected())
            {
                return;
            }

            this.listener.Shutdown(SocketShutdown.Both);
            this.listener.Close();
        }
        catch (SocketException)
        {
            // TODO:
        }
    }

    public void Dispose()
    {
        this.connected.Dispose();
        this.sent.Dispose();
        this.received.Dispose();
        this.Close();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks, looks great :) Is there a different if I use IStateObject state or StateObject state? Would you use polymorphism for AsyncSocketListener and AsyncClient? My Idea was to make one Interface for example for Send(), Start(), Close(), ... \$\endgroup\$ – Andre Hofmeister Apr 5 '13 at 17:50
  • \$\begingroup\$ The reason I factored out IStateObject is twofold: first, it could be that sometime in the future you want to alternate between implementations - for example StateObject vs. LoggingStateObject where the latter dumps client/server conversations to storage. The second reason is, as I alluded to, Mocking frameworks can create fake implementations of the interface for unit testing your other classes independently. \$\endgroup\$ – Jesse C. Slicer Apr 5 '13 at 17:54
  • \$\begingroup\$ You could define an additional interface, IConnection or something that each has to implement as those operations you mention are common between servers and clients. I don't see that as a bad thing at all. \$\endgroup\$ – Jesse C. Slicer Apr 5 '13 at 17:58
  • \$\begingroup\$ Ok. Thank you very much! What do you think about the events to get the received messages and so on..? Is there a better way? \$\endgroup\$ – Andre Hofmeister Apr 8 '13 at 6:11
  • \$\begingroup\$ It's pretty canonical the way you have it. \$\endgroup\$ – Jesse C. Slicer Apr 8 '13 at 12:21
5
\$\begingroup\$

White space and Casing is good.

Just a few things to think about:

  1. #region is not very well accepted. If you have to use #region, then you should look at moving the code out into a method or its own class.
  2. Use var instead of explicit declarations for obvious variables. This makes the code much easier to scan over.
  3. If you insist on comments to explain what the method is doing, us the C# /// syntax. This does a couple of things: it allows intellisense to pick up the description when using the library, and there are tools that can take the /// comments and create API help documentation.
  4. General practice in C# is to use either m_ or _ at the beginning of class variables. This eliminates the need to use this., which clutters up the code a little.
  5. Be consistent with your use of { } after if statements. It makes it eaasier to focus on the logic when it doesn't have to process changes in the formatting.
  6. In the line thread.Name = "Client" + i; I would make "Client" a constant.
  7. Separate concerns. For instance: Console.WriteLine does not belong in your server class.
  8. If the constructor doesn't do anything, get rid of it, it is only adding noise to your class.
  9. In my opinion, class variables should be initialized in the constructor, not on declaration.
  10. Use meaningful names for variables: a does not portray what the variable does. tcpClient on the other hand does. This makes your methods much easier to read.
  11. getClient should be GetClient. C# naming conventions

Most of these are minor, and don't affect the application, they just add that little bit to your code, and will make it easier to read and maintain in the future.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your reply! (1) I thought #region is just for structure and better reading. Why is it smelly? (3) The few commets are just for codereview :). (4) Nice to know. (7) Console.WriteLine is there just for testing. (9/10) Thanks, I will change. (11) Oh, yea you are right. What do you think about the events, to pass the messages to the main program?Is there a other, better way? \$\endgroup\$ – Andre Hofmeister Apr 5 '13 at 15:00
  • \$\begingroup\$ Code smell might of been the wrong term, but it is bad practice to use it. There is a post on Programmers.StackExchange that has a good discussion on it. \$\endgroup\$ – Jeff Vanzella Apr 5 '13 at 15:08
  • \$\begingroup\$ I don't see any #regions inside a method in the code, so I don't understand what do you mean by “pulling the code out into a method”. \$\endgroup\$ – svick Apr 5 '13 at 17:25
  • \$\begingroup\$ Changed bullet on #region. I hope its more clear now. \$\endgroup\$ – Jeff Vanzella Apr 5 '13 at 17:37

protected by Community Aug 16 '18 at 3:20

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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