Class invalid flag vs exception

Question

So i have the following. I read in lines from a text file. Each line needs to be analysed and get the important data extracted (I wont go in the details here).

I worte a Parser class which does this job. Unforunately it can happen quite frequently, that the line data is not usable. I only find this out when read out the line in the class and analyse it. So I made a valid bit which indicates if the data is ok. At the moment I check the valid bit before grabbing the data. I was wondering if it is a better practice here to throw an exception. On the other hand i heared only throw exceptions when the case doesnt occur frequently. In my case like half the lines I analyse with the Parser class are not valid data.

Here is some simplified incomplete code how it is done now to get the idea:

struct Symbol_data {        // possible data which can be extracted out of the symbolik
    //Struct with the extracted data...
};

class Parse_symbol_line {   //class extracts data from a line. it is possible the data is unsusable
public:
    Parse_symbol_line(std::string& l);
    bool is_valid() const { return valid; }         //during processing the data can become unusable
    Symbol_data get_symbolic_data() { return sym_data; }
private:
    //Private Methods here which extract and analyse the line in many steps
    void read_data_from_line();
    void analyze_raw_comment();
    void mask_out_device_id();
    void analyze_device_id();
    void mask_out_vessel();
    void create_symbol_data();

    bool valid;             // if in valid dont use the data
    Symbol_data sym_data;
};

//constructor calls all the steps to read out the data out of the line
//it is possible after each step, that the line is not valid and therefore doesnt need more checks
Parse_symbol_line::Parse_symbol_line(std::string& l)
    :valid{ true }, EA{ true }, line{ l }, sym_data{ Symbol_data{} }
{
    read_raw_data_from_line();
    if (!valid) return;         //dont continue if data is invalid anyway
    analyze_raw_comment();
    if (!valid) return;
    mask_out_device_id();
    if (!valid) return;
    analyze_device_id();
    if (!valid) return;
    mask_out_vessel();
    if (!valid) return;
    create_symbol_data();
    if (!valid) return;
}

//another class provides the lines to the parse class and gets the data from it
void Compute_Symbols::read_symbolic()       
{
    std::ifstream ifs{ in_sym_fname };
    if (!ifs) throw std::runtime_error("void Compute_Symbols::read_symbolic(); symbolic file could not be opened\n");

    for (std::string line; std::getline(ifs, line);) {  
        Parse_symbol_line psl{ line };

        if (psl.is_valid()) {
            //continue work with the data   
        }
        else if (!psl.is_valid()) {
            //dont use the data
        }
    }
}

Answer 1

Exceptions are a way of signaling that your code cannot complete the requested operation. Perhaps something was null that you never expected to be null. Maybe preconditions were violated. Possibly other assumptions or constraints were untrue. An exception is a way to let something else know that the operation couldn't complete and to let code with more context / more ability handle that in some way.

So a lot of this hinges on two things, first, what the contracts and assumptions of your code are and two, how the code calling your function / object could possibly handle an error.

So what is the contract for your code? Is the idea that "I will parse this data and return some extracted data. You must feed me valid data." If that is the case, then an exception is appropriate. The code cannot fulfill it's contract when the data is invalid (precondition violation).

If your contract is "Hand me some data, I will try and parse it. If successful, I will return the extracted data. If not, I will return something indicating failure." Then an option data type would be an appropriate return type and an exception would be the wrong way to go.

This is much like the difference between int.Parse and int.TryParse in C#. The former expects valid data and will give you an integer or blow up. The latter will attempt the parsing and will return a boolean flag saying if the data could be parsed and the parsed value (if successful).

The other thing to consider is how any calling code could/would possibly handle an exception. If your calling code is just going to swallow the exception and ignore the bad data, it is likely something that would be an inappropriate use of an exception (though there are times where this is ok, they are just few and far between). Especially if this is a common occurrence, I would not use an exception. If the calling code could handle the exception in a meaningful way, then it is a possibility. If the exception is just used for control flow, it's bad. That's what option types / boolean flags are for.

