Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 good the IDisposable interface is implemented because I never did it before and I'm not sure if I understand it 100% correct. 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 find as mistaken:
rules
Click here for bigger image.

share|improve this question
    
Could you please give a more distinctive title, preferably one based on the code's purpose? –  Jamal Apr 7 at 17:10
    
Updated it. Is this better? –  Kimmax Apr 7 at 17:11
    
Yes (after some adjustments). Thanks! –  Jamal Apr 7 at 17:13

3 Answers 3

up vote 6 down vote accepted
  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.
share|improve this answer
    
Yes I should extend the comments. I'm programming a library and I'm using the Internal* events to fire events that are used inside the library but the end user application. Is there a way I could use them inside the library project but hide them outside the lib? Point 4/5: Does this mean I should remove the Dispose part completely? Point 5: I want to handle IOExceptions and ObjectDisposedExceptions in my server class, so I used events to handle them nicely. Should I rethrow any other exceptions? Point 6/7 are right. Point 3: Should I safe the stream separately and use it instant? –  Kimmax Apr 8 at 11:06

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.

share|improve this answer
    
Okay so I moved the one liners to the bottom. The declarations should stay on the top, right? After moving the one liners to the bottom StyleCop tells me that the methods should be placed after the delegates and events. Should I care about the "Rules" or about own oppinion? –  Kimmax Apr 7 at 17:49
    
Hmmm, I am really not a C# expert, so I cannot give you much guidance. I can tell you that I think the new version is more readable. Note that you should not take away the original code, you can have a second code box with the changed code. Otherwise the mods will roll back your changes to the question. –  konijn Apr 7 at 18:17
    
Okay I think I will keep the new code and added the original code. Thank you! –  Kimmax Apr 7 at 18:21

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.

share|improve this answer
1  
I followed the Stylecop rules. Same topic here –  Kimmax Apr 8 at 9:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.