A common class for Client and Server [closed]

Question

I have written this code to use TcpClient both from a client program and a server program.

Can you review this source code?

• Kindly, take a close look at Disposable pattern at the bottom of the source code.

.

using System;
using System.Collections.Generic;
using System.IO;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading.Tasks;

namespace ClientServerLib
{
    public class ClientClass : IDisposable
    {
        private string Host { get; set; }
        private int Port { get; set; }

        private bool IsConnected = false;

        public string ID { get; private set; }
        public TcpClient Tcp { get; private set; }

        private System.Net.Sockets.NetworkStream stream;

        public ClientClass()
        {
            Random rnd = new Random();
            ID = AlphaNumRandom.GenerateUpperCaseString(5, rnd);
        }

        //constructor for server program.
        public ClientClass(TcpListener listener)
        {
            Tcp = listener.AcceptTcpClient();

            Host = ((IPEndPoint)Tcp.Client.RemoteEndPoint).Address.ToString();
            Port = ((IPEndPoint)Tcp.Client.LocalEndPoint).Port;

            IsConnected = true;

            stream = Tcp.GetStream();

            ID = Read();

            Console.WriteLine("Client [{0}] is now connected.", ID);
        }

        public bool Connect()
        {
            if (IsConnected == false)
            {
                Console.WriteLine("Client [{0}] is now connected.", ID);

                IsConnected = true;

                Tcp = new TcpClient(Host, Port);

                stream = Tcp.GetStream();

                return true;
            }

            return false;
        }

        //constructor for client.
        public ClientClass(string host, int port)
        {
            Random rnd = new Random();
            ID = AlphaNumRandom.GenerateUpperCaseString(5, rnd);
            Host = host;
            Port = port;
        }

        public string Read()
        {
            if (IsConnected)
            {
                byte[] buffer = new byte[Tcp.ReceiveBufferSize];//create a byte array
                int bytesRead = stream.Read(buffer, 0, Tcp.ReceiveBufferSize);//read count
                string str = Encoding.ASCII.GetString(buffer, 0, bytesRead);//convert to string
                return str.TrimEnd(new char[] {'\r', '\n'});//remove CR and LF
            }
            else
            {
                throw new Exception("Client " + ID + " is not connected!");
            }
        }

        public void Write(string str)
        {
            if (IsConnected)
            {
                str = str + Constants.CRLF;// add CR and LF
                byte[] bytesToSend = ASCIIEncoding.ASCII.GetBytes(str);
                stream.Write(bytesToSend, 0, bytesToSend.Length);
            }
            else
            {
                throw new Exception("Client " + ID + " is not connected!");
            }
        }

        public void PrintID()
        {
            Console.WriteLine("Client ID = {0}", ID);
        }

        public void SendIdToServer()
        {
            this.Write(ID);
        }

        public bool Disconnect()
        {
            if (IsConnected)
            {
                if (Tcp != null)
                {
                    stream.Close();
                    Tcp.Close();

                    Console.WriteLine("\nClient [{0}] is now disconnected.", ID);

                    return true;
                }
            }

            return false;
        }

        #region dispose pattern
        // Flag: Has Dispose already been called?
        bool disposed = false;

        // Public implementation of Dispose pattern callable by consumers.
        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }

        // Protected implementation of Dispose pattern.
        protected virtual void Dispose(bool disposing)
        {
            if (disposed)
                return;

            if (disposing)
            {
                // Free any other managed objects here
                if (stream != null)
                {
                    stream.Close();
                    stream.Dispose();
                    stream = null;
                }
                if (Tcp != null)
                {
                    if (Tcp.Connected)
                    {
                        Tcp.Client.Disconnect(false);
                        Tcp.Client.Close();
                        Tcp.Client.Dispose();
                        Tcp.Client = null;

                        Tcp.GetStream().Close();
                        Tcp.Close();                        
                        Tcp = null;
                    }
                }
            }

            // Free any unmanaged objects here.
            // ...
            disposed = true;
        }

        ~ClientClass()
        {
            Dispose(false);
        }
        #endregion
    }
}

