I am working on a banking application. I want to create a multithreaded TCP Payment Card (iso8583) server that can handle passbook printing requests simultaneously. Multiple devices are connected to the server from different locations.
I am going to use below code in my application. Is this thread safe? Can I face any problem in the future if I use this code? All suggestions welcome.
class Program
{
static void Main(string[] args)
{
TcpListener serverSocket = new TcpListener(8888);
TcpClient clientSocket = default(TcpClient);
int counter = 0;
serverSocket.Start();
Console.WriteLine(" >> " + "Server Started");
counter = 0;
while (true)
{
counter += 1;
clientSocket = serverSocket.AcceptTcpClient();
Console.WriteLine(" >> " + "Client No:" + Convert.ToString(counter) + " started!");
handleClinet client = new handleClinet();
client.startClient(clientSocket, Convert.ToString(counter));
}
clientSocket.Close();
serverSocket.Stop();
// Console.WriteLine(" >> " + "exit");
Console.ReadLine();
}
}
//Class to handle each client request separatly
public class handleClinet
{
TcpClient clientSocket;
string clNo;
public void startClient(TcpClient inClientSocket, string clineNo)
{
this.clientSocket = inClientSocket;
this.clNo = clineNo;
Thread ctThread = new Thread(doChat);
ctThread.Start();
}
private void doChat()
{
int requestCount = 0;
byte[] bytesFrom = new byte[10025];
string dataFromClient = null;
Byte[] sendBytes = null;
string serverResponse = null;
string rCount = null;
requestCount = 0;
while ((true))
{
try
{
var respose = "";
requestCount = requestCount + 1;
NetworkStream networkStream = clientSocket.GetStream();
networkStream.Read(bytesFrom, 0, (int)clientSocket.ReceiveBufferSize);
dataFromClient = System.Text.Encoding.ASCII.GetString(bytesFrom);
// dataFromClient = dataFromClient.Substring(0, dataFromClient.IndexOf("$"));
Console.WriteLine(" >> " + "From client-" + clNo + dataFromClient);
try
{
var isoPassbookRequestMessage = System.Text.Encoding.ASCII.GetString(bytesFrom);
WebClient wc = new WebClient();
NameValueCollection input = new NameValueCollection();
input.Add("isoPassbookRequest", Convert.ToBase64String(bytesFrom));
respose = Encoding.ASCII.GetString(wc.UploadValues("http://localhost:52835/Transaction/PassbookTransactionRequest", input));
try
{
// CommonMethods.AddtoLogFile("PassbookTransactionResponse = Clientid-" + clientID + " ProcessingCode -930000 Message -" + respose);
// atmServer.Send(clientID, Encoding.ASCII.GetBytes(respose));
}
catch (SocketException se)
{
//could not complete transaction
//Send reversal to CBS
}
}
catch (Exception e)
{
}
rCount = Convert.ToString(requestCount);
serverResponse = "Server to clinet(" + clNo + ") " + rCount;
sendBytes = Encoding.ASCII.GetBytes(respose);
networkStream.Write(sendBytes, 0, sendBytes.Length);
networkStream.Flush();
Console.WriteLine(" >> " + serverResponse);
}
catch (Exception ex)
{
Console.WriteLine(" >> " + ex.ToString());
}
}
}
}
-
1\$\begingroup\$ Just a warning, your code formatting is...really unreadable. \$\endgroup\$ediblecode– ediblecode2014年12月08日 11:44:17 +00:00Commented Dec 8, 2014 at 11:44
-
\$\begingroup\$ Please fix the formatting of your code: remove all superfluous empty lines. Also, consider having a different code-block for each class (perhaps separated by a horizontal line) instead of one gigantic block of code. Also clean your code: remove commented lines. \$\endgroup\$BCdotWEB– BCdotWEB2014年12月08日 12:27:36 +00:00Commented Dec 8, 2014 at 12:27
-
\$\begingroup\$ You state this is production code. Throw it away, learn about WCF and use it as a framework and container for your business logic. Anytime you find yourself thinking "somebody must have done something similar before" with production code, find the framework our library where it's already been done and use it instead. \$\endgroup\$psaxton– psaxton2014年12月09日 08:21:25 +00:00Commented Dec 9, 2014 at 8:21
1 Answer 1
Naming and Style
- based on the naming guidlines class names and method names should be using
PascalCase
casing. - you have to much vertical spacing (new lines)
- dead code should be removed
- names should be spelled correctly
handleClinet
->HandleClient
orrespose
->response
- class names should be made out of nouns or noun phrases.
HandleClient
->ClientHandler
General
- the use constructor
TcpListener(Int32)
is obsolete since NET 3.5 - empty try blocks are useless and should be removed
- empty catch blocks are dangerous and should be avoided. If you really need to swallow an exception you should add a comment there explaining why the catch block is empty.
Small code duplications
int requestCount = 0; // // requestCount = 0;
the second assignment is not needed.
Magic numbers
You have at least one magic number in your code byte[10025];
. You should extract such magic numbers into constants with meaningful names.
Explore related questions
See similar questions with these tags.