3

I have the need to do some logging within my code. I'm required to use an internal company-developed library to record some information. Here's how it works.

Recorder recorder = Recorder.StartTiming();
DoSomeWork();
recorder.Stop();  // Writes some diagnostic information.

To ensure that Stop() is always called, I created a wrapper class that allows a clean "using" block.

using (RecorderWrapper recorderWrapper = new RecorderWrapper)  // Automatically calls Recorder.StartTiming() under the covers
{
   DoSomeWork();
}  // When the recorderWrapper goes out of scope, the 'using' statement calls recorderWrapper.Dispose() automatically - which calls recorder.Stop() under the covers

it's worked well so far. However, there's a change my company is requiring, that would look something like this on the original code:

Recorder recorder = Recorder.StartTiming();
try
{
   DoSomeWork();
}
catch (Exception ex)
{
   recorder.ReportFailure(ex);  // Write out some exception details associated with this "transaction"
}
recorder.Stop();  // Writes some diagnostic information.

I'd like to avoid try/catches in all my 'using' scope blocks with RecorderWrapper. Is there a way I can accomodate the "ReportFailure()" call and still leverage the 'using' scope block?

Specifically, I want everyone on my team to "fall into a pit of success", i.e. make it easy to do the right thing. To me, this means making it really hard to forget to call recorder.Stop() or forget the try/catch.

Thanks!

4
  • Use a try/catch inside the using block. Am I missing something here? Commented Nov 16, 2009 at 19:29
  • I guess the OP wants to either use "using" or a try..catch..finally, but not nest another try inside the using block. Which makes sense. Commented Nov 16, 2009 at 19:34
  • why in the name of all that's holy would you want to use such an abomination? looks like a code smell to me! Commented Nov 16, 2009 at 21:18
  • and if you really want to use such a construct, at least use something like postsharp to achieve this, and don't sprinkle your code with this! Commented Nov 16, 2009 at 21:20

8 Answers 8

7

You might be able to create a method on the recorder to hide this:

public void Record(Action act)
{
    try
    {
        this.StartTiming();
        act();
    }
    catch(Exception ex)
    {
        this.ReportFailure(ex);
    }
    finally
    {
        this.Stop();
    }
}

So your example would then just be:

recorder.Record(DoSomeWork);
Sign up to request clarification or add additional context in comments.

3 Comments

I like this answer too - I added the finally block to make it safer though.
And if you can't change the recorder there's always the extension method
Yeah, this is what I really want
3

You could always try something like:

Edit by 280Z28: I'm using a static StartNew() method here similar to Stopwatch.StartNew(). Make your Recorder class IDisposable, and call Stop() from Dispose(). I don't think it gets any more clear than this.

using (Recorder recorder = Recorder.StartNew())
{
    try
    {
        DoSomeWork();
    }
    catch (Exception ex)
    {
        recorder.ReportFailure(ex);
    }
}

7 Comments

This is exactly how you handle it - reformatted it to make it clear what's going on though.
Thanks. I purposely left out the braces in the using block to highlight how close it was to what the original poster was trying to accomplish.
+1 - never seen that before! and it compiles! nifty! wait a second! reformatted, looks like my answer! d'oh! was just regular block nesting. you still keep the +1 though ;)
@Rob: Unfortunately it became completely unreadable IMO. I updated it to use a StartNew method since the .NET Framework has a similarly named function, and the consistency makes things easier to understand.
How is different that a try/catch/finally?
|
3

You could continue to use the RecorderWrapper you have, but add a TryExecuting method that accepts a lambda of what you want to happen add runs it in a try/catch block. eg:

using (RecorderWrapper recorderWrapper = new RecorderWrapper)  // Automatically calls Recorder.StartTiming() under the covers
{
    recorderWrapper.TryExecuting(() => DoSomeWork());
}

Inside RecorderWrapper:

public void TryExecuting(Action work)
{
    try { work(); }
    catch(Exception ex) { this.ReportFailure(ex); }
}

Comments

1

