C++ problem in calling a function in main that prints the map.

Question

Hi,

I am trying to print the contents of the map and this is where my code fails. I have tested all my methods and I have no problem to read from file, filer the word, put it into map, and even the print function is working. However, when I am calling the printer function from main it does not print the map. I am new to polymorphism and I think that my error is in how I am passing the map to the function in main.

Here is my main class:

using namespace std;
#include <iostream>
#include "ReadWords.h"
#include "ReadPunctWords.h"
#include "ReadNumWords.h"
#include "ReadCapWords.h"
#include "MapWorks.h"
#include <fstream>
#include <string>
#include <map>
#include <iterator>

/**
 * This main function uses all other classes.
 */
int main() {


   char* name = "RomeoJuliet.txt";
   //ReadPunctWords &obj = *new ReadPunctWords(name);
   ReadPunctWords obj(name);
   string startSearch="BEGIN";
   string endSearch="FINIS";


   ReadPunctWords rpw;
   ReadCapWords rcw;
   ReadNumWords rnw;
   MapWorks mw;

   while(rpw.isNextWord()){
       string tempword = obj.getNextWord();
       if(tempword == startSearch){
           break;
       }
   }
   while(rpw.isNextWord()){
       string tempword = obj.getNextWord();
       if(tempword == endSearch){
           break;
       }
       else{
               if(rpw.filter(tempword)){
                   mw.addToMap(tempword, mw.mapPunct);
               }

               if(rcw.filter(tempword)){
                   mw.addToMap(tempword, mw.mapCap);
               }

               if(rnw.filter(tempword)){
                   mw.addToMap(tempword, mw.mapNum);
               }
           }
   }


   mw.printMap(mw.mapPunct);
   mw.printMap(mw.mapCap);
   mw.printMap(mw.mapNum);


   //clear map
   mw.clearMap(mw.mapPunct);
   mw.clearMap(mw.mapCap);
   mw.clearMap(mw.mapNum);

   //close the file
   //obj.close();


   //delete &obj;

   //exit(0); // normal exit
   return 0;

}

And my MapWorks.cpp which contains the maps and the functions related to maps:

using namespace std;
#include <iostream>
#include <string>
#include <map>
#include <iterator>
#include "MapWorks.h"

/**
 * MapWorks class builds the maps and does the map processing and printing
 */


MapWorks::MapWorks() {}

void MapWorks::addToMap(string myword, map<string, int> & myMap){
    int n = myMap[myword];
    myMap[myword]= n+1;
}


void MapWorks::printMap (map<string, int> &myMap){

    for (map<string, int>::iterator it = myMap.begin(); it != myMap.end(); ++it)
    {
        cout << it->first << " ==> " << it->second << '\n'<<endl;
    }
}


//delete entries in map
void MapWorks::clearMap(map<string, int>myMap) {
    myMap.clear();

}

MapWorks.h :

#ifndef MAPWORKS_H
#define MAPWORKS_H
#include <string>
#include <map>
using namespace std;


/**
 * MapWorks class builds the maps and does the map processing and printing
 */

class MapWorks {
    public:

    map<string, int> mapPunct; //(word, number of occurences)
    map<string, int> mapNum; //(word, number of occurences)
    map<string, int> mapCap; //(word, number of occurences)

    MapWorks();

    void addToMap(string myword, map<string, int> & myMap); //adds words to a map

    void printMap (map<string, int> &myMap); //prints the map

    void clearMap(map<string, int>); //clear map
};

#endif

My ReadWords.h :

/**
 * ReadWords class, the base class for ReadNumWords, ReadPunctWords, ReadCapWords
 */

#ifndef READWORDS_H
#define READWORDS_H

using namespace std;
#include <string>
#include <fstream>
#include<iostream>

 class ReadWords
 {
   private:
     string nextword;
     ifstream wordfile;
     bool eoffound;

   public:
    /**
     * Constructor. Opens the file with the default name "text.txt".
     * Program exits with an error message if the file does not exist.
     */
     ReadWords();

    /**
     * Constructor. Opens the file with the given filename.
     * Program exits with an error message if the file does not exist.
     * @param filename - a C string naming the file to read.
     */
     ReadWords(char *filename);

    /**
     * Closes the file.
     */
     void close();

    /**
     * Returns a string, being the next word in the file.
     * @return - string - next word.
     */
     string getNextWord();

    /**
     * Returns true if there is a further word in the file, false if we have reached the
     * end of file.
     * @return - bool - !eof
     */
     bool isNextWord();

     //pure virtual function for filter
     virtual bool filter(string word)=0;

    /**
     * Fix the word by the definition of "word"
     * end of file.
     * @return - string
     */
     string fix(string word);
 };

 #endif

And my ReadPunctWords (ReadNumWords and ReadCapWords are quite the same, just checking if the word has digits or capital letters instead of punctuations like in here):

#ifndef READPUNCTWORDS_H
#define READPUNCTWORDS_H
using namespace std;
#include <string>
#include "ReadWords.h"

/**
 * ReadPunctWords inherits ReadWords, so MUST define the function filter.
 * It chooses to override the default constructor.
 */
class ReadPunctWords: public ReadWords {
    public:
    ReadPunctWords();
    ReadPunctWords(char *filename): ReadWords(filename){};
    virtual bool filter(string word);
};

#endif

I would appreciate any help from you. Thanks, Adriana

Answer 1

You forgot to increment iterator:

while(it!=myMap.end()){
    cout<<(*it).first<<" ==> "<<(*it).second<<endl;
    // you forgot this:
    it++;
}

