Reading lines of a file into a vector of strings

Question

I have to read from a file and store the data inside an std::vector container. The procedure of opening the file, reading and writing onto the container are enclosed in a routine: readData.

Reading and writing include the use of several std functions that may fail. Instead of putting a try catch for each of those std function calls, I have put a try catch-all block to collect all the possible errors. Is this a bad practice?

#include "global.h

void readData( std::vector<std::vector<std::string> >&, 
                    const std::string& );

int main()
{

    std::vector<std::vector<std::string> >& table;
    std::string path = "myDataFile";

    try
    {
        readData( table, path );
        std::cout << "Done" << std::endl;
    }
    catch( const char* error)
    {
        std::cout << error << std::cout;
        return 0;
    }

    // do something with the data ...

    return 0;   
}

Here is the readData implementation:

void readData( std::vector<std::vector<std::string> >& table, 
                    const std::string& path)
{

    std::ifstream ifs(path);
    if (  !ifs )
        throw "Error opening file.";

    try
    {
        while ( true ){
            std::string line;
            std::getline(ifs, line);

            if(!ifs) 
                break;

            std::vector<std::string> row;
            std::istringstream iss(line);
            std::copy(std::istream_iterator<std::string>(iss),
                 std::istream_iterator<std::string>(),
                 std::back_inserter(row));
            table.push_back(row);
        }
    }
    catch(...)
    {
        throw "Error reading the content of the file";
    }

}

Answer 1

General Comment

Instead of putting a try catch for each of those std function calls, I have put a try catch-all block to collect all the possible errors. Is this a bad practice?

No that is completely fine. The general principle is that if you are not going to handle the error then you should not catch it. Logging is an exception to the rule; where you catch log and then re-throw.

BUT: Most C++ io functions do not throw exception on error (you can make them do so, but it is not normal). Instead a an error causes the flags on the stream to be set to indicate one of the failure states. Now the stream object itself can be used in a boolean context to check the state of these flags (stream will convert to true if it is in a good usable state and false if any of the error flags are set).

Code Review

Avoid output parameters

I hate return parameters. They are non logical and make the flow of the code hard to read. I prefer to return all the results (even if there is more than one).

void readData( std::vector<std::vector<std::string> >&, 
                    const std::string& );

Now you are probably thinking that this is inefficient as this requires a copy of the object out of the function. Which is technically true. But the compiler writers put a lot of work into making sure that this copy does not actually happen. RVO and NRVO (optimizations) basically make sure that the object is built in place at the destination (no copy required).

Also in C++11 we introduced move semantics to the language. So on the rare occasion that RVO and NRVO fail a return value will be moved (not copied) from a function. moving a vector simply involves swapping three pointers (or something very close to this) so it is not very expensive.

Prefer "\n" over std::endl

The difference is that std::endl flushes the stream.

        std::cout << "Done" << std::endl;

The stream already flush themselves at the most appropriate time to make themselves efficient; manually forcing a flush is more likely to cause inefficiencies.

Return a non zero value to indicate failure

The OS uses the return value of main to detect if the program ran correctly.

    catch( const char* error)
    {
        std::cout << error << std::cout;
        return 0;
    }

Return a non zero value to indicate failure.

Catch all exceptions in main.

If you throw an exception and it is not caught the program will exit. BUT it is implementation defined if this will force the stack to unwind. If you catch in main() this will force the stack to unwind all the way to the top (thus making sure that all the appropriate destructors are called).

Note: You should re-throw the exception potentially after logging. This is because most OS detect applications that exit via an exception and provide further facilities to the user when this occurs.

int main()
{
    try
    {
         // STUFF
    }
    catch(...)
    {
         std::cout << "Exception Reached main()\n";
         throw;
    }
}

Use an exception derived from a std::exception

When you throw an exception you should throw an exception derived from std::exception. If you don't throw a specialized exception your default should probably be std::runtime_error.

    if (  !ifs )
        // this throws an object of type `char const*`
        throw "Error opening file.";
    // Try this
    if (  !ifs )
    {
        throw std::runtime_error("Error opening file.");
    }

STandard pattern for reading a file.

Your code actually works correctly

        while ( true ){
            std::string line;
            std::getline(ifs, line);

            if(!ifs)    // Most people forget this check.
                break;

            // STUFF
        }

But can be made more succinct. If you do the read as part of the test you can do the read and test as a single line.

        // This is more the standard pattern for reading a file.
        std::string line;
        while ( std::getline(ifs, line) )
        {
            // STUFF
        }

Note: std::getline() returns a reference to the input stream. Which is used in the boolean context checks the state of the error flags. So the loop is only entered if the read succeedes.

Vector can be constructed with iterators.

            std::istringstream iss(line);
            std::vector<std::string> row;
            std::copy(std::istream_iterator<std::string>(iss),
                 std::istream_iterator<std::string>(),
                 std::back_inserter(row));

Very good. But this can be simplified to:

            std::istringstream iss(line);
            std::vector<std::string> row{std::istream_iterator<std::string>(iss),
                                         std::istream_iterator<std::string>()};

Don't generalize an exception

Here you are catching a specific exception that could contain lots of information and replacing it with a string.

    catch(...)
    {
        throw "Error reading the content of the file";
    }

This looses all the relevant information about the original exception and replaces it with a string. Loosing information is never good.

Answer to Comments

I would refactor like this:

#include <iostream>
#include <fstream>
#include <sstream>
#include <iterator>
#include <stdexcept>
#include <vector>
#include <string>    

using StringTable = std::vector<std::vector<std::string>>;

StringTable readData(std::string const& path)
{
    std::ifstream ifs(path);
    if (  !ifs )
    {
        throw std::runtime_error("Error opening file.");
    }

    StringTable table;
    std::string line;
    while (std::getline(ifs, line))
    {
        std::istringstream iss(line);
        table.emplace_back(std::istream_iterator<std::string>(iss),
                           std::istream_iterator<std::string>());
    }
    return table;    
}

void doMain()
{
    std::string path  = "myDataFile";    
    StringTable table = readData(path);
    std::cout << "Done" << "\n";

    // do something with the data ...
}

int main()
{    
    try
    {
         doMain();
    }
    catch(std::exception const& e)
    {
        std::cout << "Exception: " << e.what() << "\n";
        throw;
    }
    catch(...)
    {
        std::cout << "Exception: Unknown\n";
        throw;
    }
    return 0;   
}