6
\$\begingroup\$

I coded a server-client (kind of) chat, and I need your review as I'm sure it is a mess. I used lots of tutorials and tips form different websites and forums which were posted at different times (maybe now it's a lot easier to write what I want) and slapped them on Visual Studio. It's not like I wanted it to be a chat, but a way to communicate with a server as a client.

Server-Side

using System;
using System.Text;
using System.Net;
using System.Net.Sockets;
using System.Threading;

namespace First_Server
{
    class Program
    {

    private static void SocketThread(object obj)
    {
        var ClientSocket = (TcpClient)obj;
        while (true)
        {
            try
            {
                NetworkStream NetworkStream = ClientSocket.GetStream();
                byte[] BytesFromClient = new byte[10025];
                NetworkStream.Read(BytesFromClient, 0, ClientSocket.ReceiveBufferSize);
                string DataFromClient = Encoding.ASCII.GetString(BytesFromClient);
                DataFromClient = DataFromClient.Substring(0, DataFromClient.IndexOf("#"));
                Console.WriteLine(DataFromClient);
                string ServerResponse = DataFromClient;
                Byte[] SendBytes = Encoding.ASCII.GetBytes(ServerResponse);
                NetworkStream.Write(SendBytes, 0, SendBytes.Length);
            }
            catch (Exception StreamException)
            {
                Console.WriteLine("Connection from a client closed.");
                ClientSocket.Close();
            }
        }
    }

    static void Main(string[] args)
    {
        int Port = 42069;
        IPAddress LocalIPAddress = IPAddress.Parse("192.168.1.10");
        TcpListener ServerSocket = new TcpListener(LocalIPAddress, Port);
        TcpClient ClientSocket;
        ServerSocket.Start(); Console.WriteLine("Server started. Waiting for requests...");

        while (true)
        {
            ClientSocket = ServerSocket.AcceptTcpClient(); Console.WriteLine("Connection from a client accepted.");
            ThreadPool.QueueUserWorkItem(SocketThread, ClientSocket);
        }
    }
}
}

Client-Side

using System;
using System.Text;
using System.Threading;
using System.Net.Sockets;
using System.Windows.Forms;

namespace First_Client
{
  public partial class Form1 : Form
  {

    TcpClient ClientSocket = new TcpClient();

    public void msg(string Message)
    {
        textBox1.Text = textBox1.Text + Message + Environment.NewLine;
    }

    public void ServerListener(object obj)
    {
        loop:
        var NetworkStream = (NetworkStream)obj;
        byte[] FromTheServer = new byte[10025];

        try
        {
            NetworkStream.Read(FromTheServer, 0, ClientSocket.ReceiveBufferSize);
        }
        catch (Exception StreamError)
        {
            goto loop;
        }

        string StringFromServer = Encoding.ASCII.GetString(FromTheServer);
        textBox1.Text = textBox1.Text + StringFromServer + Environment.NewLine;
    }

    public Form1()
    {
        InitializeComponent();
        ClientSocket.Connect("95.76.40.237", 42069); label1.Text = "Client status: connected to server";
        ThreadPool.QueueUserWorkItem(ServerListener);
    }

    private void button1_Click(object sender, EventArgs e)
    {
        NetworkStream NetworkStream = ClientSocket.GetStream();
        byte[] ToTheServer = Encoding.ASCII.GetBytes(textBox2.Text + "#");
        NetworkStream.Write(ToTheServer, 0, ToTheServer.Length);
        byte[] FromTheServer = new byte[10025];
        NetworkStream.Read(FromTheServer, 0, ClientSocket.ReceiveBufferSize);
        string StringFromServer = Encoding.ASCII.GetString(FromTheServer);
        msg(StringFromServer);
        textBox2.Text = "";
    }

    private void Form1_Load(object sender, EventArgs e)
    {

    }

    private void label1_Click(object sender, EventArgs e)
    {

    }
}
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Did you think to use SignalR for your task? \$\endgroup\$ Feb 3, 2017 at 21:45

1 Answer 1

3
\$\begingroup\$

Naming

You code's naming conventions are very inconsistent (which possibly reflects the fact that you've taken bits of it from different sources). Generally speaking, I expect local variables to follow a camelCasing naming convention. Your current approach ends up with lines like this:

catch (Exception StreamException)

Which honestly at first glance made me ask the question 'have they done something funky with exception filters'.

Exceptions

You don't actually do anything with any of the exceptions that you catch. If you don't need to access the variable, then you can simply catch the exception:

catch (Exception)

Typically you'd also want to try to avoid catching base exception. It suggests that you don't really know what exceptions can be thrown. Sometimes it's the right thing to do, but more often than not you should be catching more specific exceptions so that you can take more appropriate action.

When you're done, you're done

Your thread function in your server contains an endless loop. This works whilst the client is behaving, but falls apart once you encounter an error. Within the loop, you're catching all exceptions and responding by closing the socket:

catch (Exception StreamException)
{
    Console.WriteLine("Connection from a client closed.");
    ClientSocket.Close();
}

Best case, the second time you call Close, it throws an exception and the thread function crashes. Worst case, the Close doesn't throw an exception and you don't notice that the thread function is in an endless loop, eating processor time.

Once you've closed the connection, you should return from the thread function to prevent the pointless spinning.

\$\endgroup\$
2
  • \$\begingroup\$ Upvoted but I think I'll never agree to this but more often than not you should be catching more specific exceptions so that you can take more appropriate action. I don't know why but I've never seen a real-world case where it would make sense to catch a specific exception and do something different then when other exception occurs. \$\endgroup\$
    – t3chb0t
    Feb 6, 2017 at 15:08
  • \$\begingroup\$ @t3chb0t Well, in this example, there's an assumption that anything read will contain '#', if it's not there you can get ArgumentOutOfRange exceptions. You might choose to be tolerant of malformed messages and either buffer for the next read, or discard and read again. If on the other hand you receive stream related exceptions then you're probably going to want to treat the stream as aborted and stop processing. Avoiding the string exception would be an alternate/better approach, however failing that handling the exceptions differently can work. \$\endgroup\$
    – forsvarir
    Feb 6, 2017 at 15:20

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.