Is this the proper way to open a file for input?

Question

Is this the proper way to open a file for input?

void BinaryTree::read(char * path, int line_number)
{
    ifstream * file(path); //error: cannot convert ‘char*’ to ‘std::ifstream*’ in initialization
    file->seekg(0, std::ios::beg);
    int length = file.tellg();
    char * buffer = new char[length];
    file->getline(buffer, line_number);
    printf("%d", length);
    file->close();

}

I'm guessing not, because the compiler won't accept a char array, or a std::string for the ifstream constructor, yet when I read documentation, I see strings and/or char arrays being passed to ifstream constructors.

Is something wrong with my compiler or am I just using the wrong type in my parameter?

Answer 1

Couple of things I would change:

void BinaryTree::read(char * path, int line_number)
{
    // Use an object not a pointer.
    ifstream*        file(path);

    // When you open it by default it is at the beginning.
    // So we can remove it.
    file->seekg(0, std::ios::beg);

    // Doing manually memory line management.
    // Is going to make things harder. Use the std::string
    int length = file.tellg();
    char * buffer = new char[length];
    file.getline(buffer, line_number);

    // Printing the line use the C++ streams.
    printf("%d", length);

    // DO NOT manually close() the file.
    // When the object goes out of scope it will be closed automatically
    // http://codereview.stackexchange.com/q/540/507
    file->close();

}  // file closed here automatically by the iostream::close()

Simplified here:

void BinaryTree::read(char * path, int line_number)
{
    ifstream        file(path);

    std::string   line;
    std::getline(file, line);

    std::cout << line.size() << "\n";
} 

Answer 2

You've got more problems than the ones mentioned above though:

• you're using file variously as a pointer and an object (file->seekg(...) and file.tellg())
• you're taking the position with the tellg after having moved to the start of the file with the seekg and then using that position to give you the size of the buffer. This will give you an empty buffer (at best a pointer to zero bytes, I think).
• you're then calling getline with the line_number parameter passed as an argument. This will cause getline to read at most line_number bytes but you've only allocated length bytes (which is zero as we've seen above). I guess that you want to read the line_number'th line but that takes a bit more work - you've gotta count up line_number newlines until you reach the right line.
• more generally do you want to open and close the file each time you get the next line - you might want to rethink your interface as a whole.

apologies if I've missed the point here

Answer 3

No pointer needed, as @Nawaz said:

ifstream *file(path);

---

Potential memory leak:

char *buffer = new char[length];

You should delete[] it afterwards:

delete[] buffer;

...But, it's a lot easier to use std::string:

std::string buffer;

---

Final code:

std::string BinaryTree::read(std::string path, int line_number)
{
    std::string buf;
    ifstream file(path.c_str());

    if(file.is_open())
    {
        // file.seekg(0, std::ios::beg); // @Tux-D says it's unnecessary.
        file.getline(buf, line_number);
        file.close();
    }

    return buf;
}

Answer 4

ifstream * file(path); //error: cannot convert ‘char*’ to ‘std::ifstream*’ in initialization

The problem is that the construction of the object is not appropriate. You're probably trying to do the following (or something similar), indeed passing a char array to the constructor of the ifstream object:

ifstream file(path);

However, the introduction of an asterisk here changes the whole meaning. You're creating a pointer to an object ifstream, but not the object ifstream itself. And in order to construct a pointer, you would need another pointer to an ifstream object (i.e. a pointer of the same type).

ifstream file(path);
ifstream * ptr( &path );

This is not what you intended to do, anyway, you probably wanted to create an ifstream object referenced by a pointer:

ifstream * file = new ifstream( path );
//... more things...
file->close();

But remember that the object must free'd when it is not needed anymore. Objects referenced by pointers are not automatically free'd as it happens with normal (objects in the stack) objects.

ifstream * file = new ifstream( path );
//... more things...
file->close();
delete file;

Hope this helps.

Answer 5

Dont use pointer. It is not needed here.

Try this:

ifstream file(path);

and then use it as:

//...
file.getline(buffer, line_number);//file->getline(buffer, line_number);
//...