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.

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