Return to Revisions

The code is reasonably good. There are some inefficiencies and small bug that doesn't affect the result.

Bug

!in.eof(), .eof() is never true until its read. It should be the getline instead.

Inefficiencies

• reading line by line

It is in general better to just read into string variable, it is clearer and probably more efficient.

• Manually closing the file

Standard file streams close themselves when they get out of scope, which end of main in this case.

Alternative implementation

Now, lets eliminate the main culprit, string validity check:

bool is_valid_word(std::string_view word) {
    for (char letter: word) {
        switch (letter) {
            case 'g':
            case 'k':
            case 'm':
            case 'q':
            case 'v':
            case 'w':
            case 'x':
            case 'z':
                return false;
            default:
                ; //silence warnings
        }
    }

    return true;
}

The main change is to use jump table (switch statement) with fallthrough. The other change is to use std::string_view, immutable non owning view into string.

The next thing to care about is iteration through the file. My personal preference is to use a pair of iterators:

std::ifstream file("words.txt");
auto first = std::istream_iterator<std::string>(file);
auto last = std::istream_iterator<std::string>();

And the algorithm itself:

template <typename InputIterator>
std::string longest_valid_word(InputIterator first, InputIterator last) {
    std::string longest_word;
    while (first != last) {
        auto word = *first;
        if (word.size() > longest_word.size() && is_valid_word(word)) {
            longest_word = std::move(word);
        }
        ++first;
    }

    return longest_word;
}

Note that the above algorithm can run on anything that provides a pair of iterators, e.g. std::vector, std::list, etc.

Full code

No demo for this post, as I don't want to put the weight onto wandbox:

#include <string_view>
#include <utility>
#include <string>

bool is_valid_word(std::string_view word) {
    for (char letter: word) {
        switch (letter) {
            case 'g':
            case 'k':
            case 'm':
            case 'q':
            case 'v':
            case 'w':
            case 'x':
            case 'z':
                return false;
            default:
                ; //silence warnings
        }
    }

    return true;
}

template <typename InputIterator>
std::string longest_valid_word(InputIterator first, InputIterator last) {
    std::string longest_word;
    while (first != last) {
        auto word = *first;
        if (word.size() > longest_word.size() && is_valid_word(word)) {
            longest_word = std::move(word);
        }
        ++first;
    }

    return longest_word;
}

#include <fstream>
#include <iterator>
#include <chrono>
#include <atomic>
#include <iostream>

int main() {
    std::ios_base::sync_with_stdio(false);
    std::ifstream file("words.txt");
    auto first = std::istream_iterator<std::string>(file);
    auto last = std::istream_iterator<std::string>();

    using namespace std::chrono;
    auto start_time = system_clock::now();
    //std::atomic_thread_fence(std::memory_order_seq_cst);
    auto found_word = longest_valid_word(first, last);
    //std::atomic_thread_fence(std::memory_order_seq_cst);
    auto end_time = system_clock::now();

    std::cout << "longest word is " << found_word << " and it took "
              << duration_cast<microseconds>(end_time - start_time).count() << " microseconds\n";
}

The atomic thread fences can be uncommented, but they don't really change much in this case. It is probably bottlenecked by performance of my SSD and of std::ifstream.

Conclusion

Well, I got around 90-100 milliseconds on my machine. I've got no idea why it runs so slowly, but I've exhausted my sane ideas. The other one would be to do some manual parsing of the file, which is much harder.