Using graphics to draw on form, in C#

Question

I have a form, which I want to draw on-top some shapes. To be exact, rectangles and lines.

This is the "Redraw" method. I call this method every 100ms from a different thread.

public void Redraw()
 {
 g.FillRectangle(b, new Rectangle(13, 13, (int)drawMapX + 2, (int)drawMapY + 2)); /* Draw background */
 try
 {
 foreach (Location drawPoint in points)
 { 
 g.DrawLine(new Pen(y), new Point((int)Math.Round(masterpos.X), (int)Math.Round(masterpos.Y)), new Point((int)Math.Round(drawPoint.X), (int)Math.Round(drawPoint.Y)));
 g.FillRectangle(r, drawPoint.X, drawPoint.Y, 2, 2);
 }
 g.FillRectangle(gr, masterpos.X, masterpos.Y, 2, 2); 
 g.DrawString(Math.Round(pos.X).ToString() + " " + Math.Round(pos.Y), drawFont, gr, masterpos.X + 2, masterpos.Y - 20, centered);
 }
 catch { }
 }

Where "g" is

 Graphics g = Application.OpenForms["Client"].CreateGraphics();

Nothing too interesting about it, draws some shapes in some given places, and some strings

My question: Is my thread safe and memory-leak proof?

void CreateDrawingThread() {new Thread(() => {
 Thread.CurrentThread.IsBackground = true;
 do
 {
 try
 {
 Thread.Sleep(100);
 Redraw();
 }
 catch(Exception ex) { Console.WriteLine(ex.ToString()); return; }
 } while (true);
 }).Start();}

Because as you can see, I don't do anything like g.Flush every redraw, I was wondering if it stores old "draws" in memory, or if I'm doing something in a cpu costraining way.

Answer 1

new Pen(y)

This should be disposed.

---

Graphics g = Application.OpenForms["Client"].CreateGraphics();

I wonder why are you getting the Graphics object like this? To redraw something you should actually handle the Paint event or override the OnPaint method where you get a ready to use Graphics object from the system. And of course like @Cody Gray said, you need to dispose a graphics object acquired this way.

---

Those try/catch blocks are suspicious too. There is nothing that could throw an exception. If this runs every 100ms there is no room for exception handling. This would be very bad for the performance.

• Cody Gray: Nailed it. Wrap Graphics objects in using statements, never call CreateGraphics, and stop doing Pokemon exception-handling (you don't need to catch 'em all).
• Denis: The cost of simply entering a try/catch block is negligible. The cost of THROWING an exception isn't, as you said probably an exception wont occur there, like ever, so it doesn't really hurts the performance all that much, tho it's a code smell nevertheless.
• Cody Gray: It is not about performance, @denis. It's about correctness. The code as shown is catching the exception and swallowing it silently. That prevents exceptions from being useful and undermines your ability to catch bugs. The only exceptions you should catch are those that you are specifically going to handle (which also means that you should catch a derived type, rather than Exception). Additionally, although lots of people use catch blocks to log exceptions, this is still a poor solution. It would be better just to add this logging to an unhandled exception handler (or use a logging FW).
• Denis: @CodyGray As I said it's a code smell nevertheless.. All I'm saying is that it wont hurt the performance all that much.
• t3chb0t: @CodyGray I think the OP is handling or rather swallowing some invalid coordinates there instead of fixing the bug.