Answer 2

You already use an established pattern of this in your code:

std::ifstream ifs{ in_sym_fname };
if (!ifs) throw std::runtime_error("void Compute_Symbols::read_symbolic(); symbolic file could not be opened\n");

If you follow that pattern, your code would look like this:

void Compute_Symbols::read_symbolic()       
{
    std::ifstream ifs{ in_sym_fname };
    if (!ifs) throw std::runtime_error("void Compute_Symbols::read_symbolic(); symbolic file could not be opened\n");

    for (std::string line; std::getline(ifs, line);) {  
        Parse_symbol_line psl{ line };

        if (!psl) {
            // do whatever error handling neccessary
        }

        // process data here
    }
}

You probably want to implement

operator void*() const; 

and/or

explicit operator bool() const;

for this to work.

You can certainly argue whether this is a good pattern. Other languages and frameworks use different patterns. I would argue that staying in the patterns and behaviors that are already used and staying consistent in your style all over your project is more important that being perfectly correct in one place, but breaking the rules and therefor expectations of future maintainers. You can cope with any pattern, as long as it's consistent.

Answer 3

At least right off, it looks to me like you'd be better off having the functions return a bool containing the value you're now putting into valid:

bool read_data_from_line();
bool analyze_raw_comment();
bool mask_out_device_id();
bool analyze_device_id();
bool mask_out_vessel();
bool create_symbol_data();

Then I'd probably create a vector (or array) of pointers to member functions so I could call those in a loop:

typedef bool (Parse_symbol_line::*mem_ptr)();

// ...

Parse_symbol_line(std::string& l)
{
    std::vector<mem_ptr> analyzers { 
        &Parse_symbol_line::read_data_from_line, 
        &Parse_symbol_line::analyze_raw_comment,
        &Parse_symbol_line::mask_out_device_id,
        &Parse_symbol_line::analyze_device_id,
        &Parse_symbol_line::mask_out_vessel,
        &Parse_symbol_line::create_symbol_data
    };

    for (mem_ptr a : analyzers)
        if (!(this->*a)())
            handle_error();
}

For those who find pointers to members ugly or problematic, you could just use something like:

if (!(read_data_from_line() 
   && analyze_raw_comment() 
   && mask_out_device_id()
   && analyze_device_id()
   && mask_out_vessel()
   && create_symbol_data()))
{
    handle_error();
}

They work out pretty much the same in the end, but with more than about 3, I'd generally prefer a loop.

Now we come to the real question: what do we do in handle_error()? The answer to this depends on the answer to another question. If failure of one of those functions means we haven't really created a valid Parse_symbol_line object, then the only correct thing to do is throw an exception. This is a constructor, and there's only one correct way for a constructor to signal that it was unable to successfully create an object, and that's to throw an exception. This is really independent of how often that might occur. There's no other way for a ctor to signal failure, so it has to be an exception.

As a general rule, I'd say a class containing a member named valid should be an alarm bell that the class probably has a problem. An object should always be valid. A ctor has a fairly simple requirement: create a valid object. If it can't do that, it must throw an exception so the object is never created at all. I don't like to get dogmatic about things, but in this case there's really no decent alternative. Having a valid member function that you constantly check to see if the class is in a valid state is not a decent alternative. I't much better to decide that either the object is valid, or it doesn't exist at all. Creating objects that aren't valid is a recipe for disaster. If you can't make a valid object, it's better to just get it over with immediately, not prolong the agony.

A possibility to consider rather than (at least directly) using exception handling would be to have a function that returns something like an std::optional<symbol_data>:

std::optional<Symbol_data> parse(std::istream &input) { 
    // ...
}

Then your read_symbolic would look something like this:

void Compute_Symbols::read_symbolic()       
{
    std::ifstream ifs{ in_sym_fname };
    if (!ifs) throw std::runtime_error("symbolic file could not be opened\n");

    for (std::string line; std::getline(ifs, line);) {
        if (auto foo = parse(line))
            // use foo
        else
            // ignore line
    }
}