Outputting the names of cars, without repetitions, with the number of occurrences in order of decreasing repetitions

A short while ago, I have submitted a coding exercise to a
potential employer. The response came back the next
morning and you can guess what it was from the subject
of this post.

I am not totally at loss, but I need another programmer's
perspective. Is there anything that jumps out?

The idea of the exercise is simple: I'm given an input file with
names of cars, one per line, possibly repeated and in no
particular order.

The program should output the same names, except with no
repetitions, the number of occurrences listed next to each
car, and in order of decreasing repetitions.

Example:

Honda\n Audi\n Honda\n -> Honda 2 \n Audi 1\n

```
#include
#include
#include
#include
#include
#include

using namespace std;

// helper functions ///////////////////////////////////////

// reads lines from instream
void collect_lines(istream &in, map &lines);

// given lines->num_occurs map, reverses mapping
void reorg_by_count(map &lines,
multimap &bycount);
///////////////////////////////////////////////////////////

int main(int ac, char* av[])
{
istream *in;
map *lines = new map();
multimap *lines_by_count = new multimap();

if (ac good()) return 1;

collect_lines(in, lines);
reorg_by_count(lines, lines_by_count);

if (in != &cin)
{
((ifstream *)in)->close();
delete in;
}

cout ::reverse_iterator it
= lines_by_count->rbegin();

for (; it != lines_by_count->rend(); it++)
{
cout second first &lines)
{
string tmp;

while (in.good())
{
getline(in, tmp);

int i = 0;

// trim initial space (also skips empty strings)
for (i = 0; i = tmp.length()) continue;
tmp = tmp.substr(i);

for (i = 0; i num_occurs map, reverses mapping
void reorg_by_count(map &lines,
multimap &bycount)
{
map::iterator it =

Problems I see:

My problem with your code is that you are newing a lot of stuff that should just be objects.

map *lines = new map();
multimap *lines_by_count = new multimap();

Both of these should just be plain objects.

map        lines;
multimap   lines_by_count;

This one fact would have caused you to be rejected. I would have seen this and I would not have read any-further into your code straight onto the reject pile. This fundamental flaw in your style shows that you are not a C++ programmer.

Next the objects you new are stored in RAW pointers. This is a dead give away that you are not an experienced C++ programmer. There should practically never be any pointers in your code. (All pointers should be managed by an object). Even though you manually do delete these two it is not done in an exception safe way (so they can still potentially leak).

You are reading a file incorrectly.

while (in.good())
{
    getline(in, tmp);

This is the standard anti-pattern for reading a file (even in C). The problem with your version is that the last successful read will read upto but not past the EOF. Thus the state of the file is still good but there is now no content left. So you re-enter the loop and the first read operation getline() will then fail. Even though it can fail you do not test for that.

I would expect to see this:

while (getline(in, tmp))
{
    // Line read successfully
    // Now I can processes it
}

Next you are showing a fundamental misunderstanding of how maps work:

if (lines.count(tmp) == 0)
    {
        lines[tmp] = 0;
    }
    lines[tmp]++;

If you use the operator[] on a map it always returns a reference to an internal value. This means if the value does not exist one will be inserted. So there is no need to do this check. Just increment the value. If it is not their a value will be inserted and initialized for you (thus integers will be zero). Though not a big problem its usually preferable to use pre-increment. (For those that are going to say it does not matter. On integer types it does not matter. But you have to plan fro the future where somebody may change the type to a class object. This way you future proof your code against change and maintenance problems. So prefer pre-increment).

You are doing extra work you don't need to:

// trim initial space (also skips empty strings)
for (i = 0; i < tmp.length() && !isalnum(tmp[i]); i++);

The streams library already discards spaces when used correctly. Also the ';' at the end of the for. This is considered bad practice. It is really hard to spot and any maintainer is going to ask did he really mean that. When you have an empty body it is always best to use the {} and put a comment in their {/Deliberately empty/}

Here you are basically lower casing the string.

for (i = 0; i < tmp.length(); i++)
    {
        if (!isalnum(tmp[i]))
        {
            tmp[i] = ' ';
        }

You could use the C++ algorithms library to do stuff like this:

std::transform(tmp.begin(), tmp.end(), tmp.begin(), ::tolower());
                           //                       ^^^^^^^^^^^ or a custom 
                           //                        functor to do the task

Const correctness.

void reorg_by_count(map &lines, multimap &bycount)

The parameter lines is not mutated by the function nor should it be. I would expect it to be passed as a const reference as part of the documentation of the function that you are not going to mutate it. This also helps in future maintenance as it stops people from accidentally mutating the object in a way that later code would not expect.

My final thing is I did not see any encapsulation of the concept of a car. You treated it all as lines of text. If you had invented a car object you can define how cars are read from a stream and written to a stream etc. Thus you encapsulate the concept in a single location.

I would have done something like this:

Probably still overkill.

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

class Car
{
    public:
        bool operator>(std::istream& stream, Car& self)
        {
            std::string   line;
            std::getline(stream, line);

            std::stringstream linestream(line);
            linestream >> self.name;  // This strips white space

            // Lowercase the name
            std::transform(self.name.begin(), self.name.end(), self.name.begin(), ::tolower);
            // Uppercase first letter because most are proper nouns
            self.name[0] = ::toupper(self.name[0]);
            return stream;
        }
        friend std::ostream& operator  count;

    Car  nextCar;
    while(cars >> nextCar)
    {
        ++count[nextCar];
    }

    // PS deliberately left the sorting by inverse order as an exercise.
    for(auto const& car: count) {
        std::cout << car.first << ":   " << car.second << "\n";
    }
}

StackExchange Code Review Q#3714, answer score: 98