views:

216

answers:

9

I'm working on a class that handles interaction with a remote process that may or may not be available; indeed in most cases it won't be. If it's not, an object of that class has no purpose in life and needs to go away.

Is it less ugly to:

  1. Handle connection setup in the constructor, throwing an exception if the process isn't there.
  2. Handle connection setup in a separate connect() method, returning an error code if the process isn't there.

In option 1), the calling code will of course have to wrap its instantiation of that class and everything else that deals with it in a try() block. In option 2, it can simply check the return value from connect(), and return (destroying the object) if it failed, but it's less RAII-compliant,

Relatedly, if I go with option 1), is it better to throw one of the std::exception classes, derive my own exception class therefrom, roll my own underived exception class, or just throw a string? I'd like to include some indication of the failure, which seems to rule out the first of these.

Edited to clarify: The remote process is on the same machine, so it's pretty unlikely that the ::connect() call will block.

+5  A: 

I consider it bad to do a blocking connect() in a constructor, because the blocking nature is not something one typically expects from constructing an object. So, users of your class may be confused by this functionality.

As for exceptions, I think it is generally best (but also the most work) to derive a new class from std::exception. This allows the catcher to perform an action for that specific type of exception with a catch (const myexception &e) {...} statement, and also do one thing for all exceptions with a catch (const std::exception &e) {...}.

See related question: How much work should be done in a constructor?

SoapBox
There are exceptions of course where blocking constructors make sense, RAII-based locks for example. Of course then documentation and appropriate naming are important.
Georg Fritzsche
+1 By doing blocking operations in a separate connect method, you leave the door open for a `cancel` method to cancel that operation.
Emile Cormier
+1  A: 

I would go with the second one, since I believe that the constructor should not do any other thing than initialize the private members. Besides that, it's easier to deal with failures (such as not connecting). Depending on what you're exactly going to do, you could keep the object alive and call the connect method when you need it, minimizing the need of creating another object.

As for the exceptions, you should create your own. This will allow the caller to take specific rollback actions when needed.

Fernando
+1  A: 

Don't connect from the constructor, a constructor that blocks is unexpected and bad API design.

Write a connect method and mark your class noncopyable. If you rely on instances being connected already, make the constructor private and write a static factory method to get pre-connected instances.

Tobu
+1  A: 

If the connection would take a long time, it is more reasonable to put the code in another method. Still, you can (and you should) use exceptions to inform the caller whether your connect() method has been successful or not, instead of returning error codes.

It is also more advisable to create a new exception class derived from std::exception instead of throwing plain data or even throwing other STL exceptions. You may also derive your exception class from a more specific description of your error (for example, deriving from std::runtime_error), but this approach is less common.

Alek
+1  A: 

I think Option 1 is a better approach but you need to think how would you expect the consumer of the class to use this? Just the fact that they have wired it up is good enough to go ahead and connect (Option 1) or the fact they should have the option to call Connect() when they are good and ready (Option 2)?

RAII also supports the DRY principle (don't repeat yourself). However with Option 1 you need to ensure you Exception handling is spot on and you don't get into race conditions. As you know, if there is an exception thrown in the constructor the destructor wont be called to clean up. Also be vary of any static functions you might have as you will need locks around those as well - leading you down a spiral path.

If you haven't seen this post yet its a good read.

bahree
A: 

Under the RAII mind of thought, isn't this by definition good? Acquisation is Initialization.

anon
+3  A: 

Regarding throwing exceptions, its perfectly fine to create your own classes. As a hypothetical user I'd prefer if they derived from std::exception, or perhaps std::runtime_error (which allows you to pass an error string to the ctor).

Users who want to can catch your derived type, but the common idiom of:

   try {
     operation_that_might_throw ();
   } catch (std::exception& e) {
     cerr << "Caught exception: " << e.what() << endl;
   }

will work for your new exception types as well as anything thrown by the C++ runtime. This is basically the Rule of Least Surprise.

Bklyn
A: 

If your connection object is effectively non-functional if the connection fails then it doesn't make sense to have the object exist if all its other methods will always do nothing or throw exceptions. For this reason I would perform the connect in a constructor and fail by throwing an exception (derived from std::exception) if this method fails.

However, you are right that clients of the class may need to be aware that the constructor might block or fail. For this reason I might choose to make the constructor private and use a static factory method (named constructor idiom) so that clients have to make an explicit MakeConnection call.

It is still the client's responsibility to determine if not having a connection is fatal to it, or whether it can handle an offline mode. In the former case it can own a connection by value and let any connection failure propogate to its clients; in the latter it can own the object via a pointer, preferably 'smart'. In the latter case it might choose to attempt construction of the owned connection in its constructor or it might defer it until needed.

E.g. (warning: code all completely untested)

class Connection
{
    Connection(); // Actually make the connection, may throw
    // ...

public:
    static Connection MakeConnection() { return Connection(); }

    // ...
};

Here's a class that requires a working connection.

class MustHaveConnection
{
public:
    // You can't create a MustHaveConnection if `MakeConnection` fails
    MustHaveConnection()
        : _connection(Connection::MakeConnection())
    {
    }

private:
    Connection _connection;
};

Here's a class that can work without one.

class OptionalConnection
{
public:
    // You can create a OptionalConnectionif `MakeConnection` fails
    // 'offline' mode can be determined by whether _connection is NULL
    OptionalConnection()
    {
        try
        {
            _connection.reset(new Connection(Connection::MakeConnection()));
        }
        catch (const std::exception&)
        {
            // Failure *is* an option, it would be better to capture a more
            // specific exception if possible.
        }
    }

    OptionalConnection(const OptionalConnection&);
    OptionalConnection& operator=(const OptionalConnection&);

private:
    std::auto_ptr<Connection> _connection;
}

And finally one that creates one on demand, and propogates exceptions to the caller.

class OnDemandConnection
{
public:
    OnDemandConnection()
    {
    }

    OnDemandConnection(const OnDemandConnection&);
    OnDemandConnection& operator=(const OnDemandConnection&);

    // Propgates exceptions to caller
    void UseConnection()
    {
        if (_connection.get() == NULL)
            _connection.reset(new Connection(Connection::MakeConnection()));

        // do something with _connection
    }

private:
    std::auto_ptr<Connection> _connection;
}
Charles Bailey
A: 

Another thing that was unclear in my original post is that the client code doesn't have any interaction with this object once it's connected. The client runs in its own thread, and once the object is instantiated and connected, the client calls one method on it that runs for the duration of the parent process. Once that process ends (for whatever reason), the object disconnects and the client thread exits. If the remote process wasn't available, the thread exits immediately. So having a non-connected object lying around isn't really an issue.

I found another reason not to do the connection in the constructor: it means that I either have to handle teardown in the destructor, or have a separate disconnect() call with no separate connect() call, which smells funny. The teardown is non-trivial and might block or throw, so doing it in the destructor is probably less than ideal.

ceo