4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – svick Apr 4 '12 at 10:26
  • \$\begingroup\$ @svick:ok, What you think about PremieraTcpClient? \$\endgroup\$ – user348173 Apr 4 '12 at 11:04
4
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ +1 for unreleased resource. I forgot it. \$\endgroup\$ – user348173 Apr 5 '12 at 6:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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