Return to Answer

• 1

• 17.3k
• 4
• 31
• 89

• 17.3k
• 4
• 31
• 89

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:

   next_filename="shot_";
   next_filename+=countstr;
   next_filename+=".bmp";
  

Always put a space before an assignment or an 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)
   {
   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.