5
\$\begingroup\$

I have a class that I use to communicate with some hardware equipment (a step motor to be precise):

public class HdCommunicator : IDisposable
{
    private TcpClient client;
    public HdCommunicator(TcpClient _client)
    {
        client = _client;

        //create and start the listener
        Thread t = new Thread(new ThreadStart(() => { Listen(); }));
        t.Start();
    }

    public void SendCommand(int data)
    {
        NetworkStream ns = client.GetStream();
        var msgToSend =
            Encoding.UTF8.GetBytes("mr " + data + ",1" + Environment.NewLine);
        ns.Write(msgToSend, 0, msgToSend.Length);

        ns.Flush();
    }

    private void Listen()
    {
        while (client.Connected)
        {
            try
            {
                NetworkStream ns = client.GetStream();

                byte[] bytes = new byte[1024];
                int bytesRead = ns.Read(bytes, 0, bytes.Length);

                string response = Encoding.ASCII.GetString(bytes, 0, bytesRead);

                //TODO raise event
                Console.WriteLine("Response: " + response);
            }
            catch (Exception ex)
            {
                if (!client.Connected)
                { /*ignore*/ }
                else
                {
                    Console.WriteLine("Listener error: " + ex.Message);
                }
            }
        }
    }

    public void Dispose()
    {
        client.Close();
    }
}

And this is how I currently use it:

    static int port = 503;
    static string host = "192.168.33.1";

    static void Main(string[] args)
    {
        HdCommunicator comm = new HdCommunicator(new TcpClient(host, port));

        string res = "";
        while (res != "e")
        {
            Console.WriteLine("Where to send: ");
            res = Console.ReadLine();

            if (res != "e")
                comm.SendCommand(int.Parse(res));
        }

        comm.Dispose();

        Console.WriteLine("Finished");
        Console.ReadLine();
    }

Am I utilizing the TCP Client correctly? I'm particularly concerned about the streams' usage and disposing of them. Am I doing it right?

Improve this question
\$\endgroup\$
0

6 Answers 6

Reset to default
5
\$\begingroup\$

I'm afraid that I don't know anything about TCP, but I did notice this. 

  if (!client.Connected)
  { /*ignore*/ }
  else
  {
     Console.WriteLine("Listener error: " + ex.Message);
  }

Instead of checking for the negative condition, check for the positive. It removes several lines of code and a comment. 

if(client.Connected)
{
    Console.WriteLine("Listener error: " + ex.Message);
}
Improve this answer
\$\endgroup\$
0
5
\$\begingroup\$

Use the using block

Since your HDCommunicator class implements IDisposable, you should wrap your code Inside a using block, like this :

static void Main(string[] args)
{
    using (var comm = new HdCommunicator(new TcpClient(host, port)))
    {
        string res = "";
        while (res != "e")
        {
            Console.WriteLine("Where to send: ");
            res = Console.ReadLine();

            if (res != "e")
                comm.SendCommand(int.Parse(res));
        }
    }

    Console.WriteLine("Finished");
    Console.ReadLine();
}

That way, you don't need to manually dispose of your object : the using block does it for you.

Use string.Empty

I find it cleaner to use string.Empty than "" :

string res = string.Empty

Be consistent

To ensure consistency, you should use braces even on conditional statements, like this :

if (res != "e")
{
    comm.SendCommand(int.Parse(res));
}

Naming

You should avoid using abreviations or shortened terms and rename comm to hdCommunicator or communicator. Abbreviated variable names are OK when their lifetime is very short, like inside lambda expressions.

Improve this answer
\$\endgroup\$
0
4
\$\begingroup\$ 
 Thread t = new Thread(new ThreadStart(() => { Listen(); }));
 t.Start();

Because Listen takes no parameters, the lambda is unnecessary. You can rewrite as:

new Thread(Listen).Start();
Improve this answer
\$\endgroup\$
0
4
\$\begingroup\$

