patterncppMinor
string.find versus this function
Viewed 0 times
thisversusfunctionfindstring
Problem
I got bored and decided to write my own function to verify if a string contains another string.
I was wondering if there's any major difference between these two function except the one I wrote being more complicated.
I was wondering if there's any major difference between these two function except the one I wrote being more complicated.
bool Contains(string str, string find){
for (int i = 0; i 0){
int CharactersFound = 0;
for (int x = 0; x < find.length(); x++){
if (str[i + x] == find[x]){
CharactersFound++;
}
else {
CharactersFound = 0;
}
}
if (CharactersFound == find.length()) return true;
} else {
return true;
}
}
return false;
}
bool StrContains(string str, string find){
if (str.find(find) != string::npos) return true;
return false;
}Solution
There's likely a difference in your implementation and the library's. The algorithm you used is an inefficient variant of the inefficient brute force algorithm (yes, doubly inefficient). Better algorithms exist for finding a substring, for example the Knuth-Morris-Pratt algorithm. I suspect the library uses something more clever than brute force.
It's strange and inefficient to have the
Counting the characters found is inefficient if you don't break on a mismatch.
Instead of counting the characters found, it would be better to use a boolean flag. When you find a character that doesn't match, you can set the flag and break out of the loop. After the loop, if the flag changed, you know the string wasn't found.
That is:
Instead of using a flag, what's even better is to extract the inner loop to a function, as Jerry-Coffin did.
You can use boolean expressions directly as return values. So instead of this:
You can write:
There are multiple accepted naming conventions in C++,
but I don't think there is when where local variables follow
It's strange and inefficient to have the
find.length() > 0 inside the loop. The value of find.length() doesn't change inside the loop, so it's better to check it once, before the loop begins.Counting the characters found is inefficient if you don't break on a mismatch.
Instead of counting the characters found, it would be better to use a boolean flag. When you find a character that doesn't match, you can set the flag and break out of the loop. After the loop, if the flag changed, you know the string wasn't found.
That is:
for (int i = 0; i < str.length() - (find.length()-1); i++) {
bool mismatch = false;
for (int x = 0; x < find.length(); x++) {
if (str[i + x] != find[x]){
mismatch = true;
break;
}
}
if (!mismatch) {
return true;
}
}
return false;Instead of using a flag, what's even better is to extract the inner loop to a function, as Jerry-Coffin did.
You can use boolean expressions directly as return values. So instead of this:
if (str.find(find) != string::npos) return true;
return false;You can write:
return str.find(find) != string::npos;There are multiple accepted naming conventions in C++,
but I don't think there is when where local variables follow
PascalCase. It would be better to change to camelCase.Code Snippets
for (int i = 0; i < str.length() - (find.length()-1); i++) {
bool mismatch = false;
for (int x = 0; x < find.length(); x++) {
if (str[i + x] != find[x]){
mismatch = true;
break;
}
}
if (!mismatch) {
return true;
}
}
return false;if (str.find(find) != string::npos) return true;
return false;return str.find(find) != string::npos;Context
StackExchange Code Review Q#144605, answer score: 4
Revisions (0)
No revisions yet.