Review my file-searching program

Question

It's my first window program. It simply searches for a specified file in the whole computer.

File Search.h (header file that contains the prototypes of fileSearcher class methods.)

#ifndef UNICODE
#define UNICODE
#endif
#include <Windows.h>
#include <queue>
namespace fileSearch
{
class fileSearcher
{
public: 
 fileSearcher();
 ~fileSearcher(); //So far, this class doesn't allocate memory on the heap,therefore destructor is empty 
 void getAllPaths(const TCHAR* fileName,std::queue<TCHAR*> &output); 
 /*Returns all matching pathes at the current local system. Format:
 [A-Z]:\FirstPath\dir1...\fileName
 [A-Z]:\SecondPath\dir2...\fileName
 ...
 [A-Z]:\NPath\dirN...\fileName
 */
 void findFilesRecursivelly(const TCHAR* curDir,const TCHAR* fileName,std::queue<TCHAR*> &output);
 //Searches for the file in the current and in sub-directories. Sets nothingFound=false if the file has been found.
private: 
 static const int MAX_LOCATIONS = 20000;
 bool nothingFound;
 void endWithBackslash(TCHAR* string);
};
}

File Search.cpp ( ... definitions)

#ifndef UNICODE
#define UNICODE
#endif
#include "File Search.h"
using namespace fileSearch;
fileSearcher::fileSearcher()
{
 nothingFound = true;
}
fileSearcher::~fileSearcher()
{
}
void fileSearcher::getAllPaths(const TCHAR* fileName,std::queue<TCHAR*> &output)
{
 TCHAR localDrives[50];
 TCHAR currentDrive;
 int voluminesChecked=0;
 TCHAR searchedVolumine[5];
 nothingFound = true; 
 if(!wcslen(fileName))
 {
 output.push(TEXT("Invalid search key"));
 return;
 }
 GetLogicalDriveStrings(sizeof(localDrives)/sizeof(TCHAR),localDrives);
 //For all drives:
 for(int i=0; i < sizeof(localDrives)/sizeof(TCHAR); i++)
 { 
 if(localDrives[i] >= 65 && localDrives[i] <= 90)
 { 
 currentDrive = localDrives[i];
 voluminesChecked++;
 }
 else continue;
 searchedVolumine[0] = currentDrive;
 searchedVolumine[1] = L':';
 searchedVolumine[2] = 0;
 findFilesRecursivelly(searchedVolumine,fileName,output); 
 }
 if(nothingFound)
 output.push(TEXT("FILE NOT FOUND"));
}
void fileSearcher::findFilesRecursivelly(const TCHAR* curDir,const TCHAR* fileName,std::queue<TCHAR*> &output)
{
 HANDLE hFoundFile;
 WIN32_FIND_DATA foundFileData; 
 TCHAR* buffer;
 buffer = new TCHAR[MAX_PATH+_MAX_FNAME];
 wcscpy(buffer,curDir);
 endWithBackslash(buffer);
 if(!SetCurrentDirectory(buffer)) return;
 //Fetch inside current directory
 hFoundFile = FindFirstFileEx(fileName,FINDEX_INFO_LEVELS::FindExInfoBasic,&foundFileData ,FINDEX_SEARCH_OPS::FindExSearchNameMatch,NULL,FIND_FIRST_EX_LARGE_FETCH);
 if(hFoundFile != INVALID_HANDLE_VALUE)
 { 
 nothingFound = false; 
 do
 { 
 buffer = new TCHAR[MAX_PATH+_MAX_FNAME];
 wcscpy(buffer,curDir);
 endWithBackslash(buffer);
 wcscat(buffer,foundFileData.cFileName);
 wcscat(buffer,TEXT("\r\n"));
 output.push(buffer); 
 } 
 while(FindNextFile(hFoundFile,&foundFileData));
 }
 //Go to the subdirs
 hFoundFile = FindFirstFileEx(TEXT("*"),FINDEX_INFO_LEVELS::FindExInfoBasic,&foundFileData ,FINDEX_SEARCH_OPS::FindExSearchLimitToDirectories ,NULL , NULL);
 if(hFoundFile != INVALID_HANDLE_VALUE )
 { 
 do
 { 
 if(wcscmp(foundFileData.cFileName,TEXT(".")) && wcscmp(foundFileData.cFileName,TEXT("..")))
 {
 TCHAR nextDirBuffer[MAX_PATH+_MAX_FNAME]=TEXT("");
 wcscpy(nextDirBuffer,curDir);
 endWithBackslash(nextDirBuffer); //redundant?
 wcscat(nextDirBuffer,foundFileData.cFileName);
 findFilesRecursivelly( nextDirBuffer,fileName,output);
 }
 }
 while(FindNextFile(hFoundFile,&foundFileData));
 } 
}
void fileSearcher::endWithBackslash(TCHAR* string)
{
 if(string[wcslen(string)-1] != TEXT('\\')) wcscat(string,TEXT("\\"));
}

