4
\$\begingroup\$

I've written a basic server/client application to use in an automation application written in C#. The code is working pretty good, but I have a few thing I want to improve:

Server:

   public void socketListener()
{

    peerListener = new TcpListener(IPAddress.Any, ControlLayer.control_GlobalParam.TCP_PORT);
    peerListener.Start();
    int MessageLength = 0;
    //_infrastructure_TcpServerAndClient.PeerListener_start();
    Socket socket = null;
    while (true)
    {
        if (peerListener.Pending())
        {
            Thread.Sleep(500);
            continue;
        }
        else
        {
            byte[] StreamMessage = new byte[9632*2];
            try
            {
            socket = peerListener.AcceptSocket();

            Thread.Sleep(500);

                MessageLength = socket.Receive(StreamMessage, 0, StreamMessage.Length, SocketFlags.None);
            }
            catch (Exception ex)
            {
                //remote disconnected garcefully ? 
                Console.WriteLine(ex);

            }
            if (MessageLength > 0 )
            {
                string message = System.Text.Encoding.Default.GetString(StreamMessage);

                if (Domain_GlobalParam.IsCollectTcpServerOutput)
                {
                    onDataRecieved(this, "Collecting log from Remote station", "TcpServer");
                }
                else
                {
                    onDataRecieved(this, message, "TcpServer"); 
                }
                new Thread(() => ParseMessage(message)).Start();

                //ParseMessage(message);
            }
        }
    }
}

Client:

        internal void CreateClient(object message)
    {
        try
        {

            _infrastructure_TcpServerAndClient.PeerClient_Connect();
            _infrastructure_TcpServerAndClient.SendMessageViaStreamWriter(message);
            _infrastructure_TcpServerAndClient.CloseConnection();

        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.ToString());
        }
    }
    internal void PeerClient_Connect()
{
    peerClient = new TcpClient();
    peerClient.Connect(ComSettings.IpAddress, ComSettings.Port);

}
internal void SendMessageViaStreamWriter(object message)
{
    string MessageOut = (string)message + "\n";
    netStream = peerClient.GetStream();
    StreamWriter sw = new StreamWriter(netStream);
    sw.Write(MessageOut);
    sw.Flush();
}
internal void CloseConnection()
{
    netStream.Close();
    peerClient.Close();
}

I've noticed that from time to time I miss out some messages (especially long ones). What do you think I can improve here?

\$\endgroup\$

1 Answer 1

6
\$\begingroup\$
  • dead code should be removed
  • the indention is horrible

        byte[] StreamMessage = new byte[9632*2];
        try
        {
        socket = peerListener.AcceptSocket();
    
        Thread.Sleep(500);
    
            MessageLength = socket.Receive(StreamMessage, 0, StreamMessage.Length, SocketFlags.None);
        }  
    

    should be look like

        byte[] StreamMessage = new byte[9632*2];
        try
        {
            socket = peerListener.AcceptSocket();
    
            Thread.Sleep(500);
    
            MessageLength = socket.Receive(StreamMessage, 0, StreamMessage.Length, SocketFlags.None);
        }
    
  • you should enclose IDisposable's like Stream, StreamWriter in using blocks.

    internal void SendMessageViaStreamWriter(object message)
    {
        string messageOut = (string)message + "\n";
        using (StreamWriter sw = new StreamWriter(peerClient.GetStream()))
        {
            sw.Write(messageOut);
            sw.Flush();
        }
    }
    

    The use of the using statement ensures that the Dispose() method is called in any case (like an exception has happened) this also ensures that the stream is closed.

  • methods should be named using PascalCase casing and should be made out of verbs or verb phrases. See: Naming Guidelines

  • local variables should be named using camelCase casing.
  • the Thread.Sleep(500); does make no sense after accepting the socket. Why do you want the thread to sleep ?
  • SendMessageViaStreamWriter is badly named. SendMessage would be much better. What would happen if you decide to change the implementation of the method but forget to change the name, ? Mr.Maintainer would be confused.
\$\endgroup\$
5
  • \$\begingroup\$ THank you for your constructive answer ! point 1 : you right point 2 : what do you mean ? point 3 : can you please show me what do you mean ? \$\endgroup\$ Dec 15, 2014 at 16:05
  • \$\begingroup\$ @Heslacher, I think there was a paste error. Only the first line of each method looks jacked up. \$\endgroup\$
    – RubberDuck
    Dec 15, 2014 at 16:10
  • \$\begingroup\$ @RubberDuck, as all posted code is up for review ... and that isn't the only one, like you see in my updated answer. \$\endgroup\$
    – Heslacher
    Dec 15, 2014 at 16:13
  • \$\begingroup\$ ahhhh my apologies @Heslacher I had missed that. Good catch. \$\endgroup\$
    – RubberDuck
    Dec 15, 2014 at 16:25
  • \$\begingroup\$ @Heslacher , Great :) I'm going to mark this answer ofcourse But would you please give me some advise on the logic as well ? \$\endgroup\$ Dec 16, 2014 at 7:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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