TCP Client Class implementation in C#

Question

I tried to implement following class which should handle connecting to a server, as well as sending and receiving data.

Your feedback is welcome. Please note I am beginner in .NET and don't give too advanced feedback. I tested it and it seems to work. So in case you see some major problems please let me know, otherwise this class seems to fulfill my needs. I may add some small functionalities though.

Also can I better release resources than I am doing now? (maybe automatically close all resources I am using if that's possible?)

 class DppTCPClient
    {
        public TcpClient m_client = new TcpClient();

        public void Connect(string address, int port)
        {
            if (m_client.Connected)
                throw new Exception("Connect: Already connected");

            m_client.Connect(address, port);
        }

        public void WriteBytes(byte[] data, int length)
        {
            if (data.Length != length)
                throw new Exception("WriteBytes: Length should be same");

            // Get access to network stream
            Stream stm = m_client.GetStream();
            stm.Write(data, 0, data.Length);

        }

        public void ReadAllBytes(byte[] buffer, int length)
        {
            if (buffer.Length != length)
                throw new Exception("ReadAllBytes: Length should be same");

            Stream stm = m_client.GetStream();

            // Start reading
            int offset = 0;
            int remaining = length;
            while (remaining > 0)
            {
                int read = stm.Read(buffer, offset, remaining);
                if (read <= 0)
                    throw new EndOfStreamException
                        (String.Format("ReadAllBytes: End of stream reached with {0} bytes left to read", remaining));
                remaining -= read;
                offset += read;
            }

        }

        public void CloseDppClient()
        {
            if (m_client.Connected)
            {
                m_client.GetStream().Close();
                m_client.Close();
            }
        }
    }

Answer 1

It looks like you should be implementing IDisposable here. See on MSDN.

Then, in your Dispose(bool) method, you can call CloseDppClient (or, better yet, do them the other way around and call Dispose() from your CloseDppClient method).

You should also be putting explicit access modifiers on your classes, properties, methods, fields, events, etc. As it stands now, DppTCPClient is internal.

Also, it's usually expected to follow PascalCasing throughout C# programs for public, class, interface and enum definitions.

I've left DppTcpClient as internal, but if you want it accessible outside the assembly simply replace internal with public.

An example would look like:

internal class DppTcpClient
    : IDisposable
{
    // Flag: Has Dispose already been called?
    bool disposed = false;

    // Original code

    public void CloseDppClient()
    {
        Dispose();
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);           
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposed)
        {
            return; 
        }

        if (disposing)
        {
            if (m_client.Connected)
            {
                m_client.GetStream().Close();
                m_client.Close();
            }
            // Free any other managed objects here.
        }

        // Free any unmanaged objects here.
        disposed = true;
    }

    ~DppTcpClient()
    {
        Dispose(false);
    }
}

Then, you should be using your DppTcpClient where you need it:

using (DppTcpClient client = new DppTcpClient())
{
    // Code to use `client` here
}

So long as you always wrap any DppTcpClient objects with a using, you'll never run into a stale client left open.

---

Always use braces.

if (data.Length != length)
    throw new Exception("WriteBytes: Length should be same");

Should be written as:

if (data.Length != length)
{
    throw new Exception("WriteBytes: Length should be same");
}

Braces can't prevent bugs, but they can help prevent bugs. (If you know what I mean.)

---

From what I can tell, your ReadAllBytes method is actually not supposed to read all the bytes in the stream (or so it sounds), by this manner you should be changing how you work with it internally.

Instead of having the user supply a buffer, you should supply it by use of either:

1. A return type;
2. An out parameter;

The return type would be more appropriate here, so you could do something like:

public byte[] ReadAllBytes()
{
    using (MemoryStream ms = new MemoryStream())
    {
        m_client.GetStream().CopyTo(ms);
        return ms.ToArray();
    }
}

See: https://stackoverflow.com/questions/19387979/get-length-of-data-available-in-networkstream for more information on reading the entire stream.

This simplifies it greatly, and takes a large brunt of the work off your shoulders.

Just as well, you can completely remove the int length parameter from the WriteBytes method:

public void WriteBytes(byte[] data, int length)
{
    // Get access to network stream
    Stream stm = m_client.GetStream();
    stm.Write(data, 0, data.Length);
}

Lastly, I would rename your ReadAllBytes method to ReadBytes, since that's what it's doing.

Answer 2

Seems pretty solid, but there are some style things that I would personally change.

In

public void WriteBytes(byte[] data, int length) { ... }

and

public void ReadAllBytes(byte[] buffer, int length) { ... }

there doesn't seem to be a need for the length parameter. This would get rid of the data.Length and buffer.Length checks and related exceptions.

In Connect, WriteBytes, and ReadAllBytes, ensure that all of the parameters are 'trusted'; meaning you know exactly where they come from and that they are safe and not null.

Style wise, it is helpful to make comments above each of these methods stating that each method is expecting good/safe parameters.

The ReadAllBytes method, may be simplified:

/// <summary>
/// TODO
/// </summary>
/// <param name="buffer">The buffer to read data from. Assumed to be not null.</param>
public void ReadAllBytes(byte[] buffer)
{
    Stream stm = m_client.GetStream();
    while (stm.Read(buffer, 0, buffer.Length) > 0)
    {
        // TODO
    }
}

No need to over-complicate things by tracking variables that have no use. If you do have a use for say the offset variable in the future, add it then, not now. Right now, there is no use; keep it simple.

Answer 3

My biggest issue with your code is that it doesn't achieve anything useful. It's pretty much a thin wrapper over TcpClient that doesn't add any abstraction or features.

Btw your ReadAllBytes function exists in the BCL, in the form of BinaryReader.ReadBytes. If you want to use it without having to instantiate BinaryReader (which prevents you from using the underlying stream) I'd rather add a ReadBytes extension method to Stream instead of adding it on your TcpClient wrapper.

Passing in byte[] arrays as buffer is idiomatic in IO code and avoids unnecessary allocations. So choosing between returning a new instance byte[] and writing into an existing byte[] is a performance-simplicity trade-off. In my experience using existing buffers is preferable in low level code, since you don't know how it will be used and returning new instances is preferable if you write high level code and know that you don't need much throughput. As a variant of passing in buffers, you can pass in an ArraySegment<byte>.

---

Concerning the disposing: Since you don't own any unmanaged resources directly, all you need to do is forward your Dispose call to the disposable type you're wrapping. Optionally make it virtual if you want derived classes to be able to add ownership of additional disposable types.

Something like:

class DppTCPClient : IDisposable
{
    public void Dispose()
    {
        m_client.Dispose();
    }
}