3
\$\begingroup\$

The following program reads a stream and creates a two dimensional std::vector.

I'm trying to find an elegant way to handle the errors and recover from them as much as possible while still reporting the problems encountered.

It feels as though my nice clean functions are getting more and more cluttered as I add the error handling.

Is this the best way?

#include <vector>
#include <iostream>
#include <sstream>
#include <string>
#include <iomanip>
#include <stdexcept>

namespace {

    using Grid_row = std::vector<int>;
    using Grid = std::vector<Grid_row>;

    class Partial_value : public std::runtime_error {
    public:
        Partial_value(std::string text, int val) : std::runtime_error{text}, val{val} {}
        int val{};
    };

    int convert(const std::string& text)
    {
        int val{};
        try {
            val = std::stoi(text);
        }
        catch(const std::exception& e) {
            throw Partial_value{"Due to error: \"" + std::string{e.what()} +"\"; Could not convert " + text + " to integer. Value was converted to 0.", val};
        }
        std::ostringstream oss{};
        oss << val;
        if(oss.str() != text)
            throw Partial_value{"Could not fully convert " + text + " to integer. Value was converted to " + oss.str() + ".", val};
        return val;
    }

    std::istream& operator >> (std::istream& is, Grid_row& grid_row)
    {
        std::string term{};
        while(is >> term) {
            int val{};
            try {
                val = convert(term);
            }
            catch(const Partial_value& e) {
                val = e.val;
                std::cerr << e.what() << '\n';
            }
            grid_row.push_back(val);
        }
        return is;
    }

    std::istream& operator >> (std::istream& is, Grid& grid)
    {
        std::string line{};
        while(std::getline(is, line)) {
            std::istringstream is_line{line};
            Grid_row grid_row{};
            is_line >> grid_row;
            grid.push_back(grid_row);
        }
        return is;
    }

    std::ostream& operator << (std::ostream& os, const Grid& grid)
    {
        for(const auto& i : grid) {
            for(const auto& j : i) {
                os << std::setw(5) << j;
            }
            os << '\n';
        }
        return os;
    }
}

auto input_text{
R"(3 -1 -3 -1 -4
40 7.5 3 1 1
10 -2 -Z 1 1
10 0 1 0 -1
)"};

int main() {
    std::istringstream is{input_text};
    Grid grid{};
    is >> grid;
    std::cout << grid;
    return 0;
}

Produces:

Could not fully convert 7.5 to integer. Value was converted to 7.
Due to error: "invalid stoi argument"; Could not convert -Z to integer. Value was converted to 0.
    3   -1   -3   -1   -4
   40    7    3    1    1
   10   -2    0    1    1
   10    0    1    0   -1
\$\endgroup\$
11
\$\begingroup\$

Your convert function should be pure.

convert should be doing one thing and one thing only, and that is to convert the given string to int. All those checks you do are not useful because they can easily be performed in the function that calls convert.

int convert(const std::string& text) {
    return std::stoi(text);
}

Your convert function is not needed.

Seeing as after changing it to a pure function, it is now acting as a wrapper to another function call, you don't need it anymore, and it can now be replaced by the function it calls, std::stoi.

Move convert logic elsewhere

Move all the logic from within the convert method to your overloaded function, std::istream& operator >> (std::istream& is, Grid_row& grid_row).

std::istream& operator >> (std::istream& is, Grid_row& grid_row) {
    std::size_t fin;
    int val;

    for (std::string term; is >> term; ) {
        try {
            val = std::stoi(term, &fin);
            if (fin != term.size()) {
                std::cerr << "Could not fully convert " + text + " to integer. Value was converted to " + text.substr(0, fin) + "." << '\n';
            }
        } catch(const std::exception& e) {
            std::cerr << "Due to error: \"" + std::string{e.what()} +"\"; Could not convert " + text + " to integer. Value was converted to 0." << '\n';
        }

        grid_row.push_back(val);

    }
    return is;
}

Now all you have is one try-catch, which is really all that is needed

\$\endgroup\$
  • 2
    \$\begingroup\$ Thanks for the good answer. I also like the for (std::string term; is >> term; ) {. \$\endgroup\$ – wally Dec 12 '16 at 16:56
  • \$\begingroup\$ @flatmouse, yvm \$\endgroup\$ – smac89 Dec 12 '16 at 17:14
  • \$\begingroup\$ Could you explain that for loop a bit further? I have been away from c/c++ for a while. Thanks. \$\endgroup\$ – element11 Dec 12 '16 at 20:43
  • \$\begingroup\$ @element11, The for loop initializes a string called term, then reads input from the stream into the string until the stream is finished. When the condition for the loop fails, the loop is finished. If you are confused about the syntax I used, it's not very common but still valid. Remember that a for-loop has this syntax for ( declaration-or-expression(optional) ; declaration-or-expression(optional) ; expression(optional) ). Read more about it here \$\endgroup\$ – smac89 Dec 12 '16 at 22:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.