main.cpp (simple window interface for the fileSearcher class)

#ifndef UNICODE
#define UNICODE
#endif
#include <Windows.h>
#include <queue>
#include "File Search.h"
static HWND textBoxFileName;
static HWND searchButton;
static HWND textBoxOutput;
LRESULT CALLBACK WindowProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam);
using namespace fileSearch;
int WINAPI wWinMain(HINSTANCE hInstance,HINSTANCE hPrevInstance,LPWSTR cmdLine,int nCmdShow)
{
 TCHAR className[] = L"Main window class";
 WNDCLASS wc = {};
 wc.lpfnWndProc = WindowProc;
 wc.hInstance = hInstance;
 wc.lpszClassName = className;
 RegisterClass(&wc);
 HWND hMainWindow = CreateWindowEx(WS_EX_CLIENTEDGE,className,L"Path getter",WS_OVERLAPPEDWINDOW,CW_USEDEFAULT,CW_USEDEFAULT,600,300,NULL,NULL,hInstance,NULL);
 if(hMainWindow == NULL)
 return 0;
 textBoxFileName = CreateWindowEx(WS_EX_CLIENTEDGE, L"Edit", NULL,WS_CHILD | WS_VISIBLE | ES_AUTOHSCROLL, 10, 10, 300, 21, hMainWindow, NULL, NULL, NULL);
 searchButton = CreateWindowEx(WS_EX_CLIENTEDGE,L"Button",L"Search",WS_CHILD | WS_VISIBLE | ES_CENTER, 10, 41,75,30,hMainWindow,NULL,NULL,NULL); 
 textBoxOutput = CreateWindowEx(WS_EX_CLIENTEDGE,L"Edit",NULL,WS_CHILD | WS_VISIBLE | WS_VSCROLL | ES_AUTOVSCROLL | ES_MULTILINE | ES_READONLY ,10,81,500,90,hMainWindow,NULL,NULL,NULL);
 ShowWindow(hMainWindow,nCmdShow);
 MSG msg = { };
 while (GetMessage(&msg, NULL, 0, 0))
 {
 TranslateMessage(&msg);
 DispatchMessage(&msg);
 }
 return 0;
}
LRESULT CALLBACK WindowProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
{
 switch (uMsg)
 {
 case WM_COMMAND:
 if((HWND)lParam == searchButton)
 { 
 fileSearcher searcher;
 TCHAR key[1000];
 std::queue<TCHAR*> buffer;
 SetWindowText(textBoxOutput,NULL);
 GetWindowText(textBoxFileName,key,1000);
 searcher.getAllPaths(key,buffer);
 SendMessage(textBoxOutput,EM_LIMITTEXT,WPARAM(0xFFFFF),NULL);
 while(!buffer.empty()) 
 { 
 SendMessage(textBoxOutput,EM_SETSEL,GetWindowTextLength(textBoxOutput),GetWindowTextLength(textBoxOutput));
 SendMessage(textBoxOutput,EM_REPLACESEL,FALSE, (LPARAM) buffer.front()); 
 delete [] buffer.front(); 
 buffer.pop();
 }
 }
 return 0;
 break;
 case WM_DESTROY:
 PostQuitMessage(0);
 return 0;
 case WM_PAINT:
 {
 PAINTSTRUCT ps;
 HDC hdc = BeginPaint(hwnd, &ps);
 HBRUSH pedzel;
 pedzel = CreateSolidBrush(RGB(249,224,75));
 FillRect(hdc, &ps.rcPaint, pedzel);
 EndPaint(hwnd, &ps);
 }
 return 0;
 }
 return DefWindowProc(hwnd, uMsg, wParam, lParam);
}

Shortcomings that I've noticed so far:

1) The window freezes during work
2) Only a few queries can be done in a row - a memory after each isn't deallocated from heap (I simply don't know how to fix it).

• Sunderi Mehta: Are you against using a Thread pool?

Answer 1

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.

• 0x6B6F77616C74: If I had deleted buffer inside fileSearcher::findFilesRecursivelly, I would have lost the original value to which WindowProc's queue<TCHAR*> buffer references. I've tried to delete it after sending message to textBoxOutput, but mysteriously it doesn't work.
• user1201210: @0x6B6F77616C74 I see what you mean, thanks for the explanation. I can't think of a good one-line fix for that. Using std::queue<std::basic_string<TCHAR>> (my last bullet point) is probably the cleanest way to let you free up buffer by the end of the function without changing all the strings in your code.