Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote my own exception class, deriving from std::runtime_error to have Error-IDs, timestamps and inner exceptions. It seems to work, but are there any drawbacks?

The only thing I see is the deep-copy in the copy-constructor, which is not efficient, when there are many nested exceptions. But exceptions should be rare and not be nested too much, so I think it's a downside I can cope with.

#pragma once

#include <stdexcept>
#include <string>
#include <sstream>
#include <time.h>
#include <memory>


class MyException : public std::runtime_error
{
public:
    MyException(const MyException& exception)
        : std::runtime_error(exception.what()),
        exceptionId(exception.id()),
        ts(exception.timestamp()),
        innerException(NULL)
    {
        if (exception.inner() != NULL)
        {
            innerException = new MyException(*exception.inner());
        }
        else
        {
            innerException = NULL;
        }
    }

    MyException(const std::string& _Message)
        : std::runtime_error(_Message),
            exceptionId(0),
        innerException(NULL)
    {
        time(&ts);
    }

    MyException(const std::string& _Message, unsigned int id)
        : std::runtime_error(_Message),
        exceptionId(id),
        innerException(NULL)
    {
        time(&ts);
    }

    MyException(const std::string& _Message, unsigned int id, MyException* innerException)
        : std::runtime_error(_Message),
        exceptionId(id),
        innerException(new MyException(*innerException))
    {
        time(&ts);
    }

    virtual ~MyException()
    {
        delete innerException;
    }

    unsigned int id() const { return exceptionId; }
    time_t timestamp() const { return ts; }
    const MyException* inner() const { return innerException; }

private:
    unsigned int exceptionId;
    time_t ts;
    const MyException* innerException;
};

This is how I would use it:

void handleException(MyException *ex)
{
    cout << "exception " << ex->id() << " - " << ex->what() << endl;
    const MyException* innerException = ex->inner();
    int t = 1;
    while (innerException != NULL)
    {
        for (int i=0; i<t; ++i)
        {
            cout << "\t";
        }
        ++t;
        cout << "inner exception " << innerException->id() << " - " << innerException->what() << endl;
        innerException = innerException->inner();
    }
}

void throwRecursive(int temp)
{
    if (temp == 0)
    {
        throw runtime_error("std::runtime_error");
    }

    try
    {
        throwRecursive(--temp);
    }
    catch (MyException &ex)
    {
        throw MyException("MyException", (temp+1), &ex);
    }
    catch (exception &ex)
    {
        throw MyException(ex.what(), (temp+1));
    }
}

void myExceptionTest()
{
    try
    {
        throwRecursive(3);
    }
    catch (MyException &ex)
    {
        handleException(&ex);
    }
}

And the output:

exception 3 - MyException
        inner exception 2 - MyException
                inner exception 1 - std::runtime_error

UPDATE

I already found out here that I'm violating the rule of three, having forgotten the copy assignment operator.

Other issues mentioned: cloning, reserved names and throw specifications

Can anyone tell me more about that?

Edit 1

I redesigned my exception class, following Loki Astaris advices. I'm now storing the inner exception in a std::shared_ptr, which makes things simpler. No copy constructor, destructor or copy assignment operator needed anymore.

#pragma once // code doesn't have to be portable to other compilers

#include <stdexcept>
#include <string>
#include <ctime>
#include <memory>

class MyException : public std::runtime_error
{
public:

    MyException(const std::exception& ex)
        : std::runtime_error(ex.what()),
        exceptionId(0)
    {
        time(&ts);
    }

    MyException(const std::string& message, unsigned int id = 0, std::exception* innerException = NULL)
        : std::runtime_error(message),
        exceptionId(id)
    {
        time(&ts);
        if (innerException != NULL)
        {
            MyException *myex = dynamic_cast<MyException*>(innerException);
            if (myex != NULL)
            {
                this->innerException = make_shared<MyException>(*myex);
            }
            else
            {
                this->innerException = make_shared<MyException>(MyException(*innerException));
            }
        }
    }

