Earlier this academic year I had an assignment to take a project I had written outside the context of University and apply a number of refactorings to it.

I chose a program I had written a while ago that saves periodic 24-bit lossless bitmap screenshots of a user-specified target window.

BMPTimer Class:

// Header for BMPTimer
#pragma once
#include <Windows.h>
#include "BMPWriter.h"
 * BMPTimer class - a timer for periodically saving screenshots of a specified window.
class CBMPTimer
    int ticks_so_far; // New variable added with refactor #3 - keeps track of ticks made by our timer
    int ms_interval; // Inteval between clicks
    HWND hwTarget; // Target window
    HANDLE hThread; // Thread for timer tick
    LPVOID thistimer; // Pointer to "this" class instance
    static bool timeractive; // Flag for exitting the timer thread
    // Stub that invokes threadproc (required for using a callback function belonging to a class)
    static DWORD StaticThreadProc(LPVOID);
    // Actual thread procedure
    DWORD ThreadProc(LPVOID);
    // Sets the exit flag condition for the thread proc
    void StopTimer();
    // Constructor/deconstructor
    CBMPTimer(HWND, int);

#include "BMPTimer.h"
bool CBMPTimer::timeractive;
 * Class constructor for CBMPTimer
 * Takes a handle to the target window for screenshotting
 *  and an interval in milliseconds.
CBMPTimer::CBMPTimer(HWND hWnd, int ms)
    // Assign member variables
    hwTarget = hWnd;
    ms_interval = ms;
    // This cast is necessary so that the ThreadProc function can be called from a static stub
    thistimer = reinterpret_cast<LPVOID>(this);
    hThread = CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)&StaticThreadProc, thistimer, 0, NULL);
    // Set other member variables...
    timeractive = true;
 * Class deconstructor for CBMPTimer
 * Frees dynamically allocated memory
    delete thistimer;
// Stub method that invokes ThreadProc.
// Necessary so that ThreadProc can belong to the class and also be specified as a callback function.
DWORD CBMPTimer::StaticThreadProc(LPVOID lpParam)
    CBMPTimer *pThis = reinterpret_cast<CBMPTimer*>(lpParam);
    return pThis->ThreadProc(lpParam);
// Actual thread procedure.
// Uses a CBMPWriter object to save a bitmap of the target window as long as the flag to keep running stays on.
DWORD CBMPTimer::ThreadProc(LPVOID lpParam)
    while (timeractive)
        CBMPWriter bmpW(hwTarget, ++ticks_so_far);
    return EXIT_SUCCESS;

BMPWriter class:

#pragma once
#include <Windows.h>
#include <tchar.h>
#include <string>
using namespace std;
 * BMPWriter class - a file writer for bitmap screenshots of a given window.
class CBMPWriter
    char *data; // Stores the array of bits representing a bitmap of a window
    int len_data; // Length of data
    //char fname[MAX_PATH]; // Filename to write out
    BITMAPFILEHEADER bfHeader; // Bitmap file header structure
    BITMAPINFOHEADER biHeader; // Bitmap file header #2 structure
    BITMAPINFO bInfo; // Bitmap file header #3 structure
    HBITMAP hBitmap; // Handle to the bitmap object
    HWND hwTarget; // Target window
    HDC windowDC, tempDC; // Device context for target window and temporary copy in memory
    int count; // Refactor #3 - generate file names internally rather than use lazy class CFileNameGenerator
    int xd, yd; // x and y dimensions of bitmap
    char fname[MAX_PATH]; // Filename to write out
    // Generate next file name of bitmap file - refactored out of CFileNameGenerator
    string NextFileName();
    // Initialization functions for the 3 bitmap file header/signature structs
    void InitFH();
    void InitIH();
    void InitInfo();    
    // Class constructor - calls the initialization functions
    CBMPWriter(HWND, int);
    // Deconstructor - cleans up GDI objects and frees memory
    // Performs the actual bitmap file write
    void SaveBMP();

