IDisposable Interface for an Server/Client connection client

Question

Here is a class of one of my applications I'm trying to learn writing good code with. Old Code:

//-----------------------------------------------------------------------
// <copyright file="Client.cs" company="-">
//     Copyright (c) -. All rights reserved.
// </copyright>
// <author>-</author>
//-----------------------------------------------------------------------

namespace LightCS
{
    using System;
    using System.IO;
    using System.Net.Sockets;
    using System.Threading;

    /// <summary>
    /// Stores information about a single client and provides functions to control the client.
    /// </summary>
    public partial class Client : IDisposable
    {
        /// <summary>
        /// <c>TcpClient</c> of the connection</summary>
        private TcpClient tcpClient;

        /// <summary>
        /// Stores the <c>streamwriter</c> of the <c>TcpClient</c></summary>
        private StreamWriter writer;

        /// <summary>
        /// Declares if the client is alive or not.</summary>
        private bool alive = true;

        /// <summary>
        /// Unique ID of the client</summary>
        private int id;

        /// <summary>
        /// Initializes a new instance of the Client class. </summary>
        /// <param name="tcpClient">The <c>TcpClient</c> of the client connection</param>
        /// <param name="id">The unique Id of the client</param>
        public Client(TcpClient tcpClient, int id)
        {
            this.tcpClient = tcpClient;
            this.id = id;

            this.writer = new StreamWriter(tcpClient.GetStream());

            new Thread(() =>
            {
                this.Listen(new StreamReader(tcpClient.GetStream()));
            }).Start();
        }

        /// <summary>
        /// The internal OnClientMessage Handler.</summary>
        /// <param name="sender">Client who fired the event</param>
        /// <param name="e">InternalClientEventArgs include the received message</param>
        public delegate void InternalOnClientMessageHandler(object sender, InternalClientMessageEventArgs e);

        /// <summary>
        /// The internal OnClientDisconnect Handler.</summary>
        /// <param name="sender">Client who fired the event</param>
        /// <param name="e">EventArgs of the event</param>
        public delegate void InternalOnClientDisconnectHandler(object sender, EventArgs e);

        /// <summary>
        /// The Internal OnClientMessage Event fires when a new message is received.</summary>
        public event InternalOnClientMessageHandler InternalOnClientMessage;

        /// <summary>
        /// The internal OnClientDisconnect Event fires when the client closes the connection.</summary>
        public event InternalOnClientDisconnectHandler InternalOnClientDisconnect;

        /// <summary>
        /// Sends a message to the client.</summary>
        /// <param name="message">Message to send</param>
        public void Write(string message)
        {
            if (this.alive)
            {
                this.writer.WriteLine(message);
                this.writer.Flush();
            }
        }

        /// <summary>
        /// Returns the client specific id.</summary>
        /// <returns>The client specific id</returns>
        public int GetID()
        {
            return this.id;
        }

        /// <summary>
        /// Closes the client connection and fires Internal OnClientDisconnect event.</summary>
        public void Close()
        {
            this.alive = false;
            this.writer.Close();
            this.tcpClient.GetStream().Close();
            this.tcpClient.Close();

            if (this.InternalOnClientDisconnect != null)
            {
                this.InternalOnClientDisconnect(this, new EventArgs());
            }
        }

        /// <summary>
        /// Listens for incoming messages and processes them. Also fires the Internal OnClientMessage event containing the message.</summary>
        /// <param name="reader">The <c>Streamreader</c> where should be read</param>
        private void Listen(StreamReader reader)
        {
            while (this.alive)
            {
                string input = reader.ReadLine();

                if (input == "exit")
                {
                    this.Close();
                    break;
                }

                if (this.InternalOnClientMessage != null)
                {
                    this.InternalOnClientMessage(this, new InternalClientMessageEventArgs(input));
                }

                Thread.Sleep(150);
            }
        }

        /// <summary>
        /// <c>Dispose()</c></summary>
        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }

        /// <summary>
        /// Dispose()</summary>
        /// <param name="disposing"><c>disposing</c></param>
        protected virtual void Dispose(bool disposing)
        {
            if (disposing) 
            {
                // free managed resources
                if (writer != null)
                {
                    writer.Dispose();
                    writer = null;
                }
            }
        }
    }
}