You could copy the pattern used by TransactionScope, and write a wrapper that must be actively completed - if you don't call Complete(), then the Dispose() method (which gets called either way) assumes an exception and does your handling code:

using(Recorder recorder = Recorder.StartTiming()) {
    DoSomeWork();
    recorder.Complete();
}

Personally, though, I'd stick with try/catch - it is clearer for maintainers in the future - and it provides access to the Exception.

Comments

1

No, a using block is only syntactic sugar for a try/finally block. It doesn't deal with try/catch. At that point you're going to be left with handling it yourself since it looks like you need the exception for logging purposes.

Comments

1

A using block is effectively a try/finally block that calls dispose on the object in question.

So, this:

using(a = new A())
{
    a.Act();
}

is (i think, exactly) equivalent to this:

a = new A();
try
{
    a.Act();
}
finally
{
    a.Dispose();
}

And you can tack your catches onto the end of the try block.

Edit:

As an alternative to Rob's solution:

Recorder recorder = Recorder.StartNew()
try
{
    DoSomeWork();
}
catch (Exception ex)
{
    recorder.ReportFailure(ex);
}
finally
{
    recorder.Dispose();
}

3 Comments

What you say is true, but that avoids the question I'm trying to ask.
The answer is: no, the only way to have a catch block is to have a try block.
It's not exactly equivalent, although close. (See section 8.13 of the C# specification.) There is a block around the entire thing, a is checked for null (if A is a reference type), and a is cast to IDisposable before calling Dispose() (I presume that is done to get around member hiding and to have it work with explicit interface implementations).
1

Oops, I hadn't noticed that a new instance of Recorder was being created by StartTiming. I've updated the code to account for this. The Wrap function now no longer takes a Recorder parameter but instead passes the recorder it creates as an argument to the action delegate passed in by the caller so that the caller can make use of it if needed.

Hmmm, I've needed to do something very similar to this pattern, lambdas, the Action delegate and closures make it easy:

First define a class to do the wrapping:

public static class RecorderScope
{
   public static void Wrap(Action<Recorder> action)
   {
      Recorder recorder = Recorder.StartTiming();
      try
      {
         action(recorder);
      }
      catch(Exception exception)
      {
         recorder.ReportFailure(exception);
      }
      finally
      {
         recorder.Stop();
      }
   }
}

Now, use like so:

RecorderScope.Wrap(
   (recorder) =>
   {
      // note, the recorder is passed in here so you can use it if needed -
      // if you never need it you can remove it from the Wrap function.
      DoSomeWork();
   });

One question though - is it really desired that the catch handler swallows the exception without rethrowing it? This would usually be a bad practice.

BTW, I'll throw in an addition to this pattern which can be useful. Although, it doesn't sound like it applies to what you're doing in this instance: Ever wanted to do something like the above where you want to wrap some code with a set of startup actions and completion actions but you also need to be able to code some specific exception handling code. Well, if you change the Wrap function to also take an Action delegate and constrain T to Exception, then you've got a wrapper which allows user to specify the exception type to catch, and the code to execute to handle it, e.g.:

public static class RecorderScope
{
   public static void Wrap(Action<Recorder> action, 
      Action<Recorder, T1> exHandler1)
      where T1: Exception
   {
      Recorder recorder = Recorder.StartTiming();
      try
      {
         action(recorder);
      }
      catch(T1 ex1)
      {
         exHandler1(recorder, ex1);
      }
      finally
      {
         recorder.Stop();
      }
   }
}

To use.. (Note you have to specify the type of exception, as it obviously cannot be inferred. Which is what you want):

RecorderScope.Wrap(
   (recorder) =>
   {
      DoSomeWork();
   },
   (recorder, MyException ex) =>
   {
      recorder.ReportFailure(exception);
   });

You can then extend this pattern by providing multiple overloads of the Wrap function which take more than one exception handler delegate. Usually five overloads will be sufficient - it's pretty unusual for you to need to catch more than five different types of exceptions at once.

Comments

0

Don't add another level of indirection. If you need to catch the Exception, use try..catch..finally and call Dispose() in the finally block.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.