2
\$\begingroup\$

I have a small C++/Qt library that interfaces with an application in another language (so interfacing must be performed using primitives, notably pointers to Handler objects). To implement debug logging, each Handler has its own debug file, managed by a Log object. These are both constructed in the initializeHandler function.

To make use of RAII, I didn't want Log objects to have a default constructor - they have a copy-constructor, and a constructor that takes a QString to open a QFile with. If the Log can't open the file, I want the initializeHandler function to fail, so I made it throw a LogExceptFailedOpen exception. To interact with the application using my library, I have to catch that exception and re-interpret it as a returned error code (I can't just let the exception propogate - the calling application can't catch it). But I want to use the Log object if construction succeeds. Which leads me to my problem.

If I declare and initialize the Log in the try block, I can't access it after the catch block. I can get around this by using a QSharedPointer<Log> with a scope larger than the try block, then using the Log copy-constructor to create another Log object afterwards (or I could just use the Log stored in logPtr, but that's not the point). However, this feels like a hack, and I feel that the code structure could instead be reworked somehow.

There's some discussion here around someone with a similar problem, which suggests that I should either

  • do all my work within the try block, or
  • abandon RAII for Log objects.

Neither option appeals to me very much. RAII is important for Log objects, as they get passed around and copied a lot and it would be a major hassle to look for all the places where they could go out of scope.

How can I improve my code to avoid this problem?

initializeHandler function:

typedef int ErrorCode;
#define ERROR_FILE_FAILED_OPEN   -12
#define ERROR_INVALID_ARGS       -1
#define ERROR_OK                 0

ErrorCode initializeHandler(Handler*& handler, const char * logFile, int x, int y)
{
    QSharedPointer<Log> logPtr;
    try {
        logPtr.reset(new Log(QString(logFile)));
    } catch (const LogExceptFailedOpen& e) {
        return ERROR_FILE_FAILED_OPEN;
    }
    Log logger(*logPtr);

    logger.log("Log started.");
    logger.log("Initializing handler...");

    Handler* temp;
    try {
        temp = new Handler(logger, x, y);
    } catch (const HandlerExceptInvalidArgs& e) {
        logger.log("Arguments invalid. x = " + QString::number(x)
                   + ", y = " + QString::number(y));
        return ERROR_INVALID_ARGS;
    }
    handler = temp;

    logger.log("Successfully initialized handler.");

    return ERROR_OK;
}

Log class:

class Log
{
private:
    QSharedPointer<QFile> file;
    QSharedPointer<QTextStream> stream;
public:
    void log(const QString& message)
    {
        (*stream) << (message + "\r\n");
    }

    // file constructor
    Log(const QString& fileName) : file(fileName)
    {
        if (!file->open(QFile::WriteOnly)) {
            throw LogExceptFailedOpen();
        }
        stream = QSharedPointer<QTextStream>(new QTextStream(file.data()));
    }

    // copy-constructor
    Log(const Log& other) : file(other.file), stream(other.stream) {}

}
\$\endgroup\$
1
\$\begingroup\$

It would be great to be able to discern what outside requirements and what are requirements that you are imposing because you are imagining a specific way to solve this problem. You don't post the Handler class. If you want a logger per Handler why not have the handler class create the logger ?

With regard to RAII, all objects destructors will be called at the end of the program, unless you need to open and close these files for other reasons, is there really an upside to make things more complicated than they already are ?

But yes that solution with the pointer and the copy doesn't look good, it smells like you trying to enforce a specific way of dealing with things that just doesn't fit the semantics of the code. The copy constructor in the logger is only there because of the way you want to create the logger, there doesn't seem to be any outside requirement that says that copy constructor is needed. If you need to detect two different error conditions from exceptions you can just throw two different exception types, when you create your handler.

Unless there is a specific reason for having the initializeHandler function you could as well handle most of the logic you have in that function in the constructor and do the errorhandling in various catch blocks. if you throw the exceptions in the constructor you don't have to check in the constructor if the preceding operations succeeded, that is one of the advantages of throwing exceptions for error handling. Also think about how often you expect the file open function to fail, if you expect this to be a regular occurrence then the handling would be different than if this is an 'exceptional' occurrence.

Here is one option how handler initialization could be handled.

QPointer<Handler> handler;
try {
    handler = new Handler(x, y, "Handler.txt");
  }
  catch (FileOpenException e) {
    // Handle file Open Exception
  }
  catch (HandlerException e) {
    // Handle Handler failure
  }
}

if (! handler.isNull())
{
  doProcessing();
}

Unless there are other requirements that force you to copy the logger, get rid of the copy constructor, then you don't have to use the QSharedPointer in your logger class. Instead you can just use plain stack members.

\$\endgroup\$
  • \$\begingroup\$ "Unless there are other requirements that force you to copy the logger" - the Handler object stores multiple other objects that use the same logger in their functions. I pass the Log object into these other objects' constructors, and it gets copied into a member variable. The alternatives are to pass it by const reference to every sub-object function, or to pass/store a shared pointer to the constructors. In the former case, every function would need an extra parameter, and in the latter, it would need to be accessed with -> as a pointer-to member. (cont...) \$\endgroup\$ – Jamie S Sep 18 '17 at 16:50
  • \$\begingroup\$ (...cont) The current implementation makes it very easy to use: logger.log("message"); can be placed in any function in all classes in the library, because they all have a logger object. Adding an extra parameter to every function seems like it would clutter the code, and changing the storage from a direct member to a pointer-to-member seems like a similar problem. However, I'm not an expert, so I would love an explanation if my intuition is wrong. \$\endgroup\$ – Jamie S Sep 18 '17 at 16:56
  • \$\begingroup\$ If you do need to share loggers using QSharedPointer is a better way, just copying it in the constructor is fine. For example by copying the instance as you do you prevent any kind of control over the output, be that for example debug levels (changing it in one logger would have to be copied to all other loggers) or protecting the output stream in case of multithreaded access to logging. ostream is not threadsafe. With multiple instances of the logging class for one stream it's harder to control access to the stream. \$\endgroup\$ – Harald Scheirich Sep 18 '17 at 19:37
  • \$\begingroup\$ Hmm, good comments. I had written a bunch of code to help with multithreading, and it worked, but it definitely wasn't elegant. Moving it into a QSharedPointer<Log> and putting the Log initialization into the Handler's construction cleaned it up a fair bit, thanks! \$\endgroup\$ – Jamie S Sep 18 '17 at 21:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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