Return to Answer

A few points:

• Best Practice: There's no need to define UNICODE in File Search.h since it's defined at the top of those files that include it. Consider creating a configuration header that gets included by the .cpp first and let it handle the defines and other preprocessor logic. That way if you decide to make a non-UNICODE build, you only need to change one definition, not two or three.

• Best Practice: Class names in C++ generally follow one of two conventions: CamelCaseStyle or underscore_style. Consider using one of these for your class.

• Potential Build Error / Best Practice: Good use of TCHAR and the TEXT macro to conform to the Windows function definitions. Be sure to use the macro instead of directly using L"..." and L'.', also, so that the code can compile a non-UNICODE build. Same goes for the string-manipulating functions: wcscpy (should be _tcscpy), wcscat (should be _tcscat), and wcscmp (should be _tcscmp). More on strings later, though, since all of this (including the use of TCHAR*) is the hard way to manage strings in C++.

• Memory Leak: In fileSearcher::fineFilesRecursivelly, buffer is allocated with new, but never deleted. Currently the function can return in two places, so you'll need to have delete [] buffer before those two places (more on strings like buffer next). [edit: buffer is allocated twice in the function. Be sure that it's freed before allocating it again.]

• Best Practice: You're using C-style strings in your C++ code, which means you need to allocate them yourself (like buffer), deallocate them yourself, and call the various string-managing functions. Instead, consider using C++'s std::basic_string-derived classes to handle the tedious details. I think the related changes you'd need to make are out of scope for a code review because of the heavy use of C-style strings in your code, so check out Stack Overflow's many questions and answers on the topic.

• Potential Runtime Error / Best Practice: You're using std::queue<TCHAR*> to make a queue of strings, but the strings are not being copied to the queue, only referenced. The string's memory is not guaranteed to be correct once the string variable goes out of scope (I'm a little surprised it works [edit: it's not as broken as I first thought, but I still recommend the change mentioned here] ). You'll want to use std::queue<std::basic_string<TCHAR>> to ensure that copies are created and destroyed. More on strings in the previous note.

Shortcomings that you mentioned:

1. The leg-work in your code is being done from WindowProc, so additional messages cannot be processed until the work is finished. You could use a separate thread to do the work, but that topic is beyond the scope of this review.

2. I think the memory leak is buffer, as mentioned above.

A few points:

• Best Practice: There's no need to define UNICODE in File Search.h since it's defined at the top of those files that include it. Consider creating a configuration header that gets included by the .cpp first and let it handle the defines and other preprocessor logic. That way if you decide to make a non-UNICODE build, you only need to change one definition, not two or three.

• Best Practice: Class names in C++ generally follow one of two conventions: CamelCaseStyle or underscore_style. Consider using one of these for your class.

• Potential Build Error / Best Practice: Good use of TCHAR and the TEXT macro to conform to the Windows function definitions. Be sure to use the macro instead of directly using L"..." and L'.', also, so that the code can compile a non-UNICODE build. Same goes for the string-manipulating functions: wcscpy (should be _tcscpy), wcscat (should be _tcscat), and wcscmp (should be _tcscmp). More on strings later, though, since all of this (including the use of TCHAR*) is the hard way to manage strings in C++.

• Memory Leak: In fileSearcher::fineFilesRecursivelly, buffer is allocated with new, but never deleted. Currently the function can return in two places, so you'll need to have delete [] buffer before those two places (more on strings like buffer next). [edit: buffer is allocated twice in the function. Be sure that it's freed before allocating it again.]

• Best Practice: You're using C-style strings in your C++ code, which means you need to allocate them yourself (like buffer), deallocate them yourself, and call the various string-managing functions. Instead, consider using C++'s std::basic_string-derived classes to handle the tedious details. I think the related changes you'd need to make are out of scope for a code review because of the heavy use of C-style strings in your code, so check out Stack Overflow's many questions and answers on the topic.

• Potential Runtime Error / Best Practice: You're using std::queue<TCHAR*> to make a queue of strings, but the strings are not being copied to the queue, only referenced. The string's memory is not guaranteed to be correct once the string variable goes out of scope (I'm a little surprised it works [edit: it's not as broken as I first thought, but I still recommend the change mentioned here] ). You'll want to use std::queue<std::basic_string<TCHAR>> to ensure that copies are created and destroyed. More on strings in the previous note.

Shortcomings that you mentioned:

1. The leg-work in your code is being done from WindowProc, so additional messages cannot be processed until the work is finished. You could use a separate thread to do the work, but that topic is beyond the scope of this review.

