IDisposable Interface for an Server/Client connection client

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

```
//-----------------------------------------------------------------------
//
// Copyright (c) -. All rights reserved.
//
// -
//-----------------------------------------------------------------------

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

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

///
/// Stores the streamwriter of the TcpClient
private StreamWriter writer;

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

///
/// Unique ID of the client
private int id;

///
/// Initializes a new instance of the Client class.
/// The TcpClient of the client connection
/// The unique Id of the client
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();
}

///
/// The internal OnClientMessage Handler.
/// Client who fired the event
/// InternalClientEventArgs include the received message
public delegate void InternalOnClientMessageHandler(object sender, InternalClientMessageEventArgs e);

///
/// The internal OnClientDisconnect Handler.
/// Client who fired the event
/// EventArgs of the event
public delegate void InternalOnClientDisconnectHandler(object sender, EventArgs e);

///

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

-
It's confusing to have public members called Internal*

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

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

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

• You call this.InternalOnClientException without ensuring it's not null.

• Use a property for public int GetID() instead of a method.

StackExchange Code Review Q#46572, answer score: 7