patterncsharpMinor
Server-client data transfer
Viewed 0 times
serverclientdatatransfer
Problem
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.WriteLi
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.WriteLi
Solution
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:
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:
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:
Best case, the second time you call
Once you've closed the connection, you should
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.Code Snippets
catch (Exception StreamException)catch (Exception)catch (Exception StreamException)
{
Console.WriteLine("Connection from a client closed.");
ClientSocket.Close();
}Context
StackExchange Code Review Q#154386, answer score: 3
Revisions (0)
No revisions yet.