TCP Client Class implementation in C#

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)

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:

• A return type;

• 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.

StackExchange Code Review Q#107380, answer score: 18