Count word frequencies, and print them most-frequent first

Question

I'm writing a comparison of a simple word-counting program in various languages. The task is to count the frequencies of unique, space-separated words in the input, and output the results most frequent first. Case should be normalized to lowercase (ASCII is okay).

But it's been a long time since I've written C++ (before the C++11 days). Is this idiomatic modern C++? Any improvements I could make to make this more idiomatic or simpler (I'm not looking for efficiency improvements here). I want to stick to the standard library.

#include <algorithm>
#include <iostream>
#include <string>
#include <unordered_map>
#include <vector>

using namespace std;

int main() {
    string word;
    unordered_map<string, int> counts;
    while (cin >> word) {
        transform(word.begin(), word.end(), word.begin(),
            [](unsigned char c){ return tolower(c); });
        counts[word]++;
    }

    vector<pair<string, int>> ordered(counts.begin(), counts.end());
    sort(ordered.begin(), ordered.end(), [](auto &a, auto &b) {
        return a.second > b.second;
    });

    for (auto count : ordered) {
        cout << count.first << " " << count.second << "\n";
    }
    return 0;
}

Answer 1

Mainly this looks fine.

• using namespace std; is a bad habit to get into due to potential name collisions. Instead prefer to type std:: where necessary.

• counts[word]++;. We don't need an un-incremented copy of the count, so we should use pre-increment here instead: ++counts[word];

• sort(ordered.begin(), ordered.end(), [](auto &a, auto &b) we don't alter a or b in the lambda, so these should be const&.

• for (auto count : ordered) copies the pairs. Again, we should be using auto const& here.

• return 0; we don't need this, as the compiler will add it for us automatically. We might also use a named constant instead of 0, specifically EXIT_SUCCESS from <cstdlib>.

Answer 2

user673679's answer deals with low-level review; I'll look at the algorithm.

First, top marks for avoiding the most common error with functions from <cctype> - it's vitally important to pass the value as unsigned char promoted to int, rather than plain char.

Rather than writing my own loop for reading the words, I would consider transform() from an input-stream iterator to a map inserter. That said, we'd need to construct a custom iterator for the inserter we want. It would be very straightforward if we were using a std::multiset, but that would use more memory (as it stores all the added objects). We could create a "counter" class with appropriate push_back(), if we were likely to use it again.

After the while loop, we should test std::cin.bad() (or set the stream to throw on error). At present, any stream error is ignored, and we proceed as if we'd read the entire input, giving misleading results.

Instead of populating a vector, and subsequently sorting it, we might prefer to insert directly into an ordered container (std::set perhaps).

Something we can do in modern C++ is to write nested functions, by assigning a lambda expression to a variable. This may be clearer than writing the lambda inline. Or we can use the anonymous namespace for functions with static linkage.

Here's my modification of the code to have no explicit loops, or indeed any flow-control statements:

#include <algorithm>
#include <cctype>
#include <iostream>
#include <iterator>
#include <set>
#include <string>
#include <unordered_map>
#include <utility>

namespace {
    auto downcase(std::string s) {
        std::transform(s.begin(), s.end(), s.begin(),
                       [](unsigned char c){ return std::tolower(c); });
        return s;
    }
}

int main()
{
    using counter = std::unordered_map<std::string, unsigned>;
    using in_it = std::istream_iterator<std::string>;
    using out_it = std::ostream_iterator<std::string>;

    counter counts;
    auto insert = [&](std::string s) { ++counts[downcase(std::move(s))]; };

    // read words into counter
    std::cin.exceptions(std::istream::badbit);
    std::for_each(in_it{std::cin}, in_it{}, insert);

    // sort by frequency, then alphabetical
    auto by_freq_then_alpha = [](const auto &a, const auto &b) {
        return std::pair{ b.second, a.first } < std::pair{ a.second, b.first};
    };
    const std::set ordered{counts.begin(), counts.end(), by_freq_then_alpha};

    // write the output
    auto format = [](const auto& count) {
        return count.first + ' ' + std::to_string(count.second) + '\n';
    };
    std::transform(ordered.begin(), ordered.end(), out_it{std::cout}, format);
}

---

For a more modern take, C++20 includes the Ranges library, which lets us use views to transform collections:

int main()
{
    using counter = std::unordered_map<std::string, unsigned>;

    counter counts;
    auto insert = [&counts](std::string s) { ++counts[downcase(std::move(s))]; };

    // read words into counter
    std::cin.exceptions(std::istream::badbit);
    auto input_words = std::ranges::istream_view<std::string>(std::cin);
    std::ranges::for_each(input_words, insert);

    // sort by frequency, then alphabetical
    auto by_freq_then_alpha = [](const auto &a, const auto &b) {
        return std::pair{ b.second, a.first } < std::pair{ a.second, b.first};
    };
    const std::set ordered{counts.begin(), counts.end(), by_freq_then_alpha};

    // write the output
    auto write_out = [](const auto& count) {
        std::cout << count.first << ' ' << count.second << '\n';
    };
    std::ranges::for_each(ordered, write_out);
}

---

I'm not saying that either of these is necessarily what you should write; at least for now, the range-based loops are more familiar to most C++ programmers, and so probably clearest. However, it showcases some of the options we have in modern C++.

Answer 3

Added: structured bindings

Changed: to using range based algorithms. (C++20) This removes the need for begin()/end() and you can use a projection in the sort call.

Removed: lambdas - not needed

#include <algorithm>
#include <iostream>
#include <string>
#include <unordered_map>
#include <vector>

using namespace std;

int main()
{
    unordered_map<string, int> counts;

    string word;
    while (cin >> word) {
        ranges::transform(word, word.begin(), ::tolower);
        counts[word]++;
    }

    using word_count = pair<string, int>;

    vector<word_count> ordered(counts.begin(), counts.end());
    ranges::sort(ordered, greater{}, &word_count::second);

    for (auto [word, count] : ordered) {
        cout << word << " " << count << "\n";
    }

}