2
\$\begingroup\$

A week ago I posted this question and after reading the answers and accepting one I decided to refactor my code quite heavily. I would like to get some feedback on this newer version.

The changes

  1. No more predicates! I'd like it to look exactly like Python's input() function.
  2. Always return a string and let the user parse it as needed (just like Python).
  3. Keeping 2 of the custom exceptions I made because I want the user to be able to handle them specifically. I know std::ios_base::failure exists but I want to separate it into different exception types.

The design choices I stuck with

  1. I'd like to keep input() as a function, so I'm not making it a class on purpose.
  2. I know it is tightly coupled to std::cin and no other streams and it's on purpose. I'd like any new user to be able to just copy paste this input function and not worry about validation when reading input from the user.
  3. I would like to keep this C++14 for now.

Validation rationale for std::cin

  1. cppref - Look at the code example
  2. learncpp - near the end of the lesson

Last thoughts

If I were to make this a class instead of a function I would just try to implement a Scanner (Java-like) that takes std::istream& which would make it usable with any input stream. But for now I want to keep it std::cin only because it will be shorter and simpler.

The code

#include <stdexcept>
#include <iostream>
#include <string>
#include <limits>

/* Custom exceptions -------------------------------------------------------- */
class StreamFailure final : public std::runtime_error
{
public:
    StreamFailure()
        : std::runtime_error{ "Stream failed (non-recoverable state)" }
    {}
};

class ExtractionFailure final : public std::runtime_error
{
public:
    ExtractionFailure()
        : std::runtime_error{ "Failed to extract value from stream" }
    {}
};

/* Simplified input --------------------------------------------------------- */
std::string input(const std::string& prompt = "")
{
    if(!prompt.empty())
    {
        std::cout << prompt;
    }

    std::string temp{}; // <-- unavoidable if i want it to look like python
    std::getline(std::cin, temp);
    if(std::cin.eof() || std::cin.bad())
    {
        throw StreamFailure{}; // <--should rarely happen for any type extracted
    }
    else if(std::cin.fail())
    {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        throw ExtractionFailure{}; // <-- pretty hard to make this happen with
                                   //     std::string
    }

    return temp;
}

int main()
{
    while (true)
    {
        try
        {
            int i = std::stoi(input("Enter an integer: "));
            std::cout << i << '\n';
        }
        catch (std::invalid_argument const& e)
        {
            std::cerr << "invalid_argument\n";
        }
        catch (std::out_of_range const& e)
        {
            std::cerr << "out_of_range\n";
        }
        catch(const std::exception& e)
        {
            std::cerr << e.what() << '\n';
            break;
        }
    }

    return 0;
}
\$\endgroup\$
12
  • \$\begingroup\$ We should not throw StreamFailure for the empty stream. It is more like the extraction failure. \$\endgroup\$
    – panik
    Commented Oct 10, 2023 at 23:37
  • \$\begingroup\$ If you read the learncpp article I added you would see this quote: "On Unix systems, entering an end-of-file (EOF) character (via ctrl-D) closes the input stream. This is something that std::cin.clear() can’t fix, so std::cin never leaves failure mode, which causes all subsequent input operations to fail.". Based on that, its not a recoverable error like extraction failure and is indeed a StreamFailure. \$\endgroup\$ Commented Oct 11, 2023 at 8:02
  • \$\begingroup\$ Are those your actual comments? Or are those comments for reviewers? If it is the latter, please remove them, keep the code as-is. If those are actual comments, then I have some review notes for them. \$\endgroup\$
    – Zeta
    Commented Oct 11, 2023 at 18:19
  • 1
    \$\begingroup\$ The problem, as I have repeated no less than three times now, is that you are doing it wrong. You are throwing away valid input because you are not checking for “fail” before checking for “EOF”… which even the code you keep pointing to does. This means you are throwing an error when there is no error, and losing perfectly valid input. The cppreference code is also wrong. I can prove it. Just click “run this code”, then edit line 11 to remove '\n', then run. Look at the output. Note how it’s changed. The “2” is now lost. \$\endgroup\$
    – indi
    Commented Oct 13, 2023 at 20:58
  • 1
    \$\begingroup\$ To fix the cppreference code, just remove the else on line 19, then move lines 17 and 18 into the if block (and indent them correctly to silence the warning/error). Now it works; it always accepts good input, always rejects bad input, and never loses good input. The lesson is simple and clear: “always check for “fail” firstthen check for anything else (EOF or “bad”). If the fail bit was not set, then you have valid input, period, no matter what the EOF or bad bits are. \$\endgroup\$
    – indi
    Commented Oct 13, 2023 at 20:58

