Find the most frequent integer in an array

I have been a Python developer for a few years and I'm going through some simple exercises to learn C++.

Please consider the following Python snippet

# Find the most frequent integer in an array
lst = [1,2,4,3,1,3,1,4,5,15,5,2,3,5,4]
mydict   = {}
cnt, itm = 0, ''

for item in lst:
     mydict[item] = mydict.get(item, 0) + 1
     if mydict[item] >= cnt :
         cnt, itm = mydict[item], item

print(itm)

It may not be very clean and it misses elements that tie for most frequent, but it is succinct code. I re-wrote this to C++11 as:

#include 
#include 
#include 

int main(int argc, char const *argv[])
{
    using std::cout;

    int foo [] = {1,2,4,3,1,3,1,4,5,15,5,2,3,5,4};

    std::unordered_map dict;

    std::string item;

    int count = 0;

    std::string itm;

    for (int i : foo) {
        item = std::to_string(i);

        auto it = dict.find(item);

        if (it != dict.end()) {
            it->second = it->second++;

            if (it->second >= count) {
                count = it->second;
                itm = item;
            }
        } else {
            dict.insert(std::make_pair(item, 0));
        }

    }

    std::cout << "The integer most frequent is " << itm << "\n";
    return 0;
}

This executes fine and returns 4 which is one of the correct answers. Can someone please offer a re-write of this to make it more succinct.

At present I am not concerned about if there's more than one element in the list that has a tie for most frequent. I am more concerned with cleaner code.

Your original Python code translates line-for-line into C++11 as:

#include 
#include 
#include 
#include 

int main()
{
    std::vector lst = {1,2,4,3,1,3,1,4,5,15,5,2,3,5,4};
    std::map mydict = {};
    int cnt = 0;
    int itm = 0;  // in Python you made this a string '', which seems like a bug

    for (auto&& item : lst) {
        mydict[item] = mydict.emplace(item, 0).first->second + 1;
        if (mydict[item] >= cnt) {
            std::tie(cnt, itm) = std::tie(mydict[item], item);
        }
    }

    std::cout << itm << std::endl;
}

The only newly introduced lines here are the #include lines, the lines that define main(), and a couple of closing curly braces.

Your C++ code could definitely be cleaned up by removing a lot of its blank lines and premature declarations; for example, there's no need to put a blank line in between every pair of declarations, and there's no need to pre-declare item outside the for-loop. Just tightening up the style (whitespace and such) will go a long way toward readability.

You have a bug on this line:

it->second = it->second++;

In C++, it->second++ is a side-effecting expression: it means "actually increment it->second, and then return the old value." So (in C++11 and C++14), this statement as a whole has undefined behavior: it's telling the compiler that you want to increment it->second, but also assign a new value to it->second.

In C++17, this statement may or may not have defined behavior; I'll have to read up on it and get back to you. Certainly it->second++ is guaranteed to be evaluated prior to the assignment, but I don't know if that completely solves the problem.

Anyway, what you meant to write was simply

it->second = it->second+1;

or

++it->second;

Stylistically, I advise writing "increment thing" instead of "thing increment", intuitively because that's how English works, but also because it can be an optimization in some cases involving overloaded operator++ (and is never a pessimization).

Your code actually doesn't compile with any C++11 compiler I have access to, because of your misuse of the std::make_pair function template. You wrote

std::make_pair(item, 0)

but you should have written

std::make_pair(item, 0)

so that the compiler would deduce the types for you. (And in this case it deduces std::make_pair; but you as the programmer shouldn't have to care about that. Just let the compiler do its job and you try to stay out of its way as much as possible.)

The function parameters argc and argv are unused, so you should get rid of them. int main() is perfectly fine in both C and C++.

I don't understand why you write using std::cout; but then proceed to use the fully qualified names of std::string, std::make_pair, and so on. I would think that you'd either use the fully qualified names of everything from std:: (my preferred choice), or else go all out and using std::... a ton of things.

Anyway, using std::cout doesn't save you any typing — it actually costs you two lines and about 10 characters — so you'd do well to get rid of it.

As far as improving your C++ skills to the point where you can translate Python line-for-line as I did above, the best advice I can give you is go to cppreference.com and study the standard library! The STL is way better today than it was in 2003, and it takes ideas from many other languages. So for example the C++ equivalent of mydict.get(item, 0) is this humongous ugly expression mydict.emplace(item, 0).first->second (and I'm not actually saying you should write that ugly expression in production code!), but it does exist, and translation from Python to C++ is just a matter of being able to remember vaguely what it's called and look it up. And that just takes a lot of practice.

StackExchange Code Review Q#156753, answer score: 15