Pointer Issues 2 [Valid C++ Syntax]

Question

This version is working. I've made // comments throughout my code to better illustrate what I am having trouble with. The program is dependent on reading a text file. In the format of paragraphs with punctuation included.

One could copy this and the above into a text file and run the program.

// Word.cpp

#define _CRT_SECURE_NO_WARNINGS // disable warnings for strcpy
#define ARRY_SZ 100
#include <iostream>
#include <fstream>
#include "Word.h"

using namespace std;

Word::Word( const char* word )
{
    ptr_ = new char[ strlen( word ) + 1 ];
    strcpy( ptr_, word  );  
    len_ = strlen( ptr_ );
}

Word::Word( const Word* theObject ) 
{
    ptr_ = theObject->ptr_;
    len_ = theObject->len_;
}

Word::~Word()
{
    delete [] ptr_;
    ptr_ = NULL;
}

char Word::GetFirstLetterLower()
{
    // I want to use ptr_ and len_ here
    // but the information is gone!
    char c = 0;
    return c;
}

char* Word::GetWord()
{
    for (int x = 0; x < strlen( (char*)ptr_ ); x++)
        ptr_[x];  // Results in a crash.

    return ptr_;
}

// Word.h

const int FILE_PATH_SZ = 512;
class Word
{
private:
    char* ptr_;
    int len_;
public:
    Word( const Word* ); // an appropriate default constructor
    Word( const char* );
    ~Word( );
    char GetFirstLetterLower( );
    char* GetWord( );
    static char fileEntry[ FILE_PATH_SZ ];
};

// main.cpp

#ifdef  _DEBUG
#define _CRTDBG_MAP_ALLOC
#include <iostream>
#include <fstream>
#include <string>
#endif
#include "Word.h"
using namespace std;

const int WORD_SZ = 100;
Word** g_wordArray;
int g_arrSz;

static char filePath[ FILE_PATH_SZ ] = {};
void FreeWordArray();

int main( const int argc, const char **argv )
{
    int     wrdCount = 0;
    char    usrMenuOption     = 0,
            getFirstLetter          = 0,
            tmpArray[WORD_SZ] = {},
            *getWord = 0;
    string  str, 
            str2;
    ifstream  inFile, 
              inFile2;

    do 
    {
        cout << "Please make a selection: \n\
a) Read a text file\n\
b) Remove words starting with letter\n\
c) Print words to console\n\
d) Quit\n";
        cin  >> usrMenuOption;
        switch( usrMenuOption )
        {
            case'A':
            case'a':
                cout << "Enter a file name: ";
                cin.sync();
                cin  >> filePath;
                inFile.open( filePath );
                if ( !inFile ) return -1;
                inFile >> str; // prime the eof flag
                while ( !inFile.eof() )
                {        
                    inFile >> str;
                    wrdCount++;
                    g_wordArray = new Word *[wrdCount];
                }
                inFile.close();
                inFile2.open( filePath );

                while( !inFile2.eof() )
                {
                    inFile2 >> str2;
                    for ( unsigned x = 0; x < str2.length(); x++ )
                    g_wordArray[x] = new Word( str2.c_str() );
                }
                cout << wrdCount << " Words read from the file " << endl;
                inFile2.close();
                break;
            case'B':
            case'b':
                getFirstLetter = g_wordArray[wrdCount]->GetFirstLetterLower();
                //getWord = g_wordArray[wrdCount]->GetWord();
                cout << getWord << endl;
                break;
            case'C':
            case'c':
                break;
            case'D':
            case'd':
                cout << "Quit Requested. " << endl;
                break;
            default:
                cout << '"' << usrMenuOption << '"' << " Not Defined! " << endl;
        }
    } while (usrMenuOption != 'D' && usrMenuOption != 'd');

#ifdef _DEBUG
    _CrtDumpMemoryLeaks();
#endif
    cin.ignore();
    return 0;
}

void FreeWordArray()
{
    // free the memory that is allocated
    return;
}

Answer 1

EDIT: I've put this edit at the top since it directly answers your question of why Word is broken. Your copy constructor is wrong:

Word::Word( const Word* theObject ) 
{
    ptr_ = theObject->ptr_;
    len_ = theObject->len_;
}

this doesn't make a copy of what theObject->ptr_ points to, just the pointer. So you effectively have two Word objects pointing to the same internal string. This gets very bad when a Word object gets deleted. A proper implementation (using the techniques you did, I recomend against them) would be like this:

Word::Word( const Word* theObject ) 
{
    ptr_ = new char[theObject->len_ + 1 ];
    strcpy( ptr_, theObject->ptr_  );  
    len_ = theObject->len_;
}

EDIT: Earwicker noted the following as well:

... although that "copy constructor" is not a copy constructor. So the compiler-generated one will still exist, and does the same memberwise copy, and therefore the same problem still exists.

To remedy that, you'll need to make a proper copy constructor which should have the prototype:

Word::Word(const Word &theObject);

Also this code here:

