0
\$\begingroup\$

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
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Down and close voter: Please explain why you down vote and/or vote for closing this question. \$\endgroup\$ – user73941 Mar 1 at 14:33
  • 8
    \$\begingroup\$ I am not the down voter or the close voter, but it would be better if the question contained a little more information about what the code does. \$\endgroup\$ – pacmaninbw Mar 1 at 17:07
2
\$\begingroup\$

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
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Is it safe to use NetworkStream as a property? Or, should I rather keep it local? \$\endgroup\$ – user366312 Mar 2 at 12:19
  • \$\begingroup\$ @user366312 if you use private readonly NetworkStream stream this means, you only initialize it once by a constructor or inline assignment and it would be immutable. \$\endgroup\$ – iSR5 Mar 2 at 14:56

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