5
\$\begingroup\$

I made a simple program for my study that calculates words in a text files and prints every word and its repeated times in the files.

How can I improve this code?

#include <future>
#include <iostream>
#include <fstream>
#include <unordered_map>
#include <string>
#include <vector>
#include <algorithm>
#include <exception>

typedef std::unordered_map<std::string, std::size_t> Words;

Words wordsInFile(const std::string& fileName)
{
    std::ifstream file(fileName);
    if (!file.is_open())
    {
        throw "Can't open the file" + fileName;
    }

    Words loadFromFile;


    for (std::string word; file >> word;)
    {
        ++loadFromFile[word];
    }

    return loadFromFile;
}

int main(int argc, char *argv[])
{
    std::vector<std::future<Words>> futures;

    for (int i = 1; i < argc; ++i) 
    {
        futures.push_back(std::async([=] { return wordsInFile(argv[i]); }));
    }

    Words words;

    for (auto& i : futures) 
    {
        const auto results = i.get();

        for (const auto& j : results) 
        {
            words[j.first] = j.second;
        }
    }

    std::cout << "Word\tRepeated Times\n-------------------------\n";
    for (const auto& i : words)
        std::cout << i.first << "\t\t" << i.second << '\n';
}

test.txt

File system is huge subject need more work out. file is plain text file
This is some junk words. simple program for words counting in text file. 1 2 3
2 2 2 2
@ % & ^ *
all so good
this test text file

Output:

Word    Repeated Times
-------------------------
File            1
out.            1
1               1
system          1
2               5
is              3
need            1
huge            1
subject         1
work            1
more            1
%               1
file            3
plain           1
text            3
This            1
some            1
junk            1
program         1
words.          1
simple          1
for             1
words           1
counting                1
in              1
file.           1
so              1
3               1
@               1
&               1
^               1
this            1
*               1
all             1
good            1
test            1
\$\endgroup\$
  • \$\begingroup\$ why are you using futures? \$\endgroup\$ – sp2danny Dec 11 '14 at 11:26
  • \$\begingroup\$ If I get this right, you are counting the lines of each file in a separate task/tread (with async), correct? Interesting idea to optimize batch processing of several files. \$\endgroup\$ – glampert Dec 11 '14 at 13:12
  • \$\begingroup\$ BTW, the title should be either "Counting words in a file" (singular) or "Counting words in files" (plural). ;) \$\endgroup\$ – glampert Dec 11 '14 at 13:16
  • \$\begingroup\$ @sp2danny .. you can use std::thread and std::mutex but std::future and std::async are type-safe buildin and easy to implement them in your code like i did \$\endgroup\$ – MORTAL Dec 11 '14 at 16:55
  • 1
    \$\begingroup\$ @glampert .. exactly except that it stores each element from std::unordered_map that has all words which is return value from wordsInFile() function as shown above and assign by copy to a new std::unordered_map. yes, it can handle many files as well. \$\endgroup\$ – MORTAL Dec 11 '14 at 17:04
3
\$\begingroup\$

I see some things that may help you improve your code.

Simplify your code

You don't need a lambda at all. Use this instead:

    futures.push_back(std::async(wordsInFile,argv[i]));

Also, instead of if (!file.is_open()) use the more idiomatic if (!file).

Fix the counting

If we have two files which both contain the word fox exactly once then I would expect the result of this program to print fox 2 but in fact, it would falsely claim a count of 1. The problem is this line:

words[j.first] = j.second;

This overwrites the count instead of accumulating it. This is what you need instead:

words[j.first] += j.second;

Use reserve to prevent reallocations

This code already knows how many futures are going to be created. The code can be made a little more efficient by using this:

futures.reserve(argc-1);

With large numbers of files, this eliminates the re-allocation that would be required by dynamically resizing the vector.

Check for exceptions

If any of the file names can't be read. That is, if it can be opened but not read, as with a directory under Linux, this code will throw an error that isn't caught and so therefore results in a crash. Since future::get() will throw any error that was stored, it's that part of the code that should check for exceptions. One alternative would be this:

    Words results;
    try {
        results = i.get();
    }
    catch(std::exception &err)
    {
        std::cerr << "ERROR: " << err.what() << '\n';
    }
    catch(std::string &err)
    {
        std::cerr << "ERROR: " << err << '\n';
    }
    for (const auto& j : results) 
    {
        words[j.first] += j.second;
    }

Note that this will simply skip the offending file name but continue to process the rest.

Measure your results

Create a non-parallel version of this code and compare the timing to see if it's faster or slower than this version. I have found that it's often true that the overhead of the implicit (as in async) or explicit (as in thread) thread creation swamps the savings in time by making things parallel. The only way to know for sure is to time it.

\$\endgroup\$
  • \$\begingroup\$ i don't know why VC++ fail to compile without using lambda object in vector. i haven't test it yet GCC \$\endgroup\$ – MORTAL Dec 11 '14 at 17:38
  • \$\begingroup\$ bind works as well,futures.push_back(std::async(std::bind(wordsInFile, argv[i]))); in VC++ \$\endgroup\$ – MORTAL Dec 11 '14 at 17:44
  • 1
    \$\begingroup\$ If VC++ is using C++14 version of that feature, you might try an explicit policy: std::async(std::launch::async, wordsInFile, argv[i]). Also handy to check parallel vs. serial timing; simply change the policy to std::launch::deferred and you have a serial implementation. \$\endgroup\$ – Edward Dec 11 '14 at 19:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.