TCP connection and reconnection

Question

Is there anyway to make this piece of code more elegant? It's not a nice view, but I can't really see a way to improve the looks of it or shorten it.

private void Send(CancellationToken CTSSend)
{
 try
 {
 NativeMethods.Rect rc;
 NativeMethods.GetWindowRect(hwnd, out rc);
 IntPtr dc1;
 IntPtr dc2 = NativeMethods.GetWindowDC(hwnd);
 using (tcp = new TcpClient())
 {
 tcp.NoDelay = true;
 while (!tcp.Connected && !CTSSend.IsCancellationRequested)
 {
 try
 {
 if (!tcp.Connected)
 tcp.Connect(adress);
 }
 catch (Exception e)
 {
 if (e is SocketException) { Console.WriteLine(e.Message); }
 else
 MessageBox.Show(e.Message + " Tcp Connect : Send");
 }
 }
 using (EncoderParameters JpegParam = new EncoderParameters())
 using (JpegParam.Param[0] = new EncoderParameter(System.Drawing.Imaging.Encoder.Quality, 50L))
 using (Bitmap bmp = new Bitmap(rc.Width, rc.Height, System.Drawing.Imaging.PixelFormat.Format32bppArgb))
 using (Graphics g = Graphics.FromImage(bmp))
 using (var ms = new MemoryStream())
 using (var tcpStream = tcp.GetStream())
 while (tcp.Connected && !CTSSend.IsCancellationRequested)
 {
 try
 {
 dc1 = g.GetHdc();
 NativeMethods.BitBlt(dc1, 0, 0, rc.Width, rc.Height, dc2, 0, 0, 13369376);
 g.ReleaseHdc(dc1);
 bmp.Save(ms, jpeg, JpegParam);
 if (bsize != ms.Length)
 {
 bsize = ms.Length;
 tcpStream.Write(BitConverter.GetBytes(bsize), 0, 4);
 tcpStream.Write(ms.GetBuffer(), 0, (int)bsize);
 }
 ms.SetLength(0);
 }
 catch (Exception e)
 {
 if (e is IOException)
 {
 try { connect.Invoke(new Action(() => { connect.Text = "Connect"; })); }
 catch (InvalidOperationException) { return; }
 capcon = false;
 }
 else
 MessageBox.Show(e.Message + ": Send Error");
 }
 }
 }
 }
 catch (Exception e)
 {
 MessageBox.Show(e.Message + "SEND");
 }
}

I am trying to find a nice way to put TCP reconnecting in another class, but I found a limitation where I have to call return in the main class.

internal static void TCPReconnect(CancellationToken CTS, TcpClient tcp, IPEndPoint adress)
{
 tcp.NoDelay = true;
 while (!tcp.Connected && !CTS.IsCancellationRequested)
 {
 try
 {
 if (!tcp.Connected)
 tcp.Connect(adress);
 }
 catch (Exception e)
 {
 if (e is SocketException) { Console.WriteLine(e.Message); }
 else
 MessageBox.Show(e.Message + " Tcp Connect : Send");
 }
 }
}

So, when it fails, it goes out of the loop. I can also make it call Return, but it will do the same thing.

In the class Caller I have to use:

FastMethods.TCPReconnect(CTS, tcp, adress);
if (CTS.IsCancellationRequested)
 return;

As you can see, I have to recheck for CTS. Is it possible to have it inside the other class somehow?

If I use:

if (CTS.IsCancellationRequested)
 return;

It will simply return to the main class and continue.

• svick: First of all, you should apply separation of concerns. Encoding images and network communication shouldn't be in the same method, probably not even in the same class.
• Zerowalker: that would probably slow things down, as i am sending the image i take. If i seperate this, it would get out of order.
• radarbob: "that would probably slow things down" - A classic illustration of premature optimization mindset. The original question addresses a clear problem with the code and I assert debugging and maintenance in the current state will be much more of a problem than presumed performance degradation caused by refactoring. @svick's advice is sound. I suggest you start by "extracting to methods". As methods it is more flexible to re-work - getting to separation of concerns and manage the concern about "out of order."
• Zerowalker: I had them in Methods before, but i don´t really like having codes all over the place when i can have then at the same place. Though if i reuse the code in alot of places, i do have them in methods. But here, there is really no reason for me to put it in a method or separate class. I can however put the Image Encoding in a separate thread.
• Zerowalker: Though, again, I can put the TCP Reconnecting in a Method, as that just takes place for no big reason.

