2
\$\begingroup\$

Previous question:

Counting words in files - follow-up 2

For simplifying the code, I have implemented std::regex rather than templates.

How can I improve this code further?

//
//program name: WordCounter
//
//usage: C:\>WordCounter.exe test.txt test1.txt 
//
#include <iostream>
#include <iomanip>
#include <fstream>
#include <unordered_map>
#include <string>
#include <vector>
#include <algorithm>
#include <regex>

using Words = std::unordered_map<std::string, std::size_t>;

template<typename T>
std::string getToken(T begin, T end, std::regex e = std::regex{ R"([^.,\n\r]+)" })
{
    std::smatch match{};

    std::string result{};

    if (std::regex_search(begin, end, match, e))
    {
        result.append(match.str());
    }

    return result;
}

bool loadFromFile(const std::string& fileName, Words& words)
{
    std::ifstream file(fileName);

    if (!file)
    {
        return false;
    }

    for (std::string word; file >> word;)
    {
        ++words[getToken(word.cbegin(), word.cend())];
    }

    return true;
}

template<typename T>
void showResult(const T& begin, const T& end)
{
    std::vector<T> words;

    words.reserve(std::distance(begin, end));

    for (auto i = begin; i != end; ++i)
    {
        words.push_back(i);
    }

    std::sort(words.begin(), words.end(),
        [&](const T& it1, const T& it2)
    {
        return it1->second > it2->second;
    });

    int x = 0;

    for (const auto& i : words)
    {
        if (x++ == 0)
        {
            std::cout << "\nThe most repeated element is: " << i->first << "\n\n";

            std::cout << " Word\tRepeated Times";

            std::cout << '\n' << std::setfill('-') << std::setw(23) << '\n';
        }

        std::cout << ' ' << std::setfill(' ')
            << std::setw(10) << std::left << i->first
            << std::setw(6) << std::internal << i->second << '\n';
    }
}

int main(int argc, char *argv[])
{
    Words words;

    for (int i = 1; i < argc; ++i)
    {
        if (!loadFromFile(argv[i], words))
        {
            std::cerr << "Can't open the file " << argv[i] << '\n';
        }
    }

    showResult(words.begin(), words.end());
}
\$\endgroup\$
2
\$\begingroup\$

Overall, this looks pretty nice. There are few minor things that can be improved though, about this part:

int x = 0;

for (const auto& i : words)
{
    if (x++ == 0)
    {
        std::cout << "\nThe most repeated element is: " << i->first << "\n\n";

        std::cout << " Word\tRepeated Times";

        std::cout << '\n' << std::setfill('-') << std::setw(23) << '\n';
    }

    // ...
}

i is customary for loop variables that are simple counters. In this case it is a word (well, sort of), so it would be better to name it as such.

The variable x is a bit confusing:

  • its name tells nothing of its purpose
  • it's important for the first iteration of the loop
  • ... but it keeps counting during all iterations

It would be clearer and better this way:

bool first = true;

for (const auto& word : words)
{
    if (first)
    {
        std::cout << "\nThe most repeated element is: " << word->first << "\n\n";

        std::cout << " Word\tRepeated Times";

        std::cout << '\n' << std::setfill('-') << std::setw(23) << '\n';

        first = false;
    }

    // ...
}
\$\endgroup\$
1
\$\begingroup\$

I think you should include (actually exclude) more characters in your regex, like parentheses, question and exclamation marks or even change it to something like (\w+) to leave only letters in the match because ifstreams file >> word breaks on spaces.

\$\endgroup\$
  • \$\begingroup\$ thanks for answer. actually my txt file has only , . punctuations, so i need only these. \$\endgroup\$ – MORTAL Dec 14 '14 at 7:08
  • \$\begingroup\$ for whitespace i had content of file in unordered_map. it stores each word in it. \$\endgroup\$ – MORTAL Dec 14 '14 at 7:17

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.