HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

string.find versus this function

Submitted by: @import:stackexchange-codereview··
0
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.

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 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.