Answer 1

A couple of comments:

• Shortening code is not a goal - readability and maintainability is (which may, but not neccessarily, shorten it).
• The code mixes net code, graphics code, encoder code and UI code. Separating these into separate functions/classes will be very good. Just remember that (as a general rule), one function should do one thing.

• Always use braces, even when it's for a single statement where it's not required.

  if (!tcp.Connected)
  {
   tcp.Connect(adress);
  }
  

• Similarly, I'd suggest putting brackets around the !xxx in the if statement. Not needed, but can aid understandability.

  while ((!tcp.Connected) && (!CTS.IsCancellationRequested))
  

• What does 13369376 represent? Define the set of constants (SRCCOPY etc) that you're using and then use those.
• TcpClient.Connect is a blocking call - depending on your use, you may want to consider rewriting using BeginConnect instead. One problem is that when in the middle of a connect, you can not check CTSSend for a cancellation.
• You need to rethink your exception handling. At the moment, your code puts up a message box, requiring a response. You do not want to mix UI and non-UI code like this
• The exception-catching occurs in the middle of a loop, so if it happens once, it's likely to happen again and again...each time requiring acknowledgement of the message box.
• Catch specific exceptions (SocketException) rather than general ones (Exception). You can have multiple catches if you need them.
• If you're using bsize to determine if the screen has changed, it'll be unreliable. You might end up with two images that have the same size even if the images are different.
• The code grabs images as quickly as possible, possibly discarding most of them (if my understanding is correct) - this seems uneccessary. Why not grab an image (say) every 1 second? In which case you can set up something like a System.Timers.Timer instance and get the image when that is triggered.
• You should probably allocate a new MemoryStream, rather than reusing it. If you're doing that to "save time", it 'probably' doesn't matter compared with the speed of network traffic and image conversion.
• What happens when the window size changes/minimized etc, or is it guaranteed to have a fixed size?
• In regards to your edit: Rather than a void return, either return a bool or (preferably) define an enum (CancelRequested, Connected) and return that.

Edit in response to (first) comment:

1. If you have 10 stacked braces at the end of the method, the method is too long. Break it up rather than remove the braces.
2. re MemoryStream - it'll gain readability. One thing though that you may want to do is to set an initial capacity (say, rc.Width*rc.Height) - MemoryStream itself reallocates memory/copies data on resizing.
3. Have you tried using a different image format, say png, rather than jpeg. It may be quicker to compress or smaller, depending on the type of image.
4. You could also try spliting the image up into quandrants/etc, compressing & sending those and reassembling them at the other side. It may be overall quicker.
5. And of course, if you want 'quicker', always base it on actual timings rather than feelings :)

• Zerowalker: Very well written, some nice things to think about. I though prefer to not use Brackets, as i hate when there are like 10 stacked brackets ad the end of the Method. And the MemoryStream, i see no reason remaking it if i don´t earn anyhing on it, As the thing is, i need to take images as fast as possible, and currently, The Network does Not limit it, there is always one image in, one image out, Delation only occured if i use .BMP (the images goes to fast then, and are to large, And i can´t prevent duplicated send images).
• dreza: Not sure about the always use brackets suggestion. That's a bit restrictive I think, although I must admit when in doubt I would probably always use them
• Zerowalker: Well, it´s hard to break up, as i need all of them to do one thing. It´s a very bad weird thing, for example, to use Jpeg encoding, you have to have 2 Usings, 1 for the encoding parameter, and 1 for settings that parameter. That´s just irritating, but needed., Interesting idea to divide the image, may be worth looking into.