Just one small comment, I notice you are using:

Thread t = new Thread(new ThreadStart(() => { Listen(); }));
t.Start();

Unless you explicitly need a thread, I believe the preference is to use (the higher level) Task instead, so something like:

new TaskFactory().StartNew(() => { Listen(); }));

Jon Skeet's post here explains the reasons for this a lot better than I could.

Quoted:

Thread is a lower-level concept: if you're directly starting a thread, you know it will be a separate thread, rather than executing on the thread pool etc.

Task is more than just an abstraction of "where to run some code" though - it's really just "the promise of a result in the future". So as some different examples:

Task.Delay doesn't need any actual CPU time; it's just like setting a timer to go off in the future A task returned by WebClient.DownloadStringTaskAsync won't take much CPU time locally; it's representing a result which is likely to spend most of its time in network latency or remote work (at the web server) A task returned by Task.Run() really is saying "I want you to execute this code separately"; the exact thread on which that code executes depends on a number of factors. Note that the Task abstraction is pivotal to the async support in C# 5.

In general, I'd recommend that you use the higher level abstraction wherever you can: in modern C# code you should rarely need to explicitly start your own thread.

Improve this answer
\$\endgroup\$
1
  • 3
    \$\begingroup\$ While generally I would agree, I feel that this is one of the rare cases Jon talks about where threads are relevant. A TCP listener is not the "the promise of a result in the future". It is not semantically a task, it doesn't have a defined end, and I don't see why it should waste a threadpool thread for the duration of the listener's existence, when we clearly know it's a while(true) loop. \$\endgroup\$
    – Rotem
    Nov 11, 2014 at 19:27
3
\$\begingroup\$

I'm going to argue against implementing IDisposable here. The class itself does not create any IDisposable objects - the only one happens to be passed into the constructor. It's a bit of a surprise to have an object used elsewhere be disposed unceremoniously without the caller's knowledge. So, don't implement that interface and let the caller wrap the new TcpClient(...) in a using construct.

However, in SendCommand and Listen, IDisposable objects pop into being with the call to GetStream(). The lifetime of that variable needs to be wrapped in using to ensure proper deterministic disposal.

public class HdCommunicator
{
    private TcpClient client;
    public HdCommunicator(TcpClient _client)
    {
        client = _client;

        //create and start the listener
        Thread t = new Thread(new ThreadStart(() => { Listen(); }));
        t.Start();
    }

    public void SendCommand(int data)
    {
        using (NetworkStream ns = client.GetStream())
        {
            var msgToSend =
                Encoding.UTF8.GetBytes("mr " + data + ",1" + Environment.NewLine);
            ns.Write(msgToSend, 0, msgToSend.Length);

            ns.Flush();
        }
    }

    private void Listen()
    {
        while (client.Connected)
        {
            try
            {
                byte[] bytes = new byte[1024];
                int bytesRead;

                using (NetworkStream ns = client.GetStream())
                {
                    bytesRead = ns.Read(bytes, 0, bytes.Length);
                }

                string response = Encoding.ASCII.GetString(bytes, 0, bytesRead);

                //TODO raise event
                Console.WriteLine("Response: " + response);
            }
            catch (Exception ex)
            {
                if (client.Connected)
                {
                    Console.WriteLine("Listener error: " + ex.Message);
                }
            }
        }
    }
}

Then

static int port = 503;
static string host = "192.168.33.1";

