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{private:    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 threadpublic:    // 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);    ~CBMPTimer();};#include BMPTimer.hbool 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;    ticks_so_far=0;}/** * Class deconstructor for CBMPTimer * Frees dynamically allocated memory */CBMPTimer::~CBMPTimer(){    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)    {        Sleep(ms_interval);        CBMPWriter bmpW(hwTarget, ++ticks_so_far);        bmpW.SaveBMP();        bmpW.~CBMPWriter();    }    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{private:    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 bitmappublic:    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    ~CBMPWriter();    // Performs the actual bitmap file write    void SaveBMP();};#define _CRT_SECURE_NO_WARNINGS#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;    next_filename=shot_;    next_filename+=countstr;    next_filename+=.bmp;    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;    count=0;    // Initialize file/bitmap headers and signatures etc    InitFH();    InitIH();    InitInfo();    // 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 */CBMPWriter::~CBMPWriter(){    DeleteDC(tempDC);    ReleaseDC(hwTarget, windowDC);    DeleteObject(hBitmap);}// Performs the actual write of the bitmap.void CBMPWriter::SaveBMP(){    // Create a file (if we can)    HANDLE hFile = CreateFileA(fname, GENERIC_READ | GENERIC_WRITE, 0, NULL, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, NULL);    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.    CloseHandle(hFile);}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{private:    // 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;public:    // 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);    ~CMsgHandler();};#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 etcinline 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(){    DestroyWindow(hWnd);    return_val = (LRESULT)0;}// Handles the WM_DESTROY message (posts the exit code for the window; return 0)VOID CMsgHandler::Handle_Destroy(){    PostQuitMessage(0);    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 messageVOID 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)    {        Handle_Select();    }    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;            return;        }        Handle_Start();    }    if (hwCmdSource == hStopBtn)    {        Handle_Stop();    }    return_val = (LRESULT)0;}// Class constructor for CMsgHandler //  sets up member variables and determines which message handler to useCMsgHandler::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 windowVOID 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);    Sleep(2000);    POINT p;    GetCursorPos(&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 buttonVOID CMsgHandler::Handle_Stop(){    bmpT->StopTimer();    EnableWindow((HWND)lParam, FALSE);    EnableWindow(FINDBUTTON(Start), TRUE);}CMsgHandler::~CMsgHandler(){}// 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.hconst 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.hHWND 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.style = CS_BYTEALIGNWINDOW | CS_BYTEALIGNCLIENT | CS_GLOBALCLASS;    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. */#define WS_RCHILD (WS_VISIBLE | WS_CHILD)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;    if(!RegisterWindowClass(hInstance))    {        MessageBox(0, LWindow Registration Failed!, LError, 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);    CreateChildWindows();    ShowWindow(hWnd, SW_SHOW);    EnumChildWindows(hWnd, SysFontProc, 0);    UpdateWindow(hWnd);    while (GetMessage(&Msg, NULL, 0, 0) > 0)    {        TranslateMessage(&Msg);        DispatchMessage(&Msg);    }    return Msg.wParam;}// Callback function that sets all child windows of the screenshot taker window to the regular system fontBOOL CALLBACK SysFontProc(HWND hWnd, LPARAM lParam){    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 workLRESULT CALLBACK WindowProc(HWND hWnd, UINT Msg, WPARAM wParam, LPARAM lParam){    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'?
Refactorings on a Win32 Screenshot saving program
c++;image;windows
Quite a bit of code here, so this review is probably not complete:Coding style / Miscellaneous:Please keep parameter names on function prototypes. Parameter namesare a form of self documentation of the code.I would like to see a blank line between each function, both in the headerdeclarations 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 timerint         ms_interval;  // Inteval between clicksHWND        hwTarget;     // Target windowHANDLE      hThread;      // Thread for timer tickLPVOID      thistimer;    // Pointer to this class instancestatic bool timeractive;  // Flag for exitting the timer threadThis is arguably personal preference, but I think it makes sense placingthe 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 consulta 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:next_filename=shot_;next_filename+=countstr;next_filename+=.bmp;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 practicethat 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 forvariables, some are using camelCase, like in hwTarget while others are using snake_case, like in ticks_so_far. Choose one notation andstick 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 thatby applying a very unsafe cast to the function pointer. This is a crash waiting to happen. You have to declarethe 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 Cdelete 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){    Sleep(ms_interval);    CBMPWriter bmpW(hwTarget, ++ticks_so_far);    bmpW.SaveBMP();    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.