Loading data from a remote app

Question

I have a method which loads data from a remote app (send TCP request and parse response). Now, I have a simple class for sending a TCP request:

public class PremieraTcpClient
 {
 public PremieraTcpClient()
 {
 QueryItems = new NameValueCollection();
 int port;
 int.TryParse(ConfigurationManager.AppSettings["PremieraPort"], out port);
 Port = port;
 ServerIp = ConfigurationManager.AppSettings["PremieraServerIp"];
 ServiceId = ConfigurationManager.AppSettings["PremieraServiceId"];
 }
 public NameValueCollection QueryItems { get; set; }
 private int Port { get; set; }
 private string ServerIp { get; set; }
 private string ServiceId { get; set; }
 private string ReadyQuery { get; set; }
 public string SendQuery()
 { 
 StringBuilder parameters = new StringBuilder();
 //...
 // build query for request
 //...
 ReadyQuery = parameters.ToString(); 
 return Connect(); 
 }
 private string Connect()
 {
 string responseData;
 try
 {
 TcpClient client = new TcpClient(ServerIp, Port);
 client.ReceiveBufferSize = Int32.MaxValue;
 Byte[] data = Encoding.GetEncoding(1251).GetBytes(ReadyQuery);
 NetworkStream stream = client.GetStream();
 // send data
 stream.Write(data, 0, data.Length);
 var sizeBuffer = new byte[10];
 stream.Read(sizeBuffer, 0, 10);
 var sizeMessage = int.Parse(Encoding.GetEncoding(1251).GetString(sizeBuffer, 0, 10));
 data = new Byte[sizeMessage];
 var readSoFar = 0;
 //read data
 while (readSoFar < sizeMessage)
 {
 readSoFar += stream.Read(data, readSoFar, data.Length - readSoFar);
 }
 responseData = Encoding.GetEncoding(1251).GetString(data, 0, data.Length); 
 responseData = responseData.TrimStart('&'); 
 stream.Close();
 client.Close();
 return responseData;
 }
 catch (ArgumentNullException e)
 {
 //return responseData = string.Format("ArgumentNullException: {0}", e);
 }
 catch (SocketException e)
 {
 //return responseData = string.Format("SocketException: {0}", e);
 }
 return string.Empty;
 } 
 }

This is method for load data:

 private static void GetUpdatesFromPremiera()
 {
 Debug.WriteLine(DateTime.Now + ":GetUpdatesFromPremiera");
 PremieraTcpClient client = new PremieraTcpClient();
 client.QueryItems.Add("QueryCode", QueryCode.GetUpdates.ToString());
 client.QueryItems.Add("ListType", "Movie;Hall;Session;Place;Delete");
 client.QueryItems.Add("Updates", _lastUpdateId);
 _lastUpdateId = String.Empty;
 var response = client.SendQuery();
 // here parse response
 //...
 }

This code works fine. But, now I have to load data from two remote app (tomorrow may be three).

The simple solution is to iterate through all remote apps:

private static void GetUpdatesFromPremiera()
{
 foreach(var remoteApp in listRemoteApp)
 {
 PremieraTcpClient client = new PremieraTcpClient();
 // here assigned different properties
 var response = client.SendQuery();
 }
}

Is there is a better way of doing it? Also, each time a connection is established, I think it impacts performance greatly.

• svick: I'm not sure I understand what the problem is. If you have a collection of items, using foreach to do something for each item is exactly what you should be doing. I certainly don't think it's ugly.
• user348173: @svick:ok, What you think about PremieraTcpClient?

Answer 1

I have some suggestions about PremieraTcpClient. The way it is written may lead to unreleased resources. If you have an error then you will remain with a stream and client opened. The correct way to do it is by using try...catch...finally or by using using. Below you can find the code using try catch finally

 private string Connect()
 {
 string responseData = string.Empty;
 TcpClient client = null;
 NetworkStream stream = null; 
 try
 {
 client = new TcpClient(ServerIp, Port);
 client.ReceiveBufferSize = Int32.MaxValue;
 Byte[] data = Encoding.GetEncoding(1251).GetBytes(ReadyQuery);
 stream = client.GetStream();
 // send data
 stream.Write(data, 0, data.Length);
 var sizeBuffer = new byte[10];
 stream.Read(sizeBuffer, 0, 10);
 var sizeMessage = int.Parse(Encoding.GetEncoding(1251).GetString(sizeBuffer, 0, 10));
 data = new Byte[sizeMessage];
 var readSoFar = 0;
 //read data
 while (readSoFar < sizeMessage)
 {
 readSoFar += stream.Read(data, readSoFar, data.Length - readSoFar);
 }
 responseData = Encoding.GetEncoding(1251).GetString(data, 0, data.Length);
 responseData = responseData.TrimStart('&');
 //stream.Close();
 //client.Close();
 //return responseData;
 }
 catch (ArgumentNullException e)
 {
 //return responseData = string.Format("ArgumentNullException: {0}", e);
 }
 catch (SocketException e)
 {
 //return responseData = string.Format("SocketException: {0}", e);
 }
 finally
 {
 if(stream!=null) stream.Close();
 if(client!=null) client.Close();
 }
 return responseData;
 }

And another small suggestion: I think is misleading to have a method named Connect that in fact does more than connect. It will be better to break the Connect method into smaller methods, each one with specific actions (even if they are private).