2 Answers 2

1
\$\begingroup\$

After reading @indi explain what problem I have with my code I've fixed it in accordance with the comments he left. If anyone sees any more problems with it you are welcome to point it out.

The new input function:

std::string input(const std::string& prompt = "")
{
    if(!prompt.empty())
    {
        std::cout << prompt;
    }

    std::string temp{};
    std::getline(std::cin, temp);
    if(!std::cin)
    {
        if(std::cin.eof() || std::cin.bad())
        {
            throw StreamFailure{};
        }
        else
        {
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            throw ExtractionFailure{};
        }
    }

    return temp;
}
\$\endgroup\$
0
\$\begingroup\$

How about using std::error_code instead of exception? From my experience using exception for business logic will mess up the code.

#include <system_error>
#include <iostream>

enum struct MyError { //create own error value
    //don't start with 0
    invalid_input = 10 ,
    out_of_range
};

namespace std
{
  template <>
    struct is_error_code_enum<MyError> : true_type {}; //overload template to accept our error value
}

struct MyCategory : std::error_category //create own category
{
  const char* name() const noexcept override{
    return "my_error";
  }
  std::string message(int value) const override{
    switch(static_cast<MyError>(value)){
        case MyError::invalid_input:
            return "invalid input";
        case MyError::out_of_range:
            return "out of range";
        default:
            return "unknown error";
    }
  }
};

std::error_category& get_my_category() // singleton
{
    static MyCategory cate{};
    return cate;
}

std::error_code make_error_code(MyError value) //this overload function make_error_code must have namespace same with error value
{
  return {static_cast<int>(value), get_my_category()};
}
///
std::error_code get_input(std::string& buffer, int dummy)
{
    if(dummy == 1){// frist validate
        return MyError::invalid_input; //implictly construct std::error_code from MyError::invalid_input. It potentially call make_error_code
    }

    if(dummy == 2){// second validate
        return MyError::out_of_range; 
    }

    //do valid thing

    return {}; //return empty error
}

int main()
{
   std::string buffer;
   auto ec = get_input(buffer, 2);
   if(ec //has error
    && ec != MyError::invalid_input // unhandle error
    ){
       std::cerr << "[unhandle error] " << ec.message()  << "\n";
       return 0;
   }
   // handle error or use buffer
}

This seems better when you have a new error. Just add the new one to error value enum and message category.

If you don't like passing output as arguments you can use std::pair to return both.

\$\endgroup\$
3
  • \$\begingroup\$ I doubt error codes would be better here. (1) there wont be any more new errors (2) With my code the exceptions should rarely even throw, the system should never really fail to read std::string unless you run out of memory or get a bad/eof stream which is highly unlikely meaning exceptions are exceptional (3) isocpp shows the problem with this approach. If I had to call the input function multiple lines in a row to read into different variables I would end up with an if/else if hell to make sure I handle all the errors \$\endgroup\$ Commented Oct 17, 2023 at 6:52
  • \$\begingroup\$ @globalturist std::error_code is associate with std::system_error. You can throw them with ` throw std::system_error{make_error_code(MyError::invalid_input)}`. \$\endgroup\$ Commented Oct 17, 2023 at 10:28
  • \$\begingroup\$ I know I can throw std::error_code, but please read the original question, I specified I leave in my custom exceptions on purpose. Also, Doesn't that beat your entire proposal of "How about using std::error_code instead of exception" ? \$\endgroup\$ Commented Oct 17, 2023 at 10:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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