1
\$\begingroup\$

I am trying to implement the Execute Around pattern described in Kent Beck's Smalltalk Best Practice Patterns. An example in Java could be found here.

Basically, I am repeatedly opening and closing a pdf document while performing various operations, something like,

public void Parse()
{
    // Open the document
    PdfLoadedDocument loadedDocument = new PdfLoadedDocument("plan.pdf");

    List<string> annotations = Build(loadedDocument);
    // work with annotations. 

    // Close the document
    loadedDocument.Save();
    loadedDocument.Close();
}

I would like to move the opening and closing the document in a centralized place, as I have tens of similar methods. All these methods open the document, perform an action, and close the document, and it's easy to forget to close the document.

Here is what I tried:

public void BuildAnnotations()
{
    List<string> annotations = null;

    ExecuteAction("plan.pdf", (PdfLoadedDocument loadedDocument) =>
    {
        annotations = Build(loadedDocument);
        // work with annotations
    });
}

private void ExecuteAction(string path, Action<PdfLoadedDocument> perform)
{
    PdfLoadedDocument loadedDocument = new PdfLoadedDocument(path);

    try
    {
        perform(loadedDocument);
    }
    catch(Exception e)
    {
        Console.WriteLine($"An error occured. {e}");
    }

    loadedDocument.Save();
    loadedDocument.Close();
}

My question is, is passing a lambda to an Action delegate a good idea? I am not that familiar with delegates, Actions, and lambdas (other than using them in linq queries). Are there any other better alternatives?

\$\endgroup\$
  • \$\begingroup\$ The Parse method doesn't do any work and you do not use the result of Build anywhere. I suppose this is not your real code but some shortened version. Please update your question and post what you currently have and what you are trying to improve. We need to see the real and complete code. \$\endgroup\$ – t3chb0t May 6 '19 at 6:34
  • \$\begingroup\$ Yes, I have simplified my code example to make it easy to understand. The actual code does indeed manipulate annotations. \$\endgroup\$ – Akshay Khot May 6 '19 at 15:17
1
\$\begingroup\$

IMO there is a problem with the try...catch statement. If an exception is thrown you'll have to deal with communicating the error to the client. You can't rethrow it because then the Save() and Close() aren't called, so you'll have to have a kind of logging or event to notify through and that could make it all unnecessarily complicated. Besides that: if an exception is thrown, you may not want to save the document in an unknown state, but you should close it anyway.

You can handle that in the following way:

  try
  {
    perform(loadedDocument);
    loadedDocument.Save();
  }
  finally
  {
    loadedDocument.Close();
  }

Here the document is only saved if the process went well, but closed in any case. And the caller can handle the exception as needed.


Alternatively you can make a wrapper like:

  class PdfDocumentExecuter : IDisposable
  {
    private PdfLoadedDocument document;

    public PdfDocumentExecuter(string fileName)
    {
      document = new PdfLoadedDocument(fileName);
    }

    public void Execute(Action<PdfLoadedDocument> action)
    {
      action(document);
      document.Save();
    }

    public T Execute<T>(Func<PdfLoadedDocument, T> function)
    {
      T result = function(document);
      document.Save();
      return result;
    }

    public void Dispose()
    {
      if (document != null)
      {
        document.Close();
        document = null;
      }
    }
  }

called as:

  using (PdfDocumentExecuter executer = new PdfDocumentExecuter("path"))
  {
    annotations = executer.Execute(Build);
  }
\$\endgroup\$
  • 1
    \$\begingroup\$ IDisposable is what I was going to recommend. Let the language do the heavy lifting. \$\endgroup\$ – Peter Taylor May 6 '19 at 7:18

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