Cleanup / Reset before or after operation

Question

So this is the situation:

I want to use a class of 3rd party library (namely Poco) to parse a JSON string. This Parser class is designed in such a way that after each parsing (operation) it has to be reseted (by calling reset(), who would have thought it?). I reuse one Parser multiple times on different places inside the class I'm implementing, therefor this is important for me. Calling reset() even if the class is reseted already is harmless.

Therefor my question: Should I rather make sure that I reset the parser before each operation or should I ensure that I reset it after an operation.

The first case is handy because it is guaranteed that my parser is in a correct state before operation, but at the same time I feel dirty because I leave the parser in an invalid state and others might expect that I reset the parser.

The second case I dislike because I fear that I could miss a case where my method might exit (e.g a uncaught exception) leaving the parser in an invalid state while other expect it for sure to be valid this time. Of course this could be solved by using RAII (like a scoped lock) but this sounds kind of over engineered for me.

Another solution could be to combine both cases, so: reset, parse, and reset again; but this is kind of redundant IMO.

---

I'm really unsure here and can't decide for any of these cases and I hope that your opinion (based on design-related considerations, don't you close my question! ;) ) will help me to choose.

Answer 1

Well I would advise not using Poco at all but rather looking at JsonCpp. The design choice Poco made by making you call reset on it is rather odd. That aside, the RAII approach is fairly simple:

template <typename T>
struct ScopedParser {
    T& parser; // parser object

    ScopedParser(T& p) : parser(p) {} // set parser in constuctor
    ScopedParser(ScopedParser&&) = delete; // no moving
    ScopedParser(const ScopedParser&) = delete; // no copying

    ~ScopedParser() {
        parser.reset(); // reset it
    }
}

Example on how to use it:

void myFunc() {
    Poco::JSONParser p;
    ScopedParser<Poco::JSONParser> pScoped(p);
}

Answer 2

This seems a suited case for embedding your parser in a RIIA style class. The goal is to enfore the creation of one unique "user" of your parser.

Here is a quick mock up:

class MyParserBuilder
{
public:

  class MyParser
  {
  public:
    MyParser(poco::Parser& parser, MyParserBuilder& builder) : 
    _parser(parser), 
    _builder(builder)
    {

    }

    ~MyParser() 
    { 
      _builder.reset(); 
    }

   // Either expose some functions from the parser or use 
   // something like:
    poco::Parser& operator*() { return _parser; }

  private:
    poco::Parser& _parser;
    MyParserBuilder& _builder;
  };

  MyParserBuilder() : _used{false} {}

  std::unique_ptr<MyParser> build() 
  { 
      if (_used) // Or lock ?
      {
        return std::unique_ptr<MyParser>();
      }
      _used = true;
      return std::unique_ptr<MyParser>(new MyParser(_parser, *this)); 
    }

    void reset()
    {
      _parser.reset();
      _used = false;
    }

  private:
    bool _used; 
    poco::Parser _parser;
};

http://ideone.com/6wwg20

I would also add, for safety, something like a "dirty" state so that you can only do the operation that need a reset once per instantiation of MyParser.

Answer 3

The first case sounds fine to me as long as the "others" are only other member functions operating on the same private Parser object.

Combining both cases would only reduce the potential errors. There could still be an uncaught exception and others might expect that the parser is reset.

The best option would still be a RAII lock-like helper object. I don't think a class of like 5 lines is over engineering. You could add a real lock in there if the parser is used by multiple threads.

Or even have a new scoped Parser for every function, if it's not a problem for the performance of your application. This would probably even be faster than synchronising access to the same parser.