while ( !inFile.eof() )
{        
    inFile >> str;
    wrdCount++;
    g_wordArray = new Word *[wrdCount];
}

leaks like sieve! You reallocate g_wordArray after every word is read and entirely forget to delete the previous one. Once again I'll show a sane implementation using the techniques you are attempting to use.

while (inFile >> str)
{        
    inFile >> str;
    wrdCount++;
}
g_wordArray = new Word *[wrdCount];

Notice how it counts the words, then allocates room once, after it knows how much to allocate. Now g_wordArray is ready to be used for up to wrdCount word objects.

ORIGINAL ANSWER:

why don't you just replace the Word class with std::string? It would make the code much smaller and easier to work with.

If it makes it easier for you, just do this:

typedef std::string Word;

then you can do like this:

Word word("hello");
char first_char = word[0];

plus it has the added bonus of removing the need for you to use the .c_str() member to get the c-style string.

EDIT:

I would also change your g_wordArray to just be a std::vector<Word>. That way you can simple do this:

g_wordArray.push_back(Word(str));

no more dynamic allocation! it's all done for you. Size of the word array will be limited only by the amount of RAM you have, because std::vector grows as needed when you use push_back().

Also, if you do this...guess what to get the word count you simply do this:

g_wordArray.size();

no need to manually keep track of the number of them!

EDIT:

Also, this code is broken:

while( !inFile2.eof() )
{
    inFile2 >> str2;
    ...
}

because eof isn't set till after you attempt a read, you are better off using this pattern:

while(inFile2 >> str2)
{
    ...
}

which will correctly stop on EOF.

Bottom line, if you do this right, there should be very little actual code you need to write.

EDIT:

Here's a sample straight forward implementation of what I think you want. From the menu items it seems that the intention is for the user to choose option 'a' first, then 'b' zero or more times to filter out some words, then finally c to print the results (one word per line). Also, option 'D' isn't really needed since hitting Ctrl+D sends an EOF to the program and makes the "while(std::cin >> option)" test fail. Thus ending the program. (At least in my OS, it might be Ctrl+Z` for windows).

Also it makes no effort (neither did yours) to deal with punctuation, but here it is:

#include <string>
#include <vector>
#include <fstream>
#include <iostream>
#include <algorithm>
#include <functional>
#include <iterator>

struct starts_with : public std::binary_function<std::string, char, bool> {
    bool operator()(const std::string &s, char ch) const {
     return s[0] == ch;
    }
};

void print_prompt() {
    std::cout << "Please make a selection: \na) Read a text file\nb) Remove words starting with letter\nc) Print words to console" << std::endl;
}

int main( const int argc, const char **argv) {
    std::vector<std::string> file_words;
    char option;
    print_prompt();
    while(std::cin >> option) {
     switch(option) {
     case 'a':
     case 'A':
      std::cout << "Enter a file name: ";
      // scope so we can have locals declared
      {
       std::string filename;
       std::string word;
       std::cin >> filename;
       int word_count = 0;
       std::ifstream file(filename.c_str());
       while(file >> word) {
        file_words.push_back(word);
       }
       std::cout << file_words.size() << " Words read from the file " << std::endl;
      }
      break;
     case 'b':
     case 'B':
      // scope so we can have locals declared
      {
       std::cout << "Enter letter to filter: ";
       char letter;
       std::cin >> letter;

       // remove all words starting with a certain char
       file_words.erase(std::remove_if(file_words.begin(), file_words.end(), std::bind2nd(starts_with(), letter)), file_words.end());
      }
      break;   

     case 'c':
     case 'C':
      // output each word to std::cout separated by newlines
      std::copy(file_words.begin(), file_words.end(), std::ostream_iterator<std::string>(std::cout, "\n"));
      break;
     }
     print_prompt();
    }
    return 0;
}

Answer 2

Is g_wordArray[wrdCount] a valid Word object? My C++ is rusty, but it looks to me like this is beyond the end of the array. Also, you appear to be repeatedly trampling over the contents of g_wordArray in the inFile2 loop: for each string you read into str2, you're going to reinitialise the first str2.length() entries in g_wordArray. That would explain the apparent clobbering of the member variables. Finally, I'm not sure about this, but is the pointer returned by str2.c_str() still valid after you've read a new value into str2? So your GetWord() loop may be getting a garbage value from strlen and trundling off into never never land.

Answer 3

Without slogging through the entirety of your main.cpp, I did notice a problem here in your constructor/destructor set:

Word::Word( const Word* theObject ) 
{
    ptr_ = theObject->ptr_;
    len_ = theObject->len_;
}

Word::~Word()
{
    delete [] ptr_;
    ptr_ = NULL;
}

Notice that in your copy constructor, you simply assign your internal pointers to match the object you're copying from. But in your destructor, you delete data at your internal pointers.

Here's where that can get ulgy:

Word w1 = Word("hello");
Word w2 = Word(w1);
delete w2;

Now, what does your first word contain? The pointer will exist, but the data it references was deleted in w2's destructor.

They call it a "copy constructor" because that's what you're supposed to do: copy the thing.