10
\$\begingroup\$

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();
            }
        }
    }
\$\endgroup\$
  • \$\begingroup\$ For ReadAllBytes: Why do I need to provide the length if you can get it yourself? \$\endgroup\$ – Cole Johnson Oct 14 '15 at 0:52
  • \$\begingroup\$ @ColeJohnson: Maybe you wana read fewer bytes then length of byte array(but now it doesn't work like that) \$\endgroup\$ – user86726 Oct 14 '15 at 9:13
17
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Mathieu Guindon Oct 13 '15 at 18:49
  • \$\begingroup\$ MemoryStream can also take existing array in constructor, if you chose to reuse arrays. However, you changed semantics. Original ReadAllBytes throws an exception if connection cannot provide expected amount of bytes. \$\endgroup\$ – ArekBulski Oct 25 '15 at 11:58
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I've never seen these comments you're talking about assuming the safety of the class. Plus, written this way they're not even visible in the IDE. Otherwise, good answer! \$\endgroup\$ – IEatBagels Oct 13 '15 at 13:20
  • \$\begingroup\$ Maybe this isn't the place, but is there a 'good' way to handle possible null values? Or is it better to take the defensive approach and always assume the worse (ie, check that buffer != null in this case)? In addition, what about try/catching blocks of code. Like if we assume that this is suppose to throw exceptions that are passed up to the caller? Is that just a comment in that case? \$\endgroup\$ – JD.B Oct 13 '15 at 13:38
  • 2
    \$\begingroup\$ There's a [NotNull] attribute you can use, that indicates value shouldn't be null but doesn't make any validation. Apart from that, always be defensive about your input, you don't know if the user of your class is Satan or Bugs Bunny (who will be considered kind for this example). With the method's xml header there's a way to specify thrown exceptions! Check it out :) \$\endgroup\$ – IEatBagels Oct 13 '15 at 15:03
  • \$\begingroup\$ @TopinFrassi isn't [NotNull] part of the ReSharper annotations framework? AFAICT it only tells R# to check the call site isn't passing a possibly null reference. \$\endgroup\$ – Mathieu Guindon Oct 13 '15 at 17:36
  • 1
    \$\begingroup\$ Seem's it's not only resharper! \$\endgroup\$ – IEatBagels Oct 13 '15 at 17:42
6
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • \$\begingroup\$ First of all thanks a lot for your comments to Ebrowns answer! I was trying to explain to him similar points as you did, bot no result! Finally, can you please show me how to handle deallocating resources in the best way? As Ebrowns answer's approach was flawed as you indicated. If I leave it as I have now, what if exception happens, when should I call CloseDppClient on my class? \$\endgroup\$ – user86726 Oct 13 '15 at 18:30
  • \$\begingroup\$ @quser The point was that when a method alters a parameter, best-practice is to use ref or out to indicate as such, otherwise your method has surprising/unexpected side effects that need to be documented - and because self-documenting code is superior to surprising but documented code, using out or ref is preferred in this particular case. Generally speaking, returning a value is preferred to using ref or out parameters. \$\endgroup\$ – Mathieu Guindon Oct 13 '15 at 18:45
  • 2
    \$\begingroup\$ @Mat'sMug You should only use out/ref if you replace the whole instance, not if you just mutate the contents of a mutable type. In general I'd consider out only a work-around for .net's lack of support for multiple return values. \$\endgroup\$ – CodesInChaos Oct 13 '15 at 18:49
  • 3
    \$\begingroup\$ @quser using as in EBrown's answer is fine. His answer approach isn't incorrect just over-complicated. \$\endgroup\$ – CodesInChaos Oct 13 '15 at 19:07
  • 6
    \$\begingroup\$ @quser do note that reviewers don't even have to include code in their answers, those who do, do it as a courtesy. Feel free to ask on Stack Overflow if you need help with a specific issue; this is drifting from the scope of Code Review. \$\endgroup\$ – Mathieu Guindon Oct 13 '15 at 19:50

Your Answer

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