Answer 1

Few things,

Your properties should be all set to readonly as you need to enforce your class requirements. You can make them public, but with a private set;. or simply { get; }.

TcpClient is a main requirement in this class, and you didn't setup the class constructor correctly for that purpose. Because of that, you were constructing inside Connect(). For that, you need to setup your constructors something like this :

// should be private since Host and Port are Required
private ClientClass()
{
    Random rnd = new Random();
    ID = AlphaNumRandom.GenerateUpperCaseString(5, rnd);
}

// you can make it public or private it's optional.
private ClientClass(TcpClient tcp) : this()
{
    Tcp = tcp;

    Host = ((IPEndPoint)tcp.Client.RemoteEndPoint).Address.ToString();

    Port = ((IPEndPoint)tcp.Client.LocalEndPoint).Port;

    stream = tcp.GetStream();
}

//constructor for client.
public ClientClass(string host, int port) : this(new TcpClient(host, port)) { }

//constructor for server program.
public ClientClass(TcpListener listener) : this(listener.AcceptTcpClient())
{
    ID = Read();
}

the Connect() and Disconnect() methods are confusing, because you're returing a boolean, and also you have already IsConnected which you're making it true in different places, and before even initiating the TcpClient how come ?

You should do something like this :

// there is no need to return a bool
// if you need to return a value
// use any other return type other than boolean
public void Connect()
{
    if (!IsConnected)
    {
        //no need to intilize TcpClient, it's already initilized from the constroctur
        IsConnected = true;

        Console.WriteLine("Client [{0}] is now connected.", ID);
    }
    else
    {
        Console.WriteLine("Client [{0}] is already connected.", ID);
    }
}

// there is no need to return a bool
public void Disconnect()
{
    if (IsConnected && Tcp != null)
    {
        stream.Close();
        Tcp.Close();
        IsConnected = false;
    }

    Console.WriteLine("\nClient [{0}] is disconnected.", ID);
}

IsConnected property is covering the status. avoid redundancy.

public string Read()
{
    if (IsConnected)
    {
        byte[] buffer = new byte[Tcp.ReceiveBufferSize];//create a byte array
        int bytesRead = stream.Read(buffer, 0, Tcp.ReceiveBufferSize);//read count
        string str = Encoding.ASCII.GetString(buffer, 0, bytesRead);//convert to string
        return str.TrimEnd(new char[] {'\r', '\n'});//remove CR and LF
    }
    else
    {
        throw new Exception("Client " + ID + " is not connected!");
    }
}

Validations and Exceptions always comes first, to make it readable, and clear to human eye, and also would give you a good understanding on what's coming next. So, Don't move them at the bottom of your code neither inside else clause. Instead do this :

public string Read()
{
    if (!IsConnected)
    {
        throw new Exception("Client " + ID + " is not connected!");
    }

    byte[] buffer = new byte[Tcp.ReceiveBufferSize];//create a byte array
    int bytesRead = stream.Read(buffer, 0, Tcp.ReceiveBufferSize);//read count
    string str = Encoding.ASCII.GetString(buffer, 0, bytesRead);//convert to string
    return str.TrimEnd(new char[] { '\r', '\n' });//remove CR and LF
}

Lastly, disposable parts. You need to know when you call Dispose() or using using clause, the instance would be disposed, if there is a connection, it would be closed automatically, along with any inner disposable objects. At least, this would be mostly applied on .NET disposable objects.

also, if you want a better code readability, try to minimize the use of return; like on this line :

if (disposed)
return;

check this out :

#region dispose pattern

private bool disposed = false;

public void Dispose()
{
    Dispose(true);
    GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
    if (!disposed)
    {
        if (disposing)
        {
            // Free any other managed objects here
            if (stream != null)
            {
                stream.Dispose();
                stream = null;
            }

            if (Tcp != null && Tcp.Connected)
            {
                Tcp.Dispose();
                Tcp = null;
            }
        }

        disposed = true;
    }
}

~ClientClass()
{
    Dispose(false);
}

#endregion