static void Main(string[] args)
{
    using (var tcpClient = new TcpClient(host, port))
    {
        HdCommunicator comm = new HdCommunicator(tcpClient);

        string res = "";
        while (res != "e")
        {
            Console.WriteLine("Where to send: ");
            res = Console.ReadLine();

            if (res != "e")
                comm.SendCommand(int.Parse(res));
        }
    }

    Console.WriteLine("Finished");
    Console.ReadLine();
}
Improve this answer
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Regarding IDisposable: how can you prevent HdCommunicator be initiated like HdCommunicator comm = new HdCommunicator(new TcpClient(host, port)); ? Wouldn't it be better if the constructor just takes the ip and port and creates the TcpClient itself ? \$\endgroup\$
    – Heslacher
    Nov 11, 2014 at 14:25
  • \$\begingroup\$ That's a good point. I suggested the using block because I assumed HdCommunicator.Dispose was actually disposing of the TCPClient object. \$\endgroup\$
    – user57782
    Nov 11, 2014 at 14:34
  • \$\begingroup\$ @Heslacher That way I will still have to use the Dispose method right? \$\endgroup\$
    – Yoav
    Nov 11, 2014 at 14:42
  • \$\begingroup\$ @Yoav, yes but than it is a valid to do so, because the communicator creates an IDisposable (the TcpClient). \$\endgroup\$
    – Heslacher
    Nov 11, 2014 at 14:45
  • 1
    \$\begingroup\$ @Heslacher that would be an all right design, yes. As long as the caller has no other need for the TcpClient other than for the HdCommunicator. Then, you can create it and Dispose of it nicely. \$\endgroup\$
    – Jesse C. Slicer
    Nov 11, 2014 at 14:49
1
\$\begingroup\$

I've made some changes to my motor class based on some of the answers I received here and some additional research and tests. I also changed it so it can be used in a form based application (WPF in my case) and this is what I got:

public class MotorManager : IDisposable
{
    public event EventHandler<string> OnMotorMessage;

    private bool showMoves;
    private bool showAllCommands;

    private string ip;
    private int port;

    TcpClient client;
    NetworkStream ns;

    public string MotorName { get; set; }

    public MotorManager(string _ip, int _port, string name, bool showMoves = false, bool showAllCommands = false)
    {
        this.showAllCommands = showAllCommands;
        this.showMoves = showMoves;

        this.MotorName = name;
        this.ip = _ip;
        this.port = _port;

        try
        {
            client = new TcpClient(ip, port);
            ns = client.GetStream();
            new TaskFactory().StartNew(() => Listen());
        }
        catch (Exception ex)
        {
            throw new Exception("Motor Initialization error: " + ex.Message);
        }
    }

    public void SendStringCommandToMotor(string command)
    {
        if (showAllCommands)
        {
            OnMotorMessage(this, command);
        }
        SendCommand(command);
    }

    private void SendCommand(string command)
    {
        try
        {
            if (!command.EndsWith(Environment.NewLine))
                command += Environment.NewLine;

            byte[] msg = Encoding.ASCII.GetBytes(command);

            if (client.Connected)
            {
                ns.Write(msg, 0, msg.Length);
            }
        }
        catch (Exception ex)
        {
            throw new Exception("Send motor command error: " + ex.Message);
        }
    }

    private void Listen()
    {
        while (client.Connected)
        {
            try
            {
                byte[] bytes = new byte[1024];
                int bytesRead = ns.Read(bytes, 0, bytes.Length);

                string response = Encoding.ASCII.GetString(bytes, 0, bytesRead);

                //raise the event
                if (!string.IsNullOrWhiteSpace(response))
                    OnMotorMessage(this, response.Replace(Environment.NewLine, "").Replace(">", ""));

            }
            catch (Exception ex)
            {
                if (client.Connected)
                {
                    throw new Exception("Motor listener error: " + ex.Message);
                }
            }
        }
    }

    public void Dispose()
    {
        try
        {
            client.Close();
            ns.Flush();
            ns.Close();
            ns.Dispose();
        }
        catch (Exception ex)
        {
            throw new Exception("Motor dispose error: " + ex.Message);
        }
    }
}

This works but I'm still not feeling good with my long lived NetworkStream and the fact that I couldn't separate the SendCommand() and the Listener() into two different classes because the listener class stooped receiving data when used with a different TcpClient instance.
note - tried to add this as an edit to my original question but it was deleted

Improve this answer
\$\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.