#include "BMPWriter.h"

// Generates the next filename to write out the bitmap data to.
string CBMPWriter::NextFileName()
    string next_filename;
    int temp = count;
    // Get the number of digits in the count variable by using logarithm base 10
    int num_digits = (int)log10(temp)+1;
    char tmp[32];
    // Write out the ascii value of the count into the string
    for(int i = 0; i < num_digits; i++)
        tmp[num_digits-i-1]=(temp%10)+'0'; // digits 0-9 plus ascii '0' = char representation
        temp/=10; // divide by 10 to process next place-value
    tmp[num_digits]=0; // terminate string with null char
    // create final filename string and return it.
    string countstr = tmp;
    return next_filename;

/** Initalizes the fields in the bitmap file header - file signature, header size, etc... */
void CBMPWriter::InitFH()
    bfHeader.bfType = 0x4d42;  // magic number- identifies the file as a valid bitmap to image viewers etc.
    bfHeader.bfSize = 0;     // This and other 3 are reserved and always 0
    bfHeader.bfReserved1 = 0;
    bfHeader.bfReserved2 = 0;
    bfHeader.bfOffBits = sizeof(BITMAPFILEHEADER)+sizeof(BITMAPINFOHEADER);  // Specifies the number of bytes occupied by the headers (before the actual array of bits/pixels begins)

/** Initalizes the bitmap info header structure - bit intensity, planes, dimensions etc... */
void CBMPWriter::InitIH()
    biHeader.biSize = sizeof(biHeader);
    biHeader.biBitCount = 24; // 24-bits = full intensity/lossless RGB image of window
    biHeader.biCompression = BI_RGB; // No compression
    biHeader.biHeight = yd; // dimensions
    biHeader.biWidth = xd;
    biHeader.biPlanes = 1; // number of planes
    biHeader.biSizeImage = 0; // all reserved fields below, must be 0
    biHeader.biXPelsPerMeter = 0;
    biHeader.biYPelsPerMeter = 0;
    biHeader.biClrImportant = 0;
    biHeader.biClrUsed = 0;
// Initializes the bInfo structure (not a file header but necessary for various bitmap WinAPI functions)
void CBMPWriter::InitInfo()
    bInfo.bmiHeader = biHeader;

CBMPWriter::CBMPWriter(HWND window, int tick)
    // Set local variables...
    RECT r;
    hwTarget = window;
    count = tick;
    // Get the file name to save for this instance of CBMPWriter
    strcpy(fname, NextFileName().c_str());
    // Get dimensions...
    GetWindowRect(hwTarget, &r);
     xd = r.right - r.left;
     yd = r.bottom - r.top;
    // Initialize file/bitmap headers and signatures etc

    // Calculate length of data.
    len_data = ((((24 * xd + 31)&(~31)) / 8)*yd);
    // Generate dynamic char (byte) array based on length.
    data = new char[len_data];
    // Get the device context of the target window
    windowDC = GetWindowDC(hwTarget);
    // Create a new device context *in memory* with identical properties as the window DC
    tempDC = CreateCompatibleDC(windowDC);
    // Get the bitmap of the window
    hBitmap = CreateDIBSection(windowDC, &bInfo, DIB_RGB_COLORS, (void**)&data, 0, 0);
    // Select it as the GDI object we want to use
    SelectObject(tempDC, hBitmap);
    // And copy it into the memory device context 
    BitBlt(tempDC, 0, 0, xd, yd, windowDC, 0, 0, SRCCOPY);

 * Class deconstructor for CBMPWriter
 * Deletes GDI objects, frees dynamic memory, etc. to prevent memory leaks
    ReleaseDC(hwTarget, windowDC);

