Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
share|improve this question
    
Have you seen anything to suggest that this code does cause a memory leak? –  aroth Feb 3 '13 at 6:10
add comment

1 Answer

up vote 3 down vote accepted
  • 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.

share|improve this answer
    
An excellent answer, I'll be sure to use these tips. I'll probably have some questions later on, so I'll post comments then. Thanks. –  user19953 Feb 3 '13 at 14:22
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.