Is it bad practice to have an API use std::string returned as error code?

Question

When delivering an API to you often have to declare error codes (most often as int) and later you often want to provide a function converting a int error code to std::string to be able to report the error to the user in a smart way.

I found some posts on how to maintain the int/std::string mapping programmatically, like this one: Mapping error codes to string in C++

Now, I was wondering, why not simply return a std::string rather than a int? Empty string would mean no error, anything else would mean error + provide a human readable message.

Let's obviously assume you don't care about memory usage and performances (your API has functions not called that often and time of execution is not critical).

If you need the client to be able to do some specific operations programmatically, error codes could be declared as constants. But you don't need any int to std::string mapping anymore. As an example, it would be:

Declaration:

static const std::string successMessage;
static const std::string fileDoesNotExistMessage;
static const std::string internalErrorMessage;

std::string openFile( const std::string& fileName );

Implementation:

static const std::string successMessage = "";
static const std::string fileDoesNotExistMessage = "File does not exist";
static const std::string internalErrorMessage = "Internal error";

std::string openFile( const std::string& fileName )
{
    if ( ... ) // test file existance
    {
        if ( ... ) // internal tests
            return internalErrorMessage;
        else
            return successMessage;
    }
    else
    {
        return fileDoesNotExistMessage ;
    }
}

API user can then do:

int main()
{
    std::string error = openFile( "file.txt" );
    if ( error.empty() )
    {
        std::cout << "File was successfully opened" << std::endl;
    }
    else if ( error == fileDoesNotExistMessage )
    {
        // specific error handling
    }
    else
    {
        std::cout << "Unable to open file, error reported is " << error << std::endl;
    }
}

Advantages:

• No risk to have a int/std::string broken mapping
• Easy to add error codes on the fly
• Easy to maintain

There must be disavantages, because I know no open source library using this approach...any idea why?

Answer 1

Why not go one stage further and do the Right Thing(tm)?

Report errors with exceptions. It's what they're for.

example:

struct file_does_not_exist : std::runtime_error
{
  using std::runtime_error::runtime_error;
};

struct internal_error : std::runtime_error
{
  using std::runtime_error::runtime_error;
};

void openFile( const std::string& fileName )
{
    if (!fileExists())
      throw file_does_not_exist(fileName + " does not exist");

    if (!internal_tests(fileName))
      throw internal_error("internal tests failed");

    doLogic();
}

user code now becomes:

   try {
     openFile("xyz.txt");
     do_other_logic();
     ...
   }
   catch(const std::exception& e)
   {
      std::cerr << "failed because: " << e.what() << std::endl;
   }

A complete example that demonstrates why exceptions are the Right Thing

#include <iostream>
#include <stdexcept>
#include <exception>
#include <string>
#include <fstream>

// define our program's exceptions, deriving from standard types
struct failed_to_open : std::runtime_error
{
    using std::runtime_error::runtime_error;
};

struct database_op_failed : std::runtime_error
{
    using std::runtime_error::runtime_error;
};

struct user_update_failed : std::runtime_error
{
    using std::runtime_error::runtime_error;
};

// copied from cppreference.com
void print_exception(const std::exception& e, int level =  0)
{
    std::cerr << std::string(level, ' ') << "exception: " << e.what() << '\n';
    try {
        std::rethrow_if_nested(e);
    } catch(const std::exception& e) {
        print_exception(e, level+1);
    } catch(...) {}
}


void open_file(std::fstream& f, const std::string& filename)
{
    throw failed_to_open(std::string("failed to open file " + filename));
}


struct database_connection
{
    database_connection()
    try {
        open_file(_file, "database.txt");
    }
    catch(...) {
        std::throw_with_nested(database_op_failed("database failed to create"));
    }


    std::fstream _file;
};

// note the use of function try blocks coupled with
// throw_with_nested to make exception handling *clean and easy*
void update_user_age(const std::string& name, int newage)
try {
    database_connection d;
    // d.update_record(...)
    // ... RAII
}
catch(...) {
    std::throw_with_nested(user_update_failed("failed to update user " + name
                                                + " to age " + std::to_string(newage)));
}


// only one try/catch in the whole program...
int main()
{
    try {
        update_user_age("bob", 30);

        // ... lots more log here with no need to check errors

    }
    catch(const std::exception& e) {
        // ...which completely explains the error
        print_exception(e);
        return(100);
    }
    return 0;
}

expected output:

exception: failed to update user bob to age 30
 exception: database failed to create
  exception: failed to open file database.txt

Answer 2

This could be useful to a user, but not to a dev.

Given any error, I'd like to easily find the spot responsible for it. Can you guarantee the message will remain unchanged, so a future search will point to the right spot? It's much easier to get that with enums - the message can be changed with no real harm.

You should not underestimate the performance issue. Comparison, copying, allocation - it costs. If you really, really don't care, simply create a struct with both error code and string message. Or throw an exception.