Return to Revisions

Problems I see:

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

map<string, int> *lines = new map<string, int>();
multimap<int, string> *lines_by_count = new multimap<int, string>();

Both of these should just be plain objects.

map<string, int> lines;
multimap<int, string> 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<string, int> &lines, multimap<int, string> &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 <algorithm>
#include <iterator>
#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <map>
#include <cctype>
class Car
{
 public:
 bool operator<(Car const& rhs) const {return name < rhs.name;}
 friend std::istream& 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<<(std::ostream& stream, Car const& self)
 {
 return stream << self.name << "\n";
 }
 private:
 std::string name;
};
int main(int argc, char* argv[])
{
 if (argc < 2)
 { exit(1);
 }
 std::ifstream cars(argv[1]);
 std::map<Car,int> 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";
 }
}