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;
}
include <stdexcept>. \$\endgroup\$