Would this code cause memoryleaks?

Question

My question is really simple. Does this code cause memoryleaks? If so, where/how/why?

HDC hDC, memDC = 0;
HBITMAP hBMP = 0;
HBITMAP hOldBMP = 0;
PAINTSTRUCT PS;
HBRUSH hb212121, hb141414, hb070707, hb000, hbF7F7F7, hb989898, hb707070, hb494949, hb984921 = 0;
HPEN hp353535 = 0;

case WM_PAINT:
        hDC = BeginPaint(hWnd, &PS);
        memDC = CreateCompatibleDC(hDC);
        hBMP = CreateCompatibleBitmap(hDC, 450, 450);
        SelectObject(memDC, hBMP);

        hb212121 = CreateSolidBrush(RGB(33, 33, 33));
        FillRect(memDC, &rMainClntNoBorder, hb212121);

        hb494949 = CreateSolidBrush(RGB(73, 73, 73));
        hb984921 = CreateSolidBrush(RGB(152, 73, 33));
        hb000 = CreateSolidBrush(RGB(0, 0, 0));
        switch(tiles)
        {
            setTileRect();
            case 1:
                FillRect(memDC, &rTile, hb494949);
                break;
            case 2:
                FillRect(memDC, &rTile, hb984921);
                break;
            case 7:
                FillRect(memDC, &rTile, hb000);
                break;
        }
        SelectObject(memDC, hOldBMP);
        DeleteObject(hBMP);
        DeleteObject(hb984921);
        DeleteObject(hb494949);
        DeleteObject(hb212121);
        DeleteObject(hb000);

        hp353535 = CreatePen(PS_SOLID, 1, RGB(53, 53, 53));
        SelectObject(memDC, hp353535);
        GetClientRect(hWnd, &rClnt);
        MoveToEx(memDC, rClnt.left, rClnt.bottom - 1, 0);
        LineTo(memDC, rClnt.left, rClnt.top);
        LineTo(memDC, rClnt.right, rClnt.top);
        DeleteObject(hp353535);

        BitBlt(hDC, 0, 0, rMainClntNoBorder.right, rMainClntNoBorder.bottom, memDC, 0, 0, SRCCOPY);
        DeleteDC(memDC);

        EndPaint(hWnd, &PS);
        break;

Answer 1

• you select the pen in, but don't select out before delete.

  SelectObject(memDC, hp353535);
  // snip
  DeleteObject(hp353535)
  

  should be:

  hOldPen = SelectObject(memDC, hp353535);
  // snip
  SelectObject( memDC, hOldPen );
  DeleteObject(hp353535)
  

• SelectObject(memDC, hBMP); should really be hOldBMP = SelectObject(memDC, hBMP);

For some other comments:

• I recommend that you split the WM_PAINT (and all other messages) processing into its own function. It'll be easier to read/debug.

• Are rMainClntNoBorder (and rTile, rClnt) globals, or declared and initialised before the code supplied?

• Only declare one variable per line. It's easier to read and find where it's declared rather than searching. It also avoids an issue with pointers that you may come across.

• Use proper names for the brushes if you can - hbDarkOrange (or even hbCurrentActiveTile) means more than hb984921.

• Unless you have hundreds of colours, it may be better to create them at the start of the program and delete them at the end, and store their values in a struct in the 'user-area' of the window structure - You may want to avoid creating and deleting them continually.

• If you don't do the above, keep the creation and deletion as close together as possible.

  hbDarkGrey = CreateSolidBrush(RGB(33, 33, 33));
  FillRect(memDC, &rMainClntNoBorder, hbDarkGrey);
  DeleteObject( hbDarkGrey );
  

• Similarly, your switch statement could be simplified. Rather than creating three brushes and using one, just pick the one you want.

  setTileRect(); // implies that rTile is a global??
  switch(tiles)
  {
      case 1:
          tile_colour = RGB( 73, 73, 73 );
          break;
      case 2:
          tile_colour = RGB( 152, 73, 33 );
          break;
      case 7: // allow fallthrough
      default: // you need a default!!
          tile_colour = RGB( 0, 0, 0 );
          break;
  }
  hbTile = CreateSolidBrush( tile_colour );
  FillRect( memDC, &rTile, hbTile );
  DeleteObject( hbTile );
  

• Instead of 1 and 2 etc in the case statement, create some meaningful constants for them with enum.