// Performs the actual write of the bitmap.
void CBMPWriter::SaveBMP()
    // Create a file (if we can)
    if (hFile)
        // If we can, write out the headers, then write out the bitmap
        DWORD dwWritten = 0;
        WriteFile(hFile, &bfHeader, sizeof(BITMAPFILEHEADER), &dwWritten, NULL);
        WriteFile(hFile, &biHeader, sizeof(BITMAPINFOHEADER), &dwWritten, NULL);
        WriteFile(hFile, data, len_data, &dwWritten, NULL);
    else // If not, tell the user, and exit the class function
        MessageBox(0, _T("Could not open the file."), _T("Error"), MB_OK | MB_ICONSTOP);
    // Free handle from memory.

MsgHandler class:

#pragma once
#include <Windows.h>
#include "BMPTimer.h"
 * CMsgHandler class - does the "dirty work" of the window procedure (handle messages etc).
 * Refactor #4 - existence of this class
class CMsgHandler
    // The *target window* of the bitmap captures. Must be static as this class is created/deconstructed for each message posted to the queue.
    static HWND hwTarget;
    // The child windows of the parent screenshot taking app window.
    HWND hWnd, hwCmdSource, hStartBtn, hStopBtn, hSelect, hStatic2;
    // The identifier of the message (WM_whatever)
    UINT Msg;
    // WPARAM and LPARAM passed to the message 
    WPARAM wParam;
    LPARAM lParam;
    // Bitmap timer class (necessary on start call)
    CBMPTimer *bmpT;
    // Return value
    LRESULT return_val;
    // Refactor #5 - remove the various redundant calls to FindWindowEx with all but 1 identical parameters
    inline HWND FindWindowQ(LPCWSTR className, LPCWSTR winText);
    // The various message handlers.
    VOID Handle_Close();
    VOID Handle_Destroy();
    VOID Handle_Command();
    VOID Handle_Generic();

    // Sub-handlers for WM_COMMAND based on command source
    VOID Handle_Start();
    VOID Handle_Stop();
    VOID Handle_Select();

    // Return the return value (required for WindowProc stub)
    LRESULT Get_Return();

    // Constructor/deconstructor
    CMsgHandler(HWND hWnd, UINT Msg, WPARAM wParam, LPARAM lParam, HWND hStaticW);

#include "MsgHandler.h"
// Macros for eliminating duplicated code - simplifies method calls to FindWindowEx
#define FINDBUTTON(wtxt) (FindWindowQ((TEXT("BUTTON")), (L##wtxt)))
#define FINDEDIT (FindWindowQ((TEXT("EDIT")), (NULL)))
HWND CMsgHandler::hwTarget;
// Inline method for handling calls to FindWIndowEx - removes necessity for several duplicated calls etc
inline HWND CMsgHandler::FindWindowQ(LPCWSTR className, LPCWSTR winText)
    return FindWindowEx(hWnd, NULL, className, winText);

// Handles the WM_CLOSE message (destroys the window, return 0)
VOID CMsgHandler::Handle_Close()
    return_val = (LRESULT)0;

// Handles the WM_DESTROY message (posts the exit code for the window; return 0)
VOID CMsgHandler::Handle_Destroy()
    return_val = (LRESULT)0;

// Stub method to handle all calls not specifically supported by our class (call DefWindowProc)
VOID CMsgHandler::Handle_Generic()
    return_val = DefWindowProc(hWnd, Msg, wParam, lParam);

// Handles the WM_COMMAND message
VOID CMsgHandler::Handle_Command()
    // Step 1: identify the source of the command (which button)
     hwCmdSource = (HWND)lParam;
    // Step 2: pass to the appropriate sub-handle for WM_COMMAND based on the button
    if (hwCmdSource == hSelect)
    if (hwCmdSource == hStartBtn)
        if (hwTarget == NULL)
            MessageBox(hWnd, TEXT("Please first select a target window!"), TEXT("Problem"), MB_OK | MB_ICONWARNING);
            return_val = (LRESULT)0;
    if (hwCmdSource == hStopBtn)
    return_val = (LRESULT)0;

