CODEWITHSUNDEEP
c#refactoring
SGarratt
SGarratt

Reputation: 1004

How to refactor code with try-catch-finally

I have to create a bunch of methods that look like this. The things that change will be the method name, the return type and the lines marked in the middle - the rest will be the same. Is there a clean way to refactor this so that I don't repeat myself?

private bool CanPerform(WindowsIdentity identity, string applicationName, int operation)
{
    IAzApplication3 application = null;
    IAzClientContext3 context = null;
    try
    {
        application = this.store.OpenApplication(applicationName, null) as IAzApplication3;

        ulong token = (ulong)identity.Token.ToInt64();
        context = application.InitializeClientContextFromToken(token, null) as IAzClientContext3;

        // lines that change go here
    }
    catch (COMException e)
    {
        throw new SecurityException(string.Format("Unable to check operation '{0}'", operation), e);
    }
    finally
    {
        Marshal.FinalReleaseComObject(context);
        Marshal.FinalReleaseComObject(application);
    }
}

I realise this is probably basic stuff but I work alone so there's no one else to ask.

Upvotes: 2

Views: 643

Answers (2)

Jon Skeet
Jon Skeet

Reputation: 1502985

It sounds like a delegate would be appropriate here, with a generic method to cover the return type changing:

private T ExecuteWithIdentity<T>(WindowsIdentity identity,
    string applicationName, int operation,
    Func<IAzApplication3, IAzClientContext3, T> action)
{
    IAzApplication3 application = null;
    IAzClientContext3 context = null;
    try
    {
        application = this.store.OpenApplication(applicationName, null) as IAzApplication3;

        ulong token = (ulong)identity.Token.ToInt64();
        context = application.InitializeClientContextFromToken(token, null) as IAzClientContext3;

        return action(application, context);
    }
    catch (COMException e)
    {
        throw new SecurityException(
            string.Format("Unable to check operation '{0}'", operation), e);
    }
    finally
    {
        Marshal.FinalReleaseComObject(context);
        Marshal.FinalReleaseComObject(application);
    }
}

Then you put the code for each check in a separate method, or even just use a lambda expression:

bool check = ExecuteWithIdentity(identity, "Foo", 10,
                         (application, context) => context != null);

or

string check = ExecuteWithIdentity(identity, "Foo", 10, SomeComplexAction);

...
private static string SomeComplexAction(IAzApplication3 application,
                                        IAzClientContext3 context)
{
    // Do complex checks here, returning whether the user is allowed to
    // perform the operation
}

You may want to change the delegate type of course - it's not clear what operation is meant to be used for, for example.

I would also strongly consider casting instead of using as. If the application or context is returned from OpenApplication/InitializeClientContextFromTokenas a non-null value which just isn't the right type, do you really want to handle that the same was as a null value being returned?

Upvotes: 6

user1105802
user1105802

Reputation:

You could do your error handling slightly higher up the stack, so rather than catching and rethrowing the exception inside the method you could do it where the method is called?

If your method calls are all wrapped in a Manager class that might save a bit of time. If they're just ad-hoc called everywhere then naturally maybe not :)

I hope that might help.

Upvotes: 1

Related Questions