6
\$\begingroup\$

Well, it's 2 applications really. Each one does its own stuff on the front-end. It's the networking that I'm most concerned about.

The Server starts listening for connections on the specified port as soon as it starts up. When clients connect, it accepts, serves and boots them in turn (or rather that's the idea but I think there's a flaw in what I've got here).

The server is multithreaded to handle requests on new threads so as to avoid locking its UI while it works.

public void StartListening()
{
    TcpListener Svr = null;

    Int32 Port = 13000;
    IPAddress LocalAddress = IPAddress.Parse("127.0.0.1");

    Svr = new TcpListener(LocalAddress, Port);

    Svr.Start();

    Byte[] Buffer = new Byte[1024];

    Task.Run(() =>
    {
        while (true)
        {
            TcpClient Client = Svr.AcceptTcpClient();
            NetworkStream Stream = Client.GetStream();

            byte[] Response = HandleClientData(Stream, Buffer, Client);

            Stream.Write(Response, 0, Response.Length);

            Client.Close();
        }
    });
}

The function that reads and works with the data HandleClientData(NetworkStream, byte[], TcpClient) looks like this:

private byte[] HandleClientData(NetworkStream stream, Byte[] buffer, TcpClient client)
{
    int i;

    byte[] value = null;

    while ((i = stream.Read(buffer, 0, buffer.Length)) != 0)
    {
        string ClientData = Encoding.ASCII.GetString(buffer, 0, i);

        // do stuff with the client data and send a response
        value = ...;
    }
return value;
}

The Client is much the same, except its not listening for connections. It's only job is to send requests to the server and display the result that the server sends back:

public class _TcpClient
{
    public const string Server = "127.0.0.1";
    public const int Port = 13000;

    public string SendData(string data)
    {
        try
        {
            string Response = String.Empty;
            using (TcpClient Client = new TcpClient())
            {
                Client.Connect(Server, Port);

                using (NetworkStream Stream = new NetworkStream(Client.Client))
                {
                    Stream.Write(Encoding.ASCII.GetBytes(data), 0, 256);

                    int ByteRead = 0;
                    byte[] Buffer = new Byte[256];

                    while (ByteRead > 0)
                    {
                        ByteRead = Stream.Read(Buffer, 0, 256);
                        Response += Encoding.ASCII.GetString(Buffer, 0, ByteRead);
                    }
                }
            }

            return Response;
        }
        catch (Exception ex)
        {
            return String.Format("ERR,{0}", ex.Message);
        }
    }
}

To keep things simple, all comms between client and server send and receive delimited strings which are read by each component and handled in a particular way for each.

Is this a fairly decent implementation or a client/server program? How can I improve this or make it more functional?

\$\endgroup\$
  • \$\begingroup\$ How do you stop the server? \$\endgroup\$ – JanDotNet May 30 '17 at 12:04
  • 1
    \$\begingroup\$ @JanDotNet I close it like any application. Click the X on the top-right. It doesn't run as a service or behind the scenes or anything. It has a specific purpose this time which precludes that sort of thing. \$\endgroup\$ – Ortund May 30 '17 at 12:08
  • 3
    \$\begingroup\$ Small tip, if you would compile this program in release mode with code analysis on. It will warn you about the disposables you create but do not dispose. It might also show other problems. \$\endgroup\$ – Roy T. May 30 '17 at 13:21
5
\$\begingroup\$

Naming

  • Usually, local variables start with a small letter.

StartListening

  • If the method is called multiple times, it will throw an exception that the endpoint is already in use. Not very robust.
  • Task.Run(action) uses a thread from the thread pool. Those threads are usually pooled for short running actions. TaskFactory.StartNew(action, TaskCreationOptions.LongRunning) creates a new thread to run the action in. That is more appropriated for long running tasks.
  • You should use try / catch or using to ensure that the client will be disposed, even if an error occurs.
  • You should provide a way to shut down the server - closing the application is IMHO not the right way.

HandleClientData

  • If the incoming data size is bigger than 1024, only the last chunk is received. You would probably add the chunk to the string: clientData += Encoding.ASCII.GetString(buffer, 0, i);
\$\endgroup\$
2
\$\begingroup\$

Possible improvements:

  1. Do not hard-code settings. Store IP, port, buffer size, etc. in configuration file and load those settings from it. (Not that important for sandbox projects)
  2. You should not reopen NetworkStream every time you send a message. Instead your TcpClient class should open stream once and only close it when the client is closed/disposed. SendData should re-use existing connection.
  3. Always sending 256 bytes is clearly not the best approach. It bloats the traffic if message is smaller and cuts the message if it is larger. A simple solution is to prepend size to the messages you send.
  4. Use var when variable type is obvious.
  5. Consider using ...Async methods (e.g. AcceptTcpClientAsync instead of AcceptTcpClient) to improve responsiveness of your apps.
\$\endgroup\$
  • \$\begingroup\$ With regard to settings, would app.config be suitable? \$\endgroup\$ – Ortund May 30 '17 at 21:18
  • \$\begingroup\$ @Ortund, sure, it's good enough to store a couple of values. For complex settings you might want to have something more... advanced. \$\endgroup\$ – Nikita B May 31 '17 at 8:31
1
\$\begingroup\$

An infinite while loop is probably inefficient, you should look into using a TCPListener and Socket so that your code only gets called while there is data to process, and hook into this using the Async methods.

P.S. Please don't edit your post with changes as it can make existing answers obsolete. Post a new question called (v2) and put a link in this question instead

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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