6
\$\begingroup\$

This is some code that I found in a project I work on. Please tell me that I am crazy and that this code is good. Please do.

public class ExceptionHandlingHelper
{
    static IExeptionHandlerProvider _exeptionHandlerProvider = null;
    static IExeptionHandlerProvider ExeptionHandlerProvider
    {
        get
        {
            if (_exeptionHandlerProvider == null)
            {
                Assembly clientLib = Assembly.Load("Bla.BlaDiBla");
                Type type = clientLib.GetType("Bla.BlaDiBla.Controls.ExeptionHandlerProvider");
                _exeptionHandlerProvider = (IExeptionHandlerProvider)Activator.CreateInstance(type);
            }
            return _exeptionHandlerProvider;
        }
    }

    public static void HandleException(FaultException<DefaultFaultContract> fault)
    {
        var message = new StringBuilder();
        var detail = new StringBuilder();

        ExeptionHandlerProvider.GetMessage(fault, message, detail);

        if (!string.IsNullOrWhiteSpace(fault.Detail.FullStack))
        {
            detail.AppendLine();
            detail.AppendLine(fault.Detail.FullStack);
        }

        if (!string.IsNullOrWhiteSpace(fault.Detail.InnerExceptionStack))
        {
            detail.AppendLine();
            detail.AppendLine("Inner Exception Stack");
            detail.AppendLine(fault.Detail.InnerExceptionMessage);
            detail.AppendLine(fault.Detail.InnerExceptionSource);
            detail.AppendLine(fault.Detail.InnerExceptionStack);
        }

        Handle(message.ToString(), detail.ToString());
    }

    ...
}

Used like this:

        string serverName = GetServerName();
        using (var factory = new ChannelFactory<T>(GetBinding(TransportType), GetEndPointAddressForClient(typeof(T), TransportType, serverName)))
        {
            var client = GetChannel(factory);
            try
            {
                using (var op = new OperationContextScope(client as IContextChannel))
                {
                    OperationContext.Current.OutgoingMessageHeaders.Add(GetCachedHeader());
                    action(client);
                    ((IClientChannel)client).Close();
                }
            }
            catch (FaultException<DefaultFaultContract> ex)
            {
                ((IClientChannel)client).Abort();
                factory.Abort();
                ExceptionHandlingHelper.HandleException(ex);
            }
            catch (FaultException ex)
            {
                ((IClientChannel)client).Abort();
                factory.Abort();
                ExceptionHandlingHelper.HandleException(ex);
            }
            catch (Exception ex)
            {
                ((IClientChannel)client).Abort();
                factory.Abort();
                ExceptionHandlingHelper.HandleException(ex);
            }
        }
\$\endgroup\$
7
  • 3
    \$\begingroup\$ You're crazy and the code is good? \$\endgroup\$
    – Vogel612
    Jul 31, 2017 at 12:18
  • \$\begingroup\$ @Phoenix tbh, I don't see any problems with what you have provided \$\endgroup\$
    – MattMatt
    Jul 31, 2017 at 13:21
  • \$\begingroup\$ But a bit more explanation what this code actually does (e.g. a few words about the usage, not just the code), would go a long way. \$\endgroup\$
    – Graipher
    Jul 31, 2017 at 15:14
  • \$\begingroup\$ this doesn't seem thread safe, i would test it with multi thread. I'm expecting errors / issues \$\endgroup\$
    – Fredou
    Jul 31, 2017 at 16:36
  • 1
    \$\begingroup\$ I understand, @t3chb0t, and I apologize. I believe the answers I received are enough, so I won't edit the question, but I will promise to explain code better next time I post. \$\endgroup\$
    – Phoenix
    Jul 31, 2017 at 19:17

2 Answers 2

2
\$\begingroup\$

You're crazy and the code is terrible :-P

catch (FaultException<DefaultFaultContract> ex)
{
    ((IClientChannel)client).Abort();
    factory.Abort();
    ExceptionHandlingHelper.HandleException(ex);
}
catch (FaultException ex)
{
    ((IClientChannel)client).Abort();
    factory.Abort();
    ExceptionHandlingHelper.HandleException(ex);
}
catch (Exception ex)
{
    ((IClientChannel)client).Abort();
    factory.Abort();
    ExceptionHandlingHelper.HandleException(ex);
}

Why are do doing the same thing three times? The last handler is all you need.

var client = GetChannel(factory);
try
{
    using (var op = new OperationContextScope(client as IContextChannel))
    {
        OperationContext.Current.OutgoingMessageHeaders.Add(GetCachedHeader());
        action(client);
        ((IClientChannel)client).Close();
    }
}

You also don't need the ((IClientChannel)client) casting all over the place. Do it once:

var client = (IClientChannel)GetChannel(factory);


Handle(message.ToString(), detail.ToString());

So your exception-handler is actualy a logger but you thought why give it a good name if you can give it a name the has nothing to do with the actual code ;-]

\$\endgroup\$
3
\$\begingroup\$

One thing which I noticed is this little spelling error in the code

static IExeptionHandlerProvider _exeptionHandlerProvider = null;  

Both the name of the interface and the name of the field should be fixed by adding a c -> IExceptionHandlerProvider _exceptionHandlerProvider = null;


public static void HandleException(FaultException<DefaultFaultContract> fault)

You should initialze the StringBuilder's with a starting capacity, because the default capacity of a StringBuilder is 16 characters, and if it is reached the capacity will be doubled which involves copying the current buffer to the newly created buffer.

By choosing the initial capacity to be e.g 256 you will maybe waste some memory but you avoid the internal increasing of the buffer.

\$\endgroup\$

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