I would like to know how good this code is overall, but especially how well the IDisposable interface is implemented because I never did it before and I'm not sure if I understand it 100% correctly. Is there something I miss or I should add? Should I do something different?

I strictly followed the StyleCop rules and used the Analyse code function of Visual Studio.

Improved Code:

    //-----------------------------------------------------------------------
    // <copyright file="Client.cs" company="-">
    //     Copyright (c) -. All rights reserved.
    // </copyright>
    // <author>-</author>
    //-----------------------------------------------------------------------

    namespace LightCS
    {
        using System;
        using System.IO;
        using System.Net.Sockets;
        using System.Threading;

        /// <summary>
        /// Stores information about a single client and provides functions to control the client.
        /// </summary>
        public partial class Client : IDisposable
        {
            /// <summary>
            /// <c>TcpClient</c> of the connection</summary>
            private TcpClient tcpClient;

            /// <summary>
            /// Stores the <c>streamwriter</c> of the <c>TcpClient</c></summary>
            private StreamWriter writer;

            /// <summary>
            /// Declares if the client is alive or not.</summary>
            private bool alive = true;

            /// <summary>
            /// Unique ID of the client</summary>
            private int id;

            /// <summary>
            /// Initializes a new instance of the Client class. </summary>
            /// <param name="tcpClient">The <c>TcpClient</c> of the client connection</param>
            /// <param name="id">The unique Id of the client</param>
            public Client(TcpClient tcpClient, int id)
            {
                this.tcpClient = tcpClient;
                this.id = id;

                this.writer = new StreamWriter(tcpClient.GetStream());

                new Thread(() =>
                {
                    this.Listen(new StreamReader(tcpClient.GetStream()));
                }).Start();
            }

            /// <summary>
            /// Sends a message to the client.</summary>
            /// <param name="message">Message to send</param>
            /// <exception cref="ObjectDisposedException">Throws when object is disposed and is handled by firing the Internal OnClientException event</exception>
            /// <exception cref="IOException">Throws when an I/O Error occurs and is handled by firing the Internal OnClientException event</exception>
            public void Write(string message)
            {
                if (this.alive)
                {
                    try
                    {
                        this.writer.WriteLine(message);
                        this.writer.Flush();
                    }
                    catch (Exception ex)
                    {
                        if (ex is ObjectDisposedException || ex is IOException)
                        {
                            this.InternalOnClientException(this, new InternalClientExceptionEventArgs(ex));
                        }
                    }
                }
            }

            /// <summary>
            /// Listens for incoming messages and processes them. Also fires the Internal OnClientMessage event containing the message.</summary>
            /// <param name="reader">The <c>Streamreader</c> where should be read</param>
            /// <exception cref="ObjectDisposedException">Throws when object is disposed and is handled by firing the Internal OnClientException event</exception>
            /// <exception cref="IOException">Throws when an I/O Error occurs and is handled by firing the Internal OnClientException event</exception>
            private void Listen(StreamReader reader)
            {
                while (this.alive)
                {
                    try
                    {
                        string input = reader.ReadLine();

                        if (input == "exit")
                        {
                            this.Close();
                            break;
                        }

                        if (this.InternalOnClientMessage != null)
                        {
                            this.InternalOnClientMessage(this, new InternalClientMessageEventArgs(input));
                        }

                        Thread.Sleep(150);
                    }
                    catch (Exception ex)
                    {
                        if (ex is ObjectDisposedException || ex is IOException)
                        {
                            this.InternalOnClientException(this, new InternalClientExceptionEventArgs(ex));
                        }
                    }
                }
            }

            /// <summary>
            /// Returns the client specific id.</summary>
            /// <returns>The client specific id</returns>
            public int GetID()
            {
                return this.id;
            }

            /// <summary>
            /// Closes the client connection and fires Internal OnClientDisconnect event.</summary>
            public void Close()
            {
                this.alive = false;
                this.writer.Close();
                this.tcpClient.GetStream().Close();
                this.tcpClient.Close();

                if (this.InternalOnClientDisconnect != null)
                {
                    this.InternalOnClientDisconnect(this, new EventArgs());
                }
            }

            /// <summary>
            /// <c>Dispose()</c></summary>
            public void Dispose()
            {
                this.Dispose(true);
                GC.SuppressFinalize(this);
            }

            /// <summary>
            /// Calls Dispose()</summary>
            /// <param name="disposing"><c>disposing</c> argument</param>
            protected virtual void Dispose(bool disposing)
            {
                if (disposing) 
                {
                    // free managed resources
                    if (this.writer != null)
                    {
                        this.writer.Dispose();
                        this.writer = null;
                    }
                }
            }

            /// <summary>
            /// The internal <c>OnClientMessage</c> Handler.</summary>
            /// <param name="sender">Client who fired the event</param>
            /// <param name="e"><c>InternalClientEventArgs</c> include the received message</param>
            public delegate void InternalOnClientMessageHandler(object sender, InternalClientMessageEventArgs e);

            /// <summary>
            /// The internal <c>Onclientexception</c> Handler.</summary>
            /// <param name="sender">Client who fired the event</param>
            /// <param name="e">InternalClientEventArgs include the received message</param>
            public delegate void InternalOnClientExceptionHandler(object sender, InternalClientExceptionEventArgs e);

            /// <summary>
            /// The internal <c>OnClientDisconnect</c> Handler.</summary>
            /// <param name="sender">Client who fired the event</param>
            /// <param name="e">EventArgs of the event</param>
            public delegate void InternalOnClientDisconnectHandler(object sender, EventArgs e);

            /// <summary>
            /// The Internal <c>OnClientMessage</c> Event fires when a new message is received.</summary>
            public event InternalOnClientMessageHandler InternalOnClientMessage;

            /// <summary>
            /// The internal <c>OnClientException</c> Event fires when the client runs into an exception</summary>
            public event InternalOnClientExceptionHandler InternalOnClientException;

            /// <summary>
            /// The internal <c>OnClientDisconnect</c> Event fires when the client closes the connection.</summary>
            public event InternalOnClientDisconnectHandler InternalOnClientDisconnect;
        }
    }