And, more importantly, consider few modifications to your code:

// ReadPunctWords &obj = *new ReadPunctWords(name);
// should likely be:
ReadPunctWords obj(name);
// same applies to other 'newed' 'references'
// and then there's no need to do
// delete &obj;

// exit(0); // normal exit
// should probably be just a
return 0;

// obj.close();
// can be called in the destructor of ReadPunctWords class
// and RAII will help you get your file closed correctly when needed

// void MapWorks::printMap (map<string, int>myMap)
// should better be:
void MapWorks::printMap (const std::map<string, int> &myMap)
// same applies to other functions in your code

// here's how your commented-out function could look like
void MapWorks::printMap(const std::map<string, int> &myMap) {
    typedef std::map<string, int>::iterator mapsi;
    for (mapsi m = myMap.begin(); m != myMap.end(); ++m) {
        std::cout << (*m).first << " ==> " << (*m).second << "\n";
    }
}

// void MapWorks::addToMap(string myword, map<string, int>myMap)
// should be:
void MapWorks::addToMap(std::string myword, std::map<string, int> &myMap)

Answer 2

There are a number of things that are potential issues in your code, but the most obvious thing that may be causing the printMap not to work as expected is this while loop.

map<string, int>::iterator it = myMap.begin();
cout<<"test"<<endl;
while(it!=myMap.end()){
cout<<(*it).first<<" ==> "<<(*it).second<<endl;
}

Nowhere do you increment the iterator so either nothing will be printed (if the map is empty) or else the first item will printed over and over again and the loop won't terminate.

The idiomatic way to write this loop would be as a for loop.

for (std::map<string, int>::iterator it = myMap.begin(); it != myMap.end(); ++it)
{
    std::cout << it->first << " ==> " << it->second << '\n';
}

The other issue is that your addToMap function probably isn't working as intended because you pass the map to the function by value and this means that the map that the function is adding an item to is actually a copy of the map that was passed in.

When control is passed to the calling function this copy is destroyed and the map that was passed it is still empty.

To pass a map by reference you need to add & to the type of the parameter in the function declaration.

i.e. in the headfile, the the MapWorks class definition:

void addToMap(string myword, map<string, int>& myMap);

and in the source file:

void MapWorks::addToMap(string myword, map<string, int>& myMap)
{
    // definition...
}

---

Your use of references for dynamically allocated objects is unusual, to say the least. For your purposes, I don't see any point to doing:

ReadWords &rnw = *new ReadNumWords();

when you delete the object at the end of the same function in which it is created. You can just do this (exactly as you do with MapWorks mw;).

ReadNumWords rnw;

If you have to use dynamically allocated objects, just using pointers rather than references is much more usual but it is highly recommended to use some sort of a smart pointer so that you don't have to remember to call delete explicitly.

Answer 3

If possible, I'd suggest breaking the logic up into slightly smaller units, and pushing more of the logic into the classes -- right now, main does quite a lot more than I'd like to see there, and (particularly) knows more about the internals of the classes than I'd like to see either.

If I were doing it, I'd start with a map that knew how to filter out words, so it can only accept what it's supposed to:

class Map { 
    std::map<std::string, int> counts;
public:
    struct Filter { 
        virtual bool operator()(std::string const &) const = 0;
    };

    Map(Filter const &f) : filter(f) {}

    bool InsertWord(std::string const &word) { 
        return filter(word) && (++counts[word] != 0);
    }

    friend std::ostream &operator<<(std::ostream &os, Map const &m) { 
        std::copy(m.counts.begin(), 
                  m.counts.end(), 
                  std::ostream_iterator<count>(std::cout, "\n"));
        return os;
    }
private:
    Filter const &filter;
};

Then we'd need some derivatives of Filter to do the real filtering. These probably don't work the way you really want; they're really just placeholders:

struct Num : Map::Filter { 
    bool operator()(std::string const &w) const {
        return isdigit(w[0]) != 0;
    }
};

struct Punct : Map::Filter { 
    bool operator()(std::string const &w) const { 
        return ispunct(w[0]) != 0;
    }
};

struct Letter : Map::Filter { 
    bool operator()(std::string const &w) const { 
        return isalpha(w[0]) != 0;
    }
};

Then MapWorks can delegate almost all the real work to the Map (which in turn uses a Filter):

class MapWorks { 
    Map num;
    Map punct;
    Map letter;
public:

    // For the moment, these allocations just leak.
    // As long as we only create one MapWorks object,
    // they're probably not worth fixing.
    MapWorks() 
        : num(Map(*new Num())), 
          punct(Map(*new Punct())), 
          letter(Map(*new Letter())) 
    {}

    // Try adding the word until we find a Map 
    // that accepts it.
    bool push_back(std::string const &word) {
        return num.InsertWord(word) 
            || punct.InsertWord(word) 
            || letter.InsertWord(word);
    }

    // Write out by writing out the individual Map's:
    friend std::ostream &operator<<(std::ostream &os, MapWorks const &m) {
        return os << m.num << "\n" << m.punct << "\n" << m.letter << "\n";
    }       
};

With these in place, main becomes pretty simple: (though for the moment, I've just had it read a whole file instead of looking for "BEGIN" and "FINIS"):

int main() { 
    MapWorks m;

    std::string temp;
    while (std::cin >> temp)
        m.push_back(temp);

    std::cout << m;
    return 0;

}

There are a few other bits and pieces, such as typedef'ing the count type and defining an inserter for it, but they're pretty minor details.