# GUI that searches files using user inputted search strings [closed]

Basically, I've got a GUI where the user can select text files, enter in strings (that may or may not contain wildcards) and search those files for those strings.

Currently, I take the user inputted string(s) and divide them into two groups: regularly searchable strings, and strings that have wildcards (either * for 1 wildcard character, or .* for any amount).

If they are regularly searchable strings, then I use the normal str.find function (as I tested this vs regex_search and this is faster), otherwise I use the regex_search function.

The main problem is performance. As a benchmark comparison, it takes my program roughly 47 minutes and it is searching through 5,028,712 lines. From trying to figure this problem out on google it seems this entire search should take me well under a minute...

searchAllFilesForAllStrings -> bool checkbox on the GUI where if it is set to true, the program will just search every file for every string, and false, will only search each "batch" of files (if the filepath includes a wildcard, such as "Read*.txt -> all files starting with Read and that are .txt files will be chosen)

Global variables:

vector<string> regex_patterns;
vector<string> excelFiles;
vector<string> nonWildCardSearchStrings;
vector<string> searchStrings;
vector<string> searchFiles;
vector<bool> regexIndex;


This function gets called from a GUI button using an editable text box that contains the path to the file. It essentially grabs strings and files from an excel file formatted with 2 columns, one column search strings, and one column search files. In search strings, XYY denotes 1 wildcard (because the user should be able to search for a * if they desire; XYZ denotes any # of wildcards):

ifstream excelFile;
string line;
string delimiter = ",";
int bIdxStrings = 0;
int bIdxFiles = 0;
for (int i = 0; i < excelPath.size(); i++)
{
excelFile.open(excelPath.at(i));
if (excelFile.is_open()) {}
else { return false; }
int index = 0;
while (getline(excelFile, line))
{
searchStrings.push_back(line.substr(0, line.find(delimiter)));
searchFiles.push_back(line.substr(line.find(delimiter) + 1));
index = index + 1;
}
searchStrings.erase(searchStrings.begin() + bIdxStrings);
searchFiles.erase(searchFiles.begin() + bIdxFiles);
bIdxStrings = searchStrings.size() + 1;
bIdxFiles = searchFiles.size() + 1;
for (int i = 0; i < searchStrings.size(); i++)
{
}
}
excelFile.close();
string key = "\\";
size_t foundLast = 0;
string wcPath = "";
vector<string>test;
string holdTemp = "";
string regSearch = "";
string regtemp = "";
string fullPath = "";
vector<string>tempFiles;
vector<string>tempStrings;
// Search for wildcard paths. if any exist, find files based on their main directory. will not recursively search.
for (int i = 0; i < searchFiles.size();i++)
{
size_t found = searchFiles.at(i).find("*");
if (found != string::npos)
{
// temporarily hold the search string corresponding to this entry.
holdTemp = searchStrings.at(i);
foundLast = searchFiles.at(i).rfind(key);
wcPath = searchFiles.at(i).substr(0, foundLast);
regSearch = searchFiles.at(i).substr(foundLast + 1, string::npos);

regtemp = regSearch.substr(0, regSearch.find("*"));
regtemp.append(".*");
regtemp.append(regSearch.substr(regSearch.find("*") + 1, string::npos));
regSearch = regtemp;
smatch matchez;
// Should make regex search case insensitive.
regex e(regSearch, regex_constants::icase);
//searchFiles.erase(searchFiles.begin() + i);
//searchStrings.erase(searchStrings.begin() + i);
// All files in the directory:
for (int m = 0; m < get<0>(addWildCardFiles).size(); m++)
{
size_t wcBool = regex_search(get<0>(addWildCardFiles)[m], matchez, e);
if (wcBool == 1)
{
fullPath.append(wcPath); fullPath.append("\\");
tempFiles.push_back(fullPath);
tempStrings.push_back(holdTemp);
}
fullPath = "";
}
}
}

searchStrings.insert(searchStrings.end(), tempStrings.begin(), tempStrings.end());
searchFiles.insert(searchFiles.end(), tempFiles.begin(), tempFiles.end());
sort(searchStrings.begin(), searchStrings.end());
sort(searchFiles.begin(), searchFiles.end());
searchFiles.erase(unique(searchFiles.begin(), searchFiles.end()), searchFiles.end());
if (searchAllFilesForAllStrings == true)
{
searchStrings.erase(unique(searchStrings.begin(), searchStrings.end()), searchStrings.end());
}

int setNext = -1;
vector<int> filesRepeat;
vector<int> stringsRepeat;
size_t stringsCount = 0;
size_t filesCount = 0;

// Loops to get rid of duplicate search strings + duplicate files.
// Dont get rid of duplicates if only searching each file for each subsequent string because of how the code is structured;
for (int i = 0; i < searchStrings.size(); i++) { if (searchStrings.at(i).compare("Search Strings") == 0) { searchStrings.erase(searchStrings.begin() + i); } }
for (int i = 0; i < searchFiles.size(); i++) { if (searchFiles.at(i).compare("Search Files") == 0) { searchFiles.erase(searchFiles.begin() + i); } }

// Loops to get rid of wildstar patterns that are included (these can't be searched)
int idx = 0;
int startCount = searchFiles.size();
while (idx < startCount)
{
if (contains(searchFiles.at(idx), "*", 1) == 1)
{
searchFiles.erase(searchFiles.begin() + idx);
startCount = startCount - 1;
idx = 0;
}
idx = idx + 1;
}

// Loop to deal with each search string and format it for regex searching later.
// only pull strings that are non wildcard containing. everything else can be normally searched which will save time.
for (unsigned int jj = 0; jj < searchStrings.size(); jj++)
{
if (contains(searchStrings.at(jj), "XYY", 0) == 1 || contains(searchStrings.at(jj), "XYZ", 0) == 1)
{
regex_patterns.push_back(replaceWildCards(searchStrings.at(jj)));
regexIndex.push_back(true);
}
else
{
nonWildCardSearchStrings.push_back(searchStrings.at(jj));
regexIndex.push_back(false);
}
}
return true;


found and nonwcfound are the variables used to check if a match was found and subsequently to save the line text and line number in vectors. filename and foldername are variables that will also be saved to be outputted to the file.

        size_t found;
bool nonwcfound = false;
smatch matches;
vector<regex> expressions;
for (int i = 0; i < regex_patterns.size(); i++) { expressions.emplace_back(regex_patterns.at(i)); }

if (searchAllFilesForAllStrings == true)
{
ofstream myOutPut;
myOutPut.open(outputFilePath, std::ofstream::out | std::ofstream::app);
myOutPut << "Line Text, Line Number, File Name, Folder Path," << "\n";
myOutPut.close();
for (size_t j = 0; j < searchFiles.size();j++)
{
// Initialize variables for line number + text
vector<int> lineNumber;
vector<string> lineText;
vector<string>lineStrings;
string entireFile;

// Get file and folder name for storage.
string fileName;
string folderName;
fileName = searchFiles.at(j);
int fileNameSlashIdx = fileName.rfind("\\");
folderName = fileName.substr(0, fileNameSlashIdx);
fileName = fileName.substr(fileNameSlashIdx + 1, string::npos);

// File ifstream definition/opening
ifstream file;
file.open(searchFiles.at(j), ios::in | ios::ate);

// Fill and close file
if (file)
{
ifstream::streampos filesize = file.tellg();
entireFile.reserve(filesize);
file.seekg(0);
while (!file.eof())
{
entireFile += file.get();
}
}
file.close();
int linecount = 0;

stringstream stream(entireFile);
while (1)
{
string line;
getline(stream, line);
if (!stream.good())
break;
for (size_t r = 0; r < expressions.size(); r++)
{
found = regex_search(line, matches, expressions.at(r));
if (found == 1)
{
lineNumber.push_back(linecount);
lineText.push_back(line);
}
}
for (size_t rr = 0; rr < nonWildCardSearchStrings.size(); rr++)
{
nonwcfound = contains(line, nonWildCardSearchStrings.at(rr), 0);
if (nonwcfound == true)
{
lineNumber.push_back(linecount);
lineText.push_back(line);
}
}
linecount = linecount + 1;
}

entireFile.clear();
ofstream myOutPut;
myOutPut.open(outputFilePath, std::ofstream::out | std::ofstream::app);
{
tuple<vector<string>, vector<int>, string, string>result = make_tuple(lineText, lineNumber, fileName, folderName);
writeResultsToFile(result, outputFilePath);
}
myOutPut.close();
}
}
if (searchAllFilesForAllStrings == false)
{
// Do the same thing as above, except that it will search each file/batch of files only with the
// subsequent search string in the same row of the excel file that is read in using the above function.
}
MessageBox::Show("Finished execution. Your file is now available for viewing!", "Output Excel File Written");


## closed as off-topic by Martin York, Mast, 1201ProgramAlarm, esote, EdwardApr 18 at 11:42

This question appears to be off-topic. The users who voted to close gave this specific reason:

• "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Martin York, Mast, 1201ProgramAlarm, esote, Edward
If this question can be reworded to fit the rules in the help center, please edit the question.

• 1. Did you use a profiler to profile the program? We can guess about performance, but since this isn't a complete, compilable program it's just guessing. You have the power to actually measure it and see. 2. Please add the function declarations. At the moment we have to guess the types of the function parameters (e.g. excelPath). – user673679 Apr 17 at 19:11
• Have to vote to close as it stands. There are no functions here just raw code so impossible to review as it stands (no context). But there is a lot to talk about (efficiency and improvements and design) with each function. Please resubmit each function with a test harness to show that that function works as expected and can be tested by us. – Martin York Apr 18 at 0:11
• Please submit each function seprately for its own review. – Martin York Apr 18 at 0:12

Honestly, I don't think this code really works as intended.

I suggest splitting it into smaller parts (perhaps write a separate test program without the gui), and testing each part separately (print to cout, step through it with a debugger) to ensure that it does what you expect.

• Don't use global variables. Use function arguments (pass by reference or const-reference as appropriate), and function return values to bring variables into the scope in which they're needed.

• Declare variables as close to the point of use as possible, and initialize them directly to the appropriate value, rather than a temporary "invalid" value. (e.g. excelFile could be declared inside that first loop). This reduces the potential for mistakes (e.g. using the "invalid" value, or reusing the value from a previous loop) and complexity (variables are only visible where they're needed).

• Use functions to split code into logical units responsible for a single thing. This helps by "naming" sections of code (need fewer comments), reducing the scope of variables (e.g. excelFile is only used in the top part of that ginormous function), and makes the code easier to read and understand.

That first code listing could be split into several functions. Ignoring function arguments / variables it might be something like:

ReadSearchStrings();
ExpandWildcardPaths();
PrepareSearchParameters();


(And that last one might be further subdivided).

• Use const& variables to alias other variables without needing to copy them.

• Use the index type of the container for indexing (i.e. std::size_t instead of int).

• We need to handle errors in case of invalid user input.

The code:

There are a lot of issues, so going through line by line to point out problems:

ifstream excelFile; // declare inside the loop
string line; // declare inside the loop
string delimiter = ","; // make it const, make it a char

int bIdxStrings = 0; // we should index a container with it's index type (i.e. std::size_t) not an int
int bIdxFiles = 0;

for (int i = 0; i < excelPath.size(); i++) // use range-based for to easily iterate whole containers
{
excelFile.open(excelPath.at(i));

if (excelFile.is_open()) {}  // this is convoluted. just return false if it's not open.
else { return false; }

int index = 0; // index isn't actually used

while (getline(excelFile, line))
{
// what if the delimiter is missing? both searchStrings and searchFiles will contain the whole line, but that seems unintentional?
searchStrings.push_back(line.substr(0, line.find(delimiter)));
searchFiles.push_back(line.substr(line.find(delimiter) + 1)); // searching for delimiter twice!
index = index + 1;
}

// it looks like we're ignoring the first entry in the file?
// if so, we can just do an extra getline before the while loop and ignore that line
// it's much cleaner than adding and erasing an entry

searchStrings.erase(searchStrings.begin() + bIdxStrings);
searchFiles.erase(searchFiles.begin() + bIdxFiles);

bIdxStrings = searchStrings.size() + 1; // (aren't bIdxStrings and bIdxFiles always the same?)
bIdxFiles = searchFiles.size() + 1;

// note that searchStrings contains the strings from multiple excel files...
// but we're calling this on the whole set of search strings for every excel files.
// so for 10 files, the search strings in the first file will have escapes added 10 times.
// in other words: this should be ouside the excelPath loop.

for (int i = 0; i < searchStrings.size(); i++)
{
}
}
excelFile.close();


And a cleaned up version might look more like:

char const delimiter = ',';

for (auto const& path : excelPath)
{
std::ifstream excelFile(path);

if (!excelFile)
return false;

std::string ignoredLine;
std::getline(excelFile, ignoredLine);

std::string line;
while (std::getline(excelFile, line))
{
std::size_t const split = line.find(delimiter);

if (split == std::string::npos)
continue; // (or return an error?)

searchStrings.push_back(line.substr(0, split));
searchFiles.push_back(line.substr(split + 1));
}
}

for (auto& string : searchStrings)


I'm not going to include the "tidy" version for the rest of the code, because it's hard to guess the intent / correct behavior in a lot of this. I'll just point out issues:

string key = "\\"; // const
size_t foundLast = 0; // declare inside the loop
string wcPath = ""; // declare inside the loop
tuple<vector<string>, string> addWildCardFiles; // declare inside the loop
vector<string>test; // declare inside the loop (not a helpful name... and is this variable actually used?)
string holdTemp = ""; // declare inside the loop
string regSearch = ""; // declare inside the loop
string regtemp = ""; // declare inside the loop
string fullPath = ""; // declare inside the loop
vector<string> tempFiles;
vector<string> tempStrings;

for (int i = 0; i < searchFiles.size();i++) // use std::size_t
{
// we can use const& variables to alias the current file and search string
// instead of using .at(i) everywhere, e.g.:

// std::string const& string = searchStrings.at(i);
// std::string const& file = searchFiles.at(i);

size_t found = searchFiles.at(i).find("*");

// better to "return" early by using continue and avoid the indent.
//if (found == string::npos) continue;
if (found != string::npos)
{
holdTemp = searchStrings.at(i); // unnecessary copy - can use a const& instead (see above)
foundLast = searchFiles.at(i).rfind(key); // what if we don't find key?
wcPath = searchFiles.at(i).substr(0, foundLast);
regSearch = searchFiles.at(i).substr(foundLast + 1, string::npos);

// the code below just inserts ".*" in place of "*", right?
// we could do that directly w/ regSearch.insert(pos, '.');

// what if we don't find the "*"?
// regSearch.find("*") returns npos so , e.g.:
// a string of "fullpath" will become "fullpath.*fullpath".
// is this intentional?

// is regSearch.find("*") ever different from searchFiles.at(i).find("*")?
// what if the "*" occurs before key?

regtemp = regSearch.substr(0, regSearch.find("*"));
regtemp.append(".*");
regtemp.append(regSearch.substr(regSearch.find("*") + 1, string::npos)); // calling find twice!

regSearch = regtemp;

smatch matchez; // declare inside the loop
regex e(regSearch, regex_constants::icase);

// we never access the second part of this tuple...
// so change addWildCardFiles and write the get<> only once when calling read_directory.

for (int m = 0; m < get<0>(addWildCardFiles).size(); m++) // use a range-based for loop:
// for (auto const& wcFile : addWildCardFiles)
{
// uh... why not use an actual bool?
size_t wcBool = regex_search(get<0>(addWildCardFiles)[m], matchez, e);

if (wcBool == 1) // again, prefer to use continue so we don't have to indent code
{
fullPath.append(wcPath);
fullPath.append("\\");
tempFiles.push_back(fullPath);

// tempFiles.push_back(wcPath + "\\" + wcFile);

tempStrings.push_back(holdTemp); // why, though? it's already in searchStrings, no?
}

fullPath = "";
}
}
}

searchStrings.insert(searchStrings.end(), tempStrings.begin(), tempStrings.end());
searchFiles.insert(searchFiles.end(), tempFiles.begin(), tempFiles.end());

// I don't really understand this:
// For every wildcard file entry, we just added a copy of the search string to searchStrings.
// And now we sort searchStrings and searchFiles independently, which destroys any index relation between the two.

sort(searchStrings.begin(), searchStrings.end());
sort(searchFiles.begin(), searchFiles.end());

// Then we make the files unique, but only sometimes make the strings unique?
// Why is it ever useful to search for duplicate strings?

searchFiles.erase(unique(searchFiles.begin(), searchFiles.end()), searchFiles.end());
if (searchAllFilesForAllStrings == true) // if it's a bool, we don't need == true, just test the bool directly
{
searchStrings.erase(unique(searchStrings.begin(), searchStrings.end()), searchStrings.end());
}

int setNext = -1;
vector<int> filesRepeat;
vector<int> stringsRepeat;
size_t stringsCount = 0;
size_t filesCount = 0;

// ---------------------
// I'm guessing this is removing excel column headers? This should be done in the excel parsing part above.
// It also shouldn't be done based on content -> what if the user wants to search for the words "Search Strings"?
// We can use the erase-remove idiom to do this:
//searchStrings.erase(
//  std::remove(searchStrings.begin(), searchStrings.end(), "Search Strings"),
//  searchStrings.end());

for (int i = 0; i < searchStrings.size(); i++) { if (searchStrings.at(i).compare("Search Strings") == 0) { searchStrings.erase(searchStrings.begin() + i); } }
for (int i = 0; i < searchFiles.size(); i++) { if (searchFiles.at(i).compare("Search Files") == 0) { searchFiles.erase(searchFiles.begin() + i); } }

// ---------------------
// Note that std::vector::erase returns an iterator pointing one past the element removed,
// so the old-school way to do this is:

//for (auto i = searchFiles.begin(); i != searchFiles.end(); )
//{
//  if (contains(*i, "*", 1) == 1)
//      i = searchFiles.erase(i);
//  else
//      ++i;
//}

// or better, we use erase-remove again:

//searchFiles.erase(
//  std::remove_if(searchFiles.begin(), searchFiles.end(), [] (std::string const& s) { return contains(s, "*", 1) == 1; }),
//  searchFiles.end());

int idx = 0;
int startCount = searchFiles.size();
while (idx < startCount)
{
if (contains(searchFiles.at(idx), "*", 1) == 1)
{
searchFiles.erase(searchFiles.begin() + idx);
startCount = startCount - 1;
idx = 0;
}
idx = idx + 1;
}

for (unsigned int jj = 0; jj < searchStrings.size(); jj++) // use range-based for
{
if (contains(searchStrings.at(jj), "XYY", 0) == 1 || contains(searchStrings.at(jj), "XYZ", 0) == 1)
{
regex_patterns.push_back(replaceWildCards(searchStrings.at(jj)));
regexIndex.push_back(true);
}
else
{
nonWildCardSearchStrings.push_back(searchStrings.at(jj));
regexIndex.push_back(false);
}
}
return true;


And the second code listing:

    size_t found; // declare inside the loop
bool nonwcfound = false; // declare inside the loop
smatch matches; // declare inside the loop

// we could just use the vector copy constructor here:
// vector<regex> expressions = regex_patterns;
// ...but why are we copying it anyway?
vector<regex> expressions;
for (int i = 0; i < regex_patterns.size(); i++) { expressions.emplace_back(regex_patterns.at(i)); }

if (searchAllFilesForAllStrings == true) // test bools directly, don't compare to true / false
{
// use the ofstream constructor:
// std::ofstream myOutPut(outputFilePath, std::ofstream::out | std::ofstream::app);
// closing and reopening the file is probably slower than just leaving it open.

ofstream myOutPut;
myOutPut.open(outputFilePath, std::ofstream::out | std::ofstream::app);
myOutPut << "Line Text, Line Number, File Name, Folder Path," << "\n";
myOutPut.close();

for (size_t j = 0; j < searchFiles.size();j++) // use range-based for
{
vector<int> lineNumber; // declare as close to the point of use as possible
vector<string> lineText; // same
vector<string>lineStrings; // same
string entireFile; // same

string fileName; // don't declare and initialize separately! do it in one step
string folderName; // don't declare and initialize separately! do it in one step
fileName = searchFiles.at(j); // unnecessary copy! use a const& variable to alias it.

// again, what if "\\" isn't present?
// both folderName and fileName will contain the whole string...
int fileNameSlashIdx = fileName.rfind("\\");
folderName = fileName.substr(0, fileNameSlashIdx);
fileName = fileName.substr(fileNameSlashIdx + 1, string::npos);

ifstream file; // again, use the constructor to open the file
file.open(searchFiles.at(j), ios::in | ios::ate);

// does the rest of the loop body make sense if the file isn't open? might be better to do:
// if (!file)
//  continue;

if (file)
{
// reading a whole file into a string could be a separate function.
// note that there are easier ways:
// std::string entireFile((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>());

ifstream::streampos filesize = file.tellg();
entireFile.reserve(filesize);
file.seekg(0);
while (!file.eof())
{
entireFile += file.get();
}
}
file.close();

int linecount = 0;
// declare lineNumber, lineText, etc. here too

// why not use the file stream directly instead of a stringstream?
// we allocate memory for the entire file and copy it into a string above
// then we copy it again here.
stringstream stream(entireFile);

// nitpick: use while (true) instead
// while (1) is really testing while (1 != 0), which is kinda indirect
while (1)
{
string line;
getline(stream, line);
if (!stream.good())
break;

for (size_t r = 0; r < expressions.size(); r++) // use range-based for
{
found = regex_search(line, matches, expressions.at(r));
if (found == 1)
{
lineNumber.push_back(linecount);
lineText.push_back(line);
}
}
for (size_t rr = 0; rr < nonWildCardSearchStrings.size(); rr++) // use range-based for
{
nonwcfound = contains(line, nonWildCardSearchStrings.at(rr), 0);
if (nonwcfound == true)
{
lineNumber.push_back(linecount);
lineText.push_back(line);
}
}
linecount = linecount + 1;
}

entireFile.clear();

ofstream myOutPut; // again, use the constructor
myOutPut.open(outputFilePath, std::ofstream::out | std::ofstream::app);

{
tuple<vector<string>, vector<int>, string, string>result = make_tuple(lineText, lineNumber, fileName, folderName);
writeResultsToFile(result, outputFilePath);
}

myOutPut.close();
}
}

// use an "else"...
// if there's so much code in a branch that we forget the condition, split it into a separate function.
if (searchAllFilesForAllStrings == false) // test bools directly, don't compare to true / false
{
// "Do the same thing as above, except that it will search each file/batch of files only with the
// subsequent search string in the same row of the excel file that is read in using the above function."

// ^^ I don't see how that will be possible, given that we sorted searchStrings and searchFiles independently.
// So at this point there's no relation between the two!!!
}
MessageBox::Show("Finished execution. Your file is now available for viewing!", "Output Excel File Written");