The rules StlyeCop finds as mistaken:

Click here for bigger image.

Answer 1

1. Most of your comments are completely redundant. They only bloat the code without adding any information. For example for bool Client.alive you write:

   Declares if the client is alive or not.

   The reader learns nothing from this they can't already see from the the definition. You could instead document what being "alive" means. There are several possible definitions with subtle differences, telling the reader which you are using is helpful.

   It's the same for most of your other comments.

2. It's confusing to have public members called Internal*

3. When you construct a StreamWriter or StreamReader with the default constructor, it owns the underlying stream and closes it when it's closed.

   So closing the stream is redundant. You also have two objects (the reader and the writer) which own the stream, which I'd avoid.

4. GC.SuppressFinalize(this) is useless if your class has no finalizer. This also means Dispose(bool disposing) doesn't get called with false as parameter and can be thrown out.

   In general I'd recommend against using this disposing pattern. If you own unmanaged resources directly, implement a SafeHandle. If you only own them indirectly, there is no need for Dispose(bool) and the finalizer.

5. Don't silently swallow unexpected exceptions. They indicate a bug and should be rethrown or not be caught in the first place.

6. You call this.InternalOnClientException without ensuring it's not null.
7. Use a property for public int GetID() instead of a method.

Answer 2

Looks like very nice code, I only have 2 remarks:

• You declare a ton of one liners between public Client(TcpClient tcpClient, int id) and public void Write(string message) which harms the reading flow if you want to follow the code. I would suggest to put the oneliners completely at the bottom.

• You should support the throwing of IOException and ObjectDisposedException when you read (write?) from the connection.

Answer 3

On top of other remarks, I'm surprised this even works:

namespace LightCS
{
    using System;
    using System.IO;
    using System.Net.Sockets;
    using System.Threading;

Normally the using instructions sit at the very top of the code file, like this:

using System;
using System.IO;
using System.Net.Sockets;
using System.Threading;

namespace LightCS
{

It's somewhat off-putting to see them within a namespace scope.