Program that finds the longest word not containing one of the disallowed characters Problem definition

Find the longest word that doesn't contain any of the following characters: 

gkmqvwxz


Input comes from a file named words.txt and contains one word per line. A word is anything that is not a whitespace.



The twist

My C++ implementation posted below is surprisingly slow, taking 1.7 seconds to run, whereas my javascript implementation (link, will obviously be slower because it's online) takes only 55ms. Is it possible to somehow optimize my C++ implementation?

#include &lt;iostream&gt;
#include &lt;fstream&gt;
#include &lt;string&gt;
#include &lt;vector&gt;
#include &lt;cstdint&gt;

#include "TimeUtil.h"

int main()
{
    uint64_t begin_time = currentTimeNanoseconds();

    std::string longestAcceptableWord; // Variable to contain the longest word

    std::string currentLine;
    const char badLetters[] = { 'g', 'k', 'm', 'q', 'v', 'w', 'x', 'z' };

    std::ifstream in;
    in.open("words.txt");
    while (!in.eof()) // Loop through the entire file (466 544 lines, 4 749 kB).
    {
        getline(in, currentLine);

        if (currentLine.length() &lt;= longestAcceptableWord.length())
        {
            continue;
        }

        // If the word contains one of the bad letters, don't accept it.
        for (unsigned int i = 0; i &lt; sizeof(badLetters); i++)
        {
            if (currentLine.find(badLetters[i]) != std::string::npos)
            {
                continue;
            }
        }

        // If the word is longer than the current longest word found, 
        // and doesn't have any bad letters, make it the new longest 
        // word found.
        longestAcceptableWord = currentLine;
    }
    in.close();

    std::cout &lt;&lt; "[" &lt;&lt; (double) ((currentTimeNanoseconds() - begin_time) / 1000) / 1000.0f &lt;&lt; "ms] " &lt;&lt; longestAcceptableWord &lt;&lt; std::endl;

    return 0;
}
I see a number of things that may help you improve your program.

Fix the bug

There is a subtle bug in the original implementation.  It contains these lines:


// If the word contains one of the bad letters, don't accept it.
for (unsigned int i = 0; i &lt; sizeof(badLetters); i++) {
    if (currentLine.find(badLetters[i]) != std::string::npos) {
        continue;
    }
}



The intent is, of course, to escape the for loop and go on if a bad letter is found in a candidate word.  However, what actually happens is that first letter is checked, and the loop continues no matter if the letter was found or not.  That's a bug and one reason I don't much like continue.  It's too easy to make this kind of error.

Don't loop on eof()

It's almost always incorrect to loop on eof() while reading a file.  The reason is that the eof indication is only set when an attempt is made to read something from the file when we're already at the end.  So instead of this:


while (!in.eof()) {
    getline(in, currentLine); 
    // ...
}



write this:

while (getline(in, currentLine)) { 
    // ...
}


See this question for more details on why using eof is usually wrong.

Initialize variables on declaration

The best practice is to initialize variables as soon as they're created.  In C++, this most often means initializing them in the same line as the declaration.  So instead of this:


std::ifstream in;
in.open("words.txt");



write this:

std::ifstream file2{wordsFile};


Note, too, that I use the C++11 uniform initializer syntax (with the {}) to make it clear to both the compiler and the reader that this is an initialization and not a function call.  See Stroustrup's description for more details on that.

Allow the user to specify input files

The words file name is currently hardcoded which certainly greatly restricts the usefulness of the program.  Consider using argc and argv to allow the user to specify file names on the command line.  

Prefer newer structures to plain arrays

Instead of a plain array as with badLetters, one could instead use an std::array or std::string.  If your compiler has C++17 support, std::string_vew might be an even better choice.

const std::array&lt;char, 8&gt; badLetters = { 'g', 'k', 'm', 'q', 'v', 'w', 'x', 'z' };


Use standard algorithms

Instead of searching with a for loop for all badLetters, we can use std::find_first_of() instead.  The result would look like this:

while (getline(in, currentLine)) {
    if (currentLine.length() &gt; longestAcceptableWord.length()
        &amp;&amp; (std::find_first_of(currentLine.begin(),
                            currentLine.end(),
                            badLetters.begin(),
                            badLetters.end()) == currentLine.end())
    ) {
        longestAcceptableWord = currentLine;
    }
}


Note that I prefer to avoid continue and instead rely on the short-circuit evaluation of the if clauses to cause the same effect.

Don't use std::endl if you don't really need it

The difference between std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream.  This can be time-consuming in a program with a lot of I/O and is rarely actually needed.  It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one.  Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.