Reading file with istream

Question

I am working on a project which involves polymorphism and inheritance.

lets assume that the hierarchy of the classes used in the project are:

Media ----> Book ----> MediaRegistry 

and the declaration of each class are as follows: (the classes have been narrowed down to the members which I have problem)

class Media
{
public:
    Media();
    Media(int id, string content);  
    virtual void input(istream& in) = 0;
    friend istream& operator>>(istream& in, Media& media);

protected:                          
    string _mediaTitle;
    int _id;
};

with:

istream& operator>>(istream& in, Media& media) 
{
    media.input(in);
    return in;
}

and:

class Book:public Media     // Inherits from abstract base class Media
{
public:
    Book();
    Book(int id, string title, int nrOfPages); // Constructor 
    void input(istream& in);

private:
    int _nrOfPages;
};

with:

void Book::input(istream& in)
{
    in >> _id >> _mediaTitle >> _nrOfPages;
}

and:

class MediaRegistry
{
public:
    MediaRegistry();
    int addMedia(Media*);       
    int loadMedia();

private:
    static const int MAX = 10;
    //Media* _pMedias[MAX];     // Vector for MAX numbers of Media pointers
    std::vector<Media*> _pMedias;
    int _nrOfMedias;                // _nrOfMedias == MAX when registry is full
    ifstream inFile;
    ofstream outFile;
    string path = ".\\data.txt";
};

with:

int MediaRegistry::loadMedia()
{
    inFile.open(path.c_str());
    while (!inFile.eof())
    {
        int mediaType = 0;
        inFile >> mediaType;
        if (inFile.eof())
            break;
        Media* media = nullptr;
        media = new Book();
        inFile >> *media;
        addMedia(media);
     }
    inFile.close();
    return 0;
}

Now in the data.txt, as title of the media, if I have space between the words (i.e, "The Clown" in stead of e.g "the-Clown"), the program encounters a problem in this member function istream& operator>>(istream& in, Media& media). I cannot understand, however, I debugged it many times to track the problem. In fact, I want my program could be able to get a string with space in it, but it doesn't do that now??

Answer 1

The >> input operator on strings will read exactly one word (if possible) or read nothing at all. So when you have the code:

void Book::input(istream& in)
{
    in >> _id >> _mediaTitle >> _nrOfPages;
}

It expects exactly one integer, then one word and then one integer again. If you then try to give it the following input:

1234
The Clown
216

It will first read the integer 1234, then the word The and then it will try to interpret Clown as an integer (which will fail). Notice how the last 216 is left unread. The next input operation on the stream will start here, which may lead to unexpected results.

If you have your file set up with a clear line structure (e.g., each "field" on a separate line), something like std::getline(in, line) would probably work better:

#include <iostream>     // std::istream
#include <string>       // std::getline
#include <limits>       // std::numeric_limits
using namespace std;

void Book::input(istream& in)
{
    in >> _id;
    // this ignores the rest of the line after the previously read integer
    //  (otherwise getline will just read the rest of the line)
    in.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

    getline(in, _mediaTitle);

    in >> _nrOfPages;
}

Answer 2

In addition to @Frxstrem's answer your loadMedia() function could use a touchup. There are a few things that it does wrong or poorly:

1) Opens the file stream when it can be done in the member initializer-list of the constructor.

loadMedia() should be for reading the data from the input file to the program, not for performing opening and close operations. That can be done using RAII, or the concept of initializing in the constructor and releasing in the destructor. Since the file name is already known, we can just initialize it through the member initializer-list.

MediaRegisty::MediaRegistry() : inFile{path} { }

Because your're using C++11 (as evidenced by your use of nullptr), we can use std::ifstream's constructor that takes an std::string as an argument. I also used the uniform initialization syntax (T{}). Some compilers don't support that syntax so if you get an error just replace them with parentheses.

2) Performs extractions without reliably checking for failure.

!eof() was never meant to be used as a condition for input. The input should be done before control passes through the loop in order to minimize the possibility of using the result of a failed extraction (and thus avoid Undefined Behavior). Input streams have a boolean operator that check for stream validity. If you use it the right way it will be invoked after the extraction:

for (auto p = std::make_pair(int{}, make_unique<Book>());
     inFile >> p.first >> *p.second; addMedia(media));

I used a std::pair to contain the two variables and a for loop to put the statements that we want to execute. make_unique<Book>() creates a std::unique_ptr<Book>. int{} is the same as 0.

3) Allocates data dynamically without releasing it, effectively causing a memory leak.

Every instance of new/new[] needs a corresponding delete/delete[]. If you don't deallocate the resource you get a memory leak.

If you ever come into a situation where you need to use dynamic allocation, please consider using memory management containers like std::unique_ptr and std::shared_ptr. These classes leverage the use of RAII by containing the resource and deallocating it in their destructors (when the objects go out of scope).

4) Closing the file unnecessarily.

File streams close their file handles when they go out of scope by calling close() inside their destructors. Call close() only when you need to reopen with a different file.

I hoped this helped. These are the changes I've made:

template<class T, class... Args>
std::unique_ptr<T> make_unique(Args&&... args) noexcept
{
    return new T{std::forward<Args>(args)...};
}

class MediaRegistry
{
public:
    MediaRegistry();
    int addMedia(std::unique_ptr<Media>);       
    int loadMedia();

private:
    std::vector<std::unique_ptr<Media>> _pMedias;
    std::ifstream inFile;
    ...
};

MediaRegistry::MediaRegistry() : inFile{path} { }

int MediaRegistry::addMedia(std::unique_ptr<Media> p)
{
    _pMedias.push_back(std::move(p));
    return 0; // not clear what's supposed to be returned here so I put 0
}

int MediaRegistry::loadMedia()
{
    for (auto p = std::make_pair(int{}, make_unique<Book>());
         inFile >> p.first >> *p.second; addMedia(p.second));
    return 0;
}