// Class constructor for CMsgHandler 
//  sets up member variables and determines which message handler to use
CMsgHandler::CMsgHandler(HWND ahWnd, UINT aMsg, WPARAM awParam, LPARAM alParam, HWND hStaticW)
    hWnd = ahWnd;
    Msg = aMsg;
    wParam = awParam;
    lParam = alParam;

    hStartBtn = FINDBUTTON("Start");
    hStopBtn = FINDBUTTON("Stop");
    hSelect = FINDBUTTON("Select Window");

    hStatic2 = hStaticW;
    if (Msg == WM_CLOSE) Handle_Close();
    else if (Msg == WM_DESTROY) Handle_Destroy();
    else if (Msg == WM_COMMAND) Handle_Command();
    else Handle_Generic();

// Sub handler for "select window" WM_COMMAND message source
// Asks the user for the window they want to target,
//  prints out info about target window on app window
VOID CMsgHandler::Handle_Select()
    MessageBox(0, TEXT("If you are targetting a usual Windows application, try to target the *title bar* of the application.  If you are targetting a Java or Flash applet target anywhere within the applet.\n\nPress OK and then hover your mouse over the target window within 2 seconds... (before pressing OK, you can move this message box closer to your target if need be)"), TEXT("Info"), MB_OK | MB_ICONINFORMATION);
    ShowWindow(hWnd, SW_MINIMIZE);
    POINT p;
    hwTarget = WindowFromPoint(p);
    TCHAR wndTxt[255];
    SendMessage(hwTarget, WM_GETTEXT, 255, (LPARAM)wndTxt);
    TCHAR sttcbuf[1024];
    RECT wRect;
    GetWindowRect(hwTarget, &wRect);
    wsprintf(sttcbuf, TEXT("Target Window: %s\n(handle = 0x%.8X; rect=(%d,%d)-(%d,%d))"), wndTxt, hwTarget, wRect.left, wRect.top, wRect.right, wRect.bottom);
    SetWindowText(hStatic2, sttcbuf);
    ShowWindow(hWnd, SW_RESTORE);


// Handles the "Start" WM_COMMAND message
// 1. Toggles the enabled state of start and stop button
// 2. intializes a CBMPTimer with the specified target and specified interval.
VOID CMsgHandler::Handle_Start()
    EnableWindow((HWND)lParam, FALSE);
    EnableWindow(FINDBUTTON("Stop"), TRUE);
    int len = GetWindowTextLength((FINDEDIT)) + 1;
    TCHAR *lenBuf = new TCHAR[len];
    GetWindowText(FINDEDIT, lenBuf, len);
    int ms = _wtoi(lenBuf);
    if (ms == 0)ms++;
    bmpT = new CBMPTimer(hwTarget, ms);

// Handles a "Stop" WM_COMMAND message
// 1. Stops the running CBMPTimer 
// 2. Toggles the enabled state of start and stop button

VOID CMsgHandler::Handle_Stop()
    EnableWindow((HWND)lParam, FALSE);
    EnableWindow(FINDBUTTON("Start"), TRUE);


// Returns the stored LRESULT to WindowProc stub.
LRESULT CMsgHandler::Get_Return()
    return return_val;

WinMain (global namespace):

// The below files include my self-written classes for this project.
#include "BMPWriter.h"
#include "BMPTimer.h"
#include "MsgHandler.h"
const TCHAR szClassName[] = TEXT("ScreenSnapperWnd");
// The below two files allow for necessary Windows API and Process API functions in this project
#include <Windows.h>
#include <Psapi.h>
#include "Global.h"
HWND hwTarget = (HWND)0x0000000, hStaticWI = (HWND)0x00000000, hWnd, hStart, hStop, hFindTarget, hTimerMS, hStaticE;

