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
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) {
if (first->size() > longest_word.size() && is_valid_word(*first)) {
longest_word = *first;
}
++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>
#include <fstream>
#include <iterator>
#include <chrono>
#include <atomic>
#include <iostream>
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) {
if (first->size() > longest_word.size() && is_valid_word(*first)) {
longest_word = *first;
}
++first;
}
return longest_word;
}
int main() {
std::ifstream file("words.txt");
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(std::istream_iterator<std::string>(file), {});
//std::atomic_thread_fence(std::memory_order_seq_cst);
auto end_time = system_clock::now();
auto duration = duration_cast<milliseconds>(end_time - start_time);
std::cout << "[" << duration.count() << "ms] "
<< found_word << std::endl;
}
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.
Of course it didn't go as I expected :) this is what happens when optimizations are done in a rush. Let me break down why the code above is slower than what Toby and Edward (my speculations from results, of course):
Different version of alternative approach (uses std::getline)
std::string longest_valid_word(std::ifstream& is) {
std::string buffer;
std::string longest_word;
while (std::getline(is, buffer)) {
if (buffer.size() > longest_word.size() && is_valid_word(buffer)) {
longest_word = buffer;
}
}
return longest_word;
}
Just paste the function into the code and call it with the file stream, instead of calling the one with iterators.
letter of badLetters, notletter in badLetters. \$\endgroup\$