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

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?

share|improve this question
    
First glance: innerException == NULL; could be quite a nasty typo. –  Corbin Oct 9 '12 at 14:47

1 Answer 1

Not all compilers support #pragma once

#pragma once

There are C++ equivalent of most C libraries that put the 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 capital letter is reserved in all scopes (i.e. the implementation is potentially going to use a macro with that name and Message seems like a prime candidate).

Your three versions 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 getter 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 of by using the non standard version).

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

The inner exception 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

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.