Exceptions in constructor..

Suppose I have a list class and two of its constructors are like this:

Code:

list(const std::string& s);
list(const std::string& s, bool& b);

The first one throws an exception if the string is not suitable for making a list.
The second one does not throw an exception and just sets the passed boolean variable to false (after doing some cleanup).
I want to use the second one to determine if a string can be made into a list.

Is this a good design wrt to constructors and exceptions ?

O_o

Nope.

It invites a scenario in which the programmer doesn't know if a reference to the object he has references a valid object or an object that on construction "returned false" even though such a notion is impossible.

You have multiple options; many of them presenting a better design. (The ones below are certainly better options.)

You can simply rearrange the notion of construction for your object with "two stage" construction. Though I despise this method, it has a place in the real world. Instead of the constructor "returning false", you setup the constructor to initialize the object to a "ready but invalid" state, provide a second function that does the real work of constructor (such as `bool construct(string &)'), and a third function to query the validity of the object.

You completely create your object normally and doing nothing else or completely fail to create the object normally by throwing an exception, and you rearrange the nature of "determine if a string can be made into a list" by moving the implementation of that question into a function. You only need to create a function (such as `list listfromstring(string &, bool &)') which can catch the exception and take other appropriate steps.

Soma

Originally Posted by phantomotap

You can simply rearrange the notion of construction for your object with "two stage" construction. Though I despise this method, it has a place in the real world. Instead of the constructor "returning false", you setup the constructor to initialize the object to a "ready but invalid" state, provide a second function that does the real work of constructor (such as `bool construct(string &)'), and a third function to query the validity of the object.

You completely create your object normally and doing nothing else or completely fail to create the object normally by throwing an exception, and you rearrange the nature of "determine if a string can be made into a list" by moving the implementation of that question into a function. You only need to create a function (such as `list listfromstring(string &, bool &)') which can catch the exception and take other appropriate steps.

Soma

I already did the second one.... but everything seems identical to doing it in the constructor if the 'non-throwing' constructor makes an empty list (which I'm never going to use..btw)
Doesn't that solve the 'broken object' problem you mentioned ?

I disagree on the "two-stage" construction approach.

I suggest only having the constructor that throws an exception if the string is invalid.

If you want to provide an ability to check if a string is valid, then provide a separate non-throwing function (named something pointed like IsValid()) to perform that check. That can be a normal non-member function, or it might be a static member of the list class.

Then you have two implementation choices..... either the constructor uses the IsValid() function in deciding to throw, or the IsValid() function attempts to construct an object and, if the constructor throws, swallows the exception and returns false.

I would also generally suggest a different name for your list class, given the potential confusion (in minds of people using your classes) with std::list.

Originally Posted by grumpy

I disagree on the "two-stage" construction approach.

I suggest only having the constructor that throws an exception if the string is invalid.

If you want to provide an ability to check if a string is valid, then provide a separate non-throwing function (named something pointed like IsValid()) to perform that check. That can be a normal non-member function, or it might be a static member of the list class.

Then you have two implementation choices..... either the constructor uses the IsValid() function in deciding to throw, or the IsValid() function attempts to construct an object and, if the constructor throws, swallows the exception and returns false.

I would also generally suggest a different name for your list class, given the potential confusion (in minds of people using your classes) with std::list.

Yes.. I already did that..

Code:

bool is_list(std::string s, list& l)
{
    try{l=list(s);}
    catch(exception e){if(e.what()==std::string("BAD_LIST: "+s)) return false;}
    return true;
}

But I still can't understand the difference between this and doing the same in the constructor.
(As the validity of the list object created doesn't matter if the string is invalid)

As for a different name...I would do so when refactoring...(just list ...in my namespace is a short and sweet name for quickly writing the code.)

Code:

bool is_list(std::string s, list& l)
{
    try{l=list(s);}
    catch(exception e){if(e.what()==std::string("BAD_LIST: "+s)) return false;}
    return true;
}

Don't catch exceptions by value!

O_o

This function says that the list is valid if, for example, allocation from `new' fails.

But I still can't understand the difference between this and doing the same in the constructor.

One is right; the other is wrong, but if you need a reason, look no further than consistency.

If the constructor fails for any reason other than "this string is not a list" the constructor has to raise an exception. Why do you want a special case to ask a question that can be more truly answered using a different method?

Soma

When it comes to the second solution, the IsValid() function must be named differently, if it is supposed to create any object.

But I still can't understand the difference between this and doing the same in the constructor.
(As the validity of the list object created doesn't matter if the string is invalid)

Created object must be valid. If constructor of any object succeeded, that object must be usable.

As for a different name...I would do so when refactoring...(just list ...in my namespace is a short and sweet name for quickly writing the code.)

I consider STL naming convensions (like that one) very bad, which is fine, since noone is going to create confusing names ;].

Originally Posted by phantomotap

Don't catch exceptions by value!

Oops.. slipped.

This function says that the list is valid if, for example, allocation from `new' fails.

What new ? ...isn't the memory for 'l' already there?

If the constructor fails for any reason other than "this string is not a list" the constructor has to raise an exception. Why do you want a special case to ask a question that can be more truly answered using a different method?

Consider this usage of the checking:

Code:

    std::string setv(list& l)
    {
        if(l.size()!=2)throw(exception("BAD_SETV"));
        std::string name=l.car(),value=l.cdr().car();


        list result;
        if(is_list(value,result))value=result.eval();
        
        auto v = var_scope.find(name);
        if(v!=nullptr)*v = value;
        else var_scope.new_global_val(name,value);
        return name;
    }
        //VS
    std::string setv(list& l)
    {
        if(l.size()!=2)throw(exception("BAD_SETV"));
        std::string name=l.car(),value=l.cdr().car();
                
                bool test(true);
        list result(value,test);
        if(test) value=result.eval();
        
        auto v = var_scope.find(name);
        if(v!=nullptr)*v = value;
        else var_scope.new_global_val(name,value);
        return name;
    }

My point is that, the function itself calls the constructor...so all the potential problems remain same....other exceptions will propagate downwards the same way.

I disagree with your comment that validity of the list object created doesn't matter if the string is invalid. Your approach is propagating an invalidly constructed object to the caller. That is an unnecessary invitation for the caller to use an improperly constructed object.

Try implementing your is_list() function like this.

Code:

bool is_list(const std::string &s)
{
     bool retval(true);
     try{list s;}
     catch(...){retval = false;}
     return retval; 
}

With this approach, as far as the caller is concerned, validity of the string can be checked, but the caller has no access to any invalidly constructed object.

If the caller wants to create an object regardless, the caller can still use the constructor directly, and then deal with any exception thrown.

Originally Posted by kmdv

When it comes to the second solution, the IsValid() function must be named differently, if it is supposed to create any object.

Ok..

Created object must be valid. If constructor of any object succeeded, that object must be usable.

Hence it could make a blank list..if the exception is caught.

Originally Posted by grumpy

If the caller wants to create an object regardless, the caller can still use the constructor directly, and then deal with any exception thrown.

Why construct the same thing twice ?
(Wouldn't that quickly add up to a lot of slowdown?)

You're overly attached to the notion that an object must be created. If a constructor throws, there is no longer an object.

I am not suggesting constructing anything twice. You are.

If you are checking validity of a string, you (presumably) only subsequently need an object if the string is valid.

Originally Posted by grumpy

You're overly attached to the notion that an object must be created. If a constructor throws, there is no longer an object.

I am not suggesting constructing anything twice. You are.

If you are checking validity of a string, you (presumably) only subsequently need an object if the string is valid.

But the constructor runs, even if not to completion...
If the string to be checked is..say 200 lines... that would result in the thing being parsed twice.(..or rather 1.x times).
Surely doing it once is better ~..?

O_o

So, to sum up, you are building a `list', whatever it is, as you parse the contents of a `string' and only want to construct one `list' object while staying true to good design and implementation patterns?

Instead of going further down this path for now, I have to ask, why don't you make construction of an empty `list' very cheap and provide a copy assignment that is destructive in nature?

If constructing the empty `list' on the stack is almost free, constructing it can't be a "hotspot".

If taking one `list' that is already constructed and swapping it for another `list' that is already constructed is almost free, swapping the two can't be a "hotspot".

In other words, your implied reason for not using a free function to build the string is built on misunderstanding and bad design.

Consider this:

Code:

list result; // L1
if(is_list(value,result))value=result.eval();

and now this:

Code:

bool trytobuildlistfromstring(std::string s, list& l /*L2*/)
{
try{list newlist(s); /*L3*/
l.swap(newlist);}
catch(...){return false;}
return true;
}

while maintaining that the constructor must succeed or fail spectacularly with a raised exception.

Look at all the advantages this approach has:

We know that construction will succeed or fail spectacularly always; there are no special aspects to remember.

If the creation of 'L1' is almost free, as it certainly will be if it is just initializing a few pointers to null, the cost of that object is trivial.

As construction is all or nothing, we can see that 'L2' (and thus 'L1') will not be modified if the string can't be parsed correctly.

If the members of the `list' objects are swapped, instead of using a potentially expensive copy operation, the construction is "free" in that we will continue using those elements as referenced from 'L2' (and thus 'L1').

The function is clear and concise. (Though the name is verbose here for other purposes.)

Soma

The constructor is only invoked because you are choosing to invoke it in your is_list() function. If there is some realistic option to inexpensively check the string (without creating a list) then THAT is what you need to do in the is_list() function.

If there is no such realistic option, and you care as much as you (apparently) do about parsing the string more than once, or how many times the constructor runs, then you only need the single constructor.

All that then requires is that the user of your class always attempt to create an object, and then handles (or propagates) any exception that is thrown.