/** Refactor #1:
 * Given the length of WinMain, extract some methods from it, namely, the initialization of the WNDCLASSEX structure 
ATOM RegisterWindowClass(HINSTANCE hInstance)
    WNDCLASSEX wcex;

    wcex.cbSize = sizeof(WNDCLASSEX);
    wcex.hbrBackground = (HBRUSH)COLOR_WINDOW;
    wcex.hCursor = LoadCursor(NULL, IDC_ARROW);
    wcex.hIcon = LoadIcon(NULL, IDI_APPLICATION);
    wcex.hIconSm = LoadIcon(NULL, IDI_APPLICATION);
    wcex.hInstance = hInstance;
    wcex.lpszMenuName = NULL;
    wcex.lpszClassName = szClassName;
    wcex.lpfnWndProc = WindowProc;
    wcex.cbClsExtra = 0;
    wcex.cbWndExtra = 0;

    return RegisterClassEx(&wcex);

 * Still Refactor #1 - extract child Window initialization from WinMain into a new method.
VOID CreateChildWindows()
    hStaticE = CreateWindow(TEXT("STATIC"), TEXT("Enter the interval between screenshots (milliseconds):"), WS_RCHILD, 0, 0, 270, 20, hWnd, NULL, GetModuleHandle(NULL), NULL);
    hTimerMS = CreateWindowEx(WS_EX_CLIENTEDGE, TEXT("EDIT"), TEXT("5000"), WS_RCHILD | ES_NUMBER, 0, 22, 270, 20, hWnd, NULL, GetModuleHandle(NULL), NULL);
    hFindTarget = CreateWindow(TEXT("BUTTON"), TEXT("Select Window"), WS_RCHILD | BS_TEXT, 10, 44, 250, 30, hWnd, NULL, GetModuleHandle(NULL), NULL);
    hStaticWI = CreateWindow(TEXT("STATIC"), TEXT(""), WS_RCHILD | SS_LEFT, 0, 88, 270, 40, hWnd, NULL, GetModuleHandle(NULL), NULL);
    hStart = CreateWindow(TEXT("BUTTON"), TEXT("Start"), WS_RCHILD | BS_TEXT, 10, 130, 125, 30, hWnd, NULL, GetModuleHandle(NULL), NULL);
    hStop = CreateWindow(TEXT("BUTTON"), TEXT("Stop"), WS_RCHILD | BS_TEXT | WS_DISABLED, 135, 130, 125, 30, hWnd, NULL, GetModuleHandle(NULL), NULL);
 * WinMain function - entry point for the application
int APIENTRY WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nShowCmd)

    MSG Msg;

        MessageBox(0, L"Window Registration Failed!", L"Error", MB_OK|MB_ICONSTOP);
        return -1;
    hWnd = CreateWindowEx(WS_EX_OVERLAPPEDWINDOW, szClassName, TEXT("Screenshot Taker"), WS_VISIBLE | WS_SYSMENU, 100, 100, 290, 210, NULL, NULL, GetModuleHandle(NULL), NULL);

    ShowWindow(hWnd, SW_SHOW);
    EnumChildWindows(hWnd, SysFontProc, 0);
    while (GetMessage(&Msg, NULL, 0, 0) > 0)
    return Msg.wParam;
// Callback function that sets all child windows of the screenshot taker window to the regular system font
    HFONT hfDefault = (HFONT)GetStockObject(DEFAULT_GUI_FONT);
    SendMessage(hWnd, WM_SETFONT, (WPARAM)hfDefault, 0);
    return TRUE;
// Window procedure stub - required to be passed to WNDCLASSEX but invokes CMsgHandler to do all of the actual work
    CMsgHandler msghandler(hWnd, Msg, wParam, lParam, hStaticWI);
    return msghandler.Get_Return();

The .h files for the classes are first; the .cpp follows with the actual function bodies etc.

Though my grade on this assignment was good (95%), I'm curious if anyone can offer an outside opinion on the quality of my post-refactored code: is there anything that still smells bad in this code? What can be improved further? Is this something I can safely call 'well-written'?

  • 1
    \$\begingroup\$ Did you test it with an OpenGl window? In my experience they do not get captured with a simple BitBlt. \$\endgroup\$ – nwp Nov 29 '14 at 15:10

Quite a bit of code here, so this review is probably not complete:

Coding style / Miscellaneous:

  • Please keep parameter names on function prototypes. Parameter names are a form of self documentation of the code.

  • I would like to see a blank line between each function, both in the header declarations and in the implementation. Your code is a bit cluttered in some places.

  • Aligning things in a block such as this might also help readability (from CBMPTimer):

    int         ticks_so_far; // New variable added with refactor #3 - keeps track of ticks made by our timer
    int         ms_interval;  // Inteval between clicks
    HWND        hwTarget;     // Target window
    HANDLE      hThread;      // Thread for timer tick
    LPVOID      thistimer;    // Pointer to "this" class instance
    static bool timeractive;  // Flag for exitting the timer thread
  • This is arguably personal preference, but I think it makes sense placing the public section of a class first. Since we read a file from top to bottom, things at the top have more visibility. When a user of your class needs to consult a header file, what should have more visibility, private variables / implementation details, or the public interface?

  • using namespace std; in a header file: No, no. Read this for more.

  • Poor spacing at places, for instance:


    Always put spaces between an assignment or arithmetical operator:

    next_filename = "shot_";
    next_filename += countstr;
    next_filename += ".bmp";
  • char fname[MAX_PATH] should be an std::string.

  • Use more smart pointers. Manual memory management is a dated practice that has proved time after time to lead to memory corruption bugs, dangling pointers and memory leaks.

  • Be more mindful of your naming convention. You have mixed naming for variables, some are using camelCase, like in hwTarget while others are using snake_case, like in ticks_so_far. Choose one notation and stick to it. Consistency is very important.

  • If you can use C++11, then prefer the new nullptr over the NULL macro.

A few security problems and potential bugs:

  • This is not technically legal in C++:

    hThread = CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)&StaticThreadProc, thistimer, 0, NULL);

    You are passing a static class method (CBMPTimer::StaticThreadProc) as the thread's start routine. CreateThread() expects a C function using the "WINAPI" (STD Call) calling convention. You have fixed that by applying a very unsafe cast to the function pointer. This is a crash waiting to happen. You have to declare the thread function as an extern C function using the adequate calling convention:

    extern "C" {
    DWORD WINAPI MyThreadProc(LPVOID lpParam)
        CBMPTimer *pThis = reinterpret_cast<CBMPTimer*>(lpParam);
        return pThis->ThreadProc(lpParam);
    } // extern C
  • delete thistimer;, I don't even know how that is compiling. thistimer is a void pointer (LPVOID). delete shouldn't be able to delete void* because it has no type. This is probably being allowed by a Microsoft extension to the standard library.

  • But thistimer get worse. It is actually a pointer to a CBMPTimer instance, initialized in CBMPTimer's constructor:

    thistimer = reinterpret_cast<LPVOID>(this);

    So by trying to delete it in the destructor, you are attempting to delete a pointer to the class that is already being destroyed. This is 100% Undefined Behavior.

  • Manually calling a destructor:

    while (timeractive)
        CBMPWriter bmpW(hwTarget, ++ticks_so_far);
        bmpW.~CBMPWriter(); // <--- Not a good idea and also unnecessary.

    Doing this is a bad idea. There are only a few rare cases where you'll want to manually call a destructor. This is not one. The object is already going out of scope, so it will be destroyed on the next line.


Few things surely do not look right.

  • ~CBMPTimer tries to delete an int. Good luck. It also doesn't stop the thread.

  • Having no BMPWriter.h to review I may only suppose what log10 is supposed to do. I also suppose that there's no place for floating point here, especially since num_digits has no relation to a capacity of tmp.

  • It is very strange to see NextFileName be called from the constructor.

  • I am sure that 0x4d42 has a proper name as a macro value (even a plain BM will do).

  • \$\begingroup\$ Added the header files - hope that clears some things up. thistimer is actually a pointer of type void (LPVOID) \$\endgroup\$ – Govind Parmar Nov 29 '14 at 8:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.