    unsigned int id() const { return exceptionId; }
    time_t timestamp() const { return ts; }
    const std::runtime_error* inner() const { return innerException.get(); }

private:
    unsigned int exceptionId;
    time_t ts;
    std::shared_ptr<std::runtime_error> innerException;
};
share|improve this question
    
I don't think this is on-topic here, but I would have some advice. –  John Dibling Oct 9 '12 at 13:36
3  
I voted to close as "not constructive". i can't see that it's off topic. anyways, issues that OP may want to address: (1) rule of three, (2) cloning, (3) c++11 exception propagation and inner exception facilities, (4) reserved names, (5) throw specifications. In other words, much wrongness here. Even an answer would be too short to discuss it all. –  Alf P. Steinbach Oct 9 '12 at 13:41
1  
It is generally expected that you will create your own exception classes so that you can specifically catch them and handle the special errors that occur in your code differently than anything the standard libraries would throw. So why not have a base exception class all your own exceptions derive from that contains common "extra" functionality like timestamps? Good idea. –  Clark Oct 9 '12 at 13:41
    
Oh, I see... but why is it off-topic? Just because there is a better-suited site for it? Is the question not specific enough? –  Ben Oct 9 '12 at 13:41
    
@ben right, it's not specific enough, and there is much better-suited site –  Alf P. Steinbach Oct 9 '12 at 13:43
show 3 more comments

1 Answer 1

Not all compilers support #pragma once

#pragma once

There are C++ equivalent of most C libraries that put appropriate interface into the standard namespace.

#include <time.h>

// Prefer

#include <ctime>

My pet peeve (OK second after using namespace std;); because everybody thinks they know the rules but get it wrong all the time:

_Message

DO NOT USE IDENTIFIERS WITH A LEADING UNDERSCORE. Even if you know the rules most people get it wrong (such as this case) as a result it leads to maintenance problems. http://stackoverflow.com/a/228797/14065 An identifier with a leading underscore and a capitol letter is reserved in all scopes (ie the implementation is potentially going to use a macro with that name and Message seems like a prime candidate).

Your three version of the constructor can be written as a single constructor:

    MyException( const std::string& _Message
               , unsigned            int id = 0             // Use default values
               , MyException*        innerException = NULL) // That way you do not have
        : std::runtime_error(_Message)                      // three versions of the
        , exceptionId(id)                                   // same constructor
        , innerException(innerException)
    {
        time(&ts);
    }

A class is its own friend.
So you don't need to call geter methods to access members of the object you are copying in the copy constructor; you can just get the values. When constructing the base class (std::runtime_error) why not take advantage of its copy constructor (it may have optimizations that you are failing to take use by using the non standard version.

MyException(const MyException& exception)
    : std::runtime_error(exception),
    , exceptionId(exception.id),
    , ts(exception.timestamp),
    , innerException(NULL)

The inner exceptions is passed around as a pointer. So we have no ownership semantics associated with it. You attempt to take ownership but fail (because you do not implement the rule of 3 (5 in C++11)).

{
    if (exception.inner != NULL)
    {
        // Slicing problem if exception.inner is derived from MyException
        innerException = new MyException(exception.inner);
    }
    else
    {
        // Already set in the initializer list no need to set again.
        innerException = NULL;
    }
}

Also because of the way you implement it you can not subclass MyException because the copy constructor will slice your object on copy. You should probably use a smart pointer to handle ownership of the exception. Since sharing the same exception would solve most of your problems the easy way out is to use shared_ptr<X>. Personally I would make X a std::runtime_errornotMyException` thus allowing you to chain on standard exceptions (or anything derived from them).

In the handler:

void handleException(MyException *ex)

Why pass a pointer to the exception. Now you have to check for NULL. Pass it as a reference.

share|improve this answer
    
Wow! Thank you for this detailed answer. I'm new to this site. Can I simply edit my question by replacing my code by an improved version for a next revision step? –  Ben Oct 9 '12 at 17:09
    
@Ben: Rather than replace your answer Add a new section below with a title called ###edit 1 –  Loki Astari Oct 9 '12 at 18:30
    
I edited my question. Please have a look at it! –  Ben Oct 10 '12 at 9:13
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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