2. I think the memory leak is buffer, as mentioned above.

A few points:

• Best Practice: There's no need to define UNICODE in File Search.h since it's defined at the top of those files that include it. Consider creating a configuration header that gets included by the .cpp first and let it handle the defines and other preprocessor logic. That way if you decide to make a non-UNICODE build, you only need to change one definition, not two or three.

• Best Practice: Class names in C++ generally follow one of two conventions: CamelCaseStyle or underscore_style. Consider using one of these for your class.

• Potential Build Error / Best Practice: Good use of TCHAR and the TEXT macro to conform to the Windows function definitions. Be sure to use the macro instead of directly using L"..." and L'.', also, so that the code can compile a non-UNICODE build. Same goes for the string-manipulating functions: wcscpy (should be _tcscpy), wcscat (should be _tcscat), and wcscmp (should be _tcscmp). More on strings later, though, since all of this (including the use of TCHAR*) is the hard way to manage strings in C++.

• Memory Leak: In fileSearcher::fineFilesRecursivelly, buffer is allocated with new, but never deleted. Currently the function can return in two places, so you'll need to have delete [] buffer before those two places (more on strings like buffer next).

• Best Practice: You're using C-style strings in your C++ code, which means you need to allocate them yourself (like buffer), deallocate them yourself, and call the various string-managing functions. Instead, consider using C++'s std::basic_string-derived classes to handle the tedious details. I think the related changes you'd need to make are out of scope for a code review because of the heavy use of C-style strings in your code, so check out Stack Overflow's many questions and answers on the topic.

• Potential Runtime Error / Best Practice: You're using std::queue<TCHAR*> to make a queue of strings, but the strings are not being copied to the queue, only referenced. The string's memory is not guaranteed to be correct once the string variable goes out of scope (I'm a little surprised it works). You'll want to use std::queue<std::basic_string<TCHAR>> to ensure that copies are created and destroyed. More on strings in the previous note.

Shortcomings that you mentioned:

1. The leg-work in your code is being done from WindowProc, so additional messages cannot be processed until the work is finished. You could use a separate thread to do the work, but that topic is beyond the scope of this review.

2. I think the memory leak is buffer, as mentioned above.

A few points:

• Best Practice: There's no need to define UNICODE in File Search.h since it's defined at the top of those files that include it. Consider creating a configuration header that gets included by the .cpp first and let it handle the defines and other preprocessor logic. That way if you decide to make a non-UNICODE build, you only need to change one definition, not two or three.

• Best Practice: Class names in C++ generally follow one of two conventions: CamelCaseStyle or underscore_style. Consider using one of these for your class.

• Potential Build Error / Best Practice: Good use of TCHAR and the TEXT macro to conform to the Windows function definitions. Be sure to use the macro instead of directly using L"..." and L'.', also, so that the code can compile a non-UNICODE build. Same goes for the string-manipulating functions: wcscpy (should be _tcscpy), wcscat (should be _tcscat), and wcscmp (should be _tcscmp). More on strings later, though, since all of this (including the use of TCHAR*) is the hard way to manage strings in C++.

• Memory Leak: In fileSearcher::fineFilesRecursivelly, buffer is allocated with new, but never deleted. Currently the function can return in two places, so you'll need to have delete [] buffer before those two places (more on strings like buffer next). [edit:buffer is allocated twice in the function. Be sure that it's freed before allocating it again.]

• Best Practice: You're using C-style strings in your C++ code, which means you need to allocate them yourself (like buffer), deallocate them yourself, and call the various string-managing functions. Instead, consider using C++'s std::basic_string-derived classes to handle the tedious details. I think the related changes you'd need to make are out of scope for a code review because of the heavy use of C-style strings in your code, so check out Stack Overflow's many questions and answers on the topic.

• Potential Runtime Error / Best Practice: You're using std::queue<TCHAR*> to make a queue of strings, but the strings are not being copied to the queue, only referenced. The string's memory is not guaranteed to be correct once the string variable goes out of scope (I'm a little surprised it works [edit: it's not as broken as I first thought, but I still recommend the change mentioned here] ). You'll want to use std::queue<std::basic_string<TCHAR>> to ensure that copies are created and destroyed. More on strings in the previous note.

Shortcomings that you mentioned:

1. The leg-work in your code is being done from WindowProc, so additional messages cannot be processed until the work is finished. You could use a separate thread to do the work, but that topic is beyond the scope of this review.

2. I think the memory leak is buffer, as mentioned above.