Web service proxy that switches endpoint URLs in the event of a TimeoutException

Question

I am creating a service (FaultTolerantCommunication) that wraps a call to a (web) service. Its responsibility is to switch the endpoint URL in the event of a TimeoutException.

Currently the web service is a soap web service but I am attempting to make it reusable for other remote services such as a JSON based web service, for example.

I am looking for a review on the design. I have not really done any defensive programming yet in terms of argument validation and null checking but please feel free to suggest improvements on this too.

The idea is for the client code to call the service as below:

I would probably extract an interface from FaultTolerantCommunication and inject an instance using an IoC container.

Client Code:

var faultTolerantCommunication = new FaultTolerantCommunication<FooRequest, FooResponse, ISoapClient>(this.serviceCaller);
FooResponse response = faultTolerantCommunication.Request(request, this.soapClient);

Fault Tolerant Communication

    /// <summary>
    /// Fault Tolerant Communication. Recurses to fall over web service endpoints in event of timeout.
    /// </summary>
    public class FaultTolerantCommunication<TRequest, TResponse, TClient> 
        where TRequest : FooRequest, new()
        where TResponse : FooResponse, new()
        where TClient : ISoapClient
    {
        private readonly Configuration config;
        private readonly FooIntegration FooIntegration;
        private readonly ExceptionLogger exceptionLogger;
        private TClient client;
        private readonly SoapClientSwitcher<TClient> switcher;

        public FaultTolerantCommunication(FooIntegration FooIntegration)
        {
            // Read the config during construction.
            config = new ConfigReader().Read();

            // Constructor inject FooIntegration
            this.FooIntegration = FooIntegration;

            // Exception Logger
            this.exceptionLogger = new ExceptionLogger("FaultTolerantCommunication");

            // Generic SoapClient
            this.client = default(TClient);

            // Soap Client Switcher
            switcher = new SoapClientSwitcher<TClient>(config);
        }

        /// <summary>
        /// Do a single request.
        /// </summary>
        /// <param name="request">The Request</param>
        /// <param name="client">The Client</param>
        /// <returns>The Response</returns>
        private TResponse DoRequest(TRequest request, TClient client)
        {
            // Set the endpoint soap client.
            FooIntegration.ChangeEndPoint(client);

            // Call the service
            FooResponse response = this.FooIntegration.DoTransaction(request);

            // Return the response
            return (TResponse)response;
        }

        /// <summary>
        /// Recursively fall back to secondary and stand-in endpoints on TimeoutException
        /// </summary>
        /// <param name="request">The Request</param>
        /// <param name="client">The Client</param>
        /// <returns>The Response</returns>
        public TResponse Request(TRequest request, TClient client)
        {
            try
            {
                return DoRequest(request, client);
            }
            catch (TimeoutException timeoutException)
            {
                // Log the time out
                exceptionLogger.DbLogException(timeoutException);

                // Change endpoint
                this.client = switcher.Switch(this.client);
                FooIntegration.ChangeEndPoint(client);

                // Recurse
                return Request(request, client);
            }
        }
    }

Soap Client Switcher

internal class SoapClientSwitcher<TClient> where TClient : ISoapClient
{
    private readonly Configuration config;

    internal SoapClientSwitcher(Configuration config)
    {
        this.config = config;
    }

    internal TClient Switch(TClient client)
    {
        if (client is SoapClient)
        {
            client = (TClient)new SoapClientFactory().GetSecondarySoapClient(config);
            return client;
        }

        if (client is SecondarySoapClient)
        {
            client = (TClient)new SoapClientFactory().GetStandInSoapClient(config);
            return client;
        }

        if (client is StandInSoapClient)
        {
            client = (TClient)new SoapClientFactory().GetPrimarySoapClient(config);
            return client;
        }

        return client;
    }
}

ISoapClient

public interface ISoapClient : IClient<string, string>
{
    string EndPointUrl { get; set; }
}

IClient

public interface IClient<TRequest, TResponse>
{
    TResponse Transaction(TRequest transaction);
}

Soap Client Factory

public class SoapClientFactory
{
    private ISoapClient soapCient;

    public ISoapClient GetPrimarySoapClient(Configuration configuration)
    {
        soapCient = new SoapClient(configuration);
        return soapCient;
    }

    public ISoapClient GetSecondarySoapClient(Configuration configuration)
    {
        soapCient = new SecondarySoapClient(configuration);
        return soapCient;
    }

    public ISoapClient GetStandInSoapClient(Configuration configuration)
    {
        soapCient = new StandInSoapClient(configuration);
        return soapCient;
    }
}

Soap Client

public class SoapClient : ISoapClient
{
    private wsSoapClient client;
    private string password;

    public SoapClient(Configuration configuration)
    {
        // Some Config reading which is omitted for brevity

        EndpointAddress address = SetEndPoint(settings);
        client = new wsSoapClient(binding, address);
    }

    protected virtual EndpointAddress SetEndPoint(AppSettingsSection settings)
    {
        EndpointAddress address = new EndpointAddress(settings.Settings["EndpointUrl"].Value);
        return address;
    }

    public string Transaction(string transaction)
    {
        Console.WriteLine("Calling real soapClient");
        return client.Transaction(transaction, password);
    }

    public string EndPointUrl { get; set; }
}

Answer 1

1. For this:

   public TResponse Request(TRequest request, TClient client)
   {
       try
       {
           return DoRequest(request, client);
       }
       catch (TimeoutException timeoutException)
       {
           // Log the time out
           exceptionLogger.DbLogException(timeoutException);
   
           // Change endpoint
           this.client = switcher.Switch(this.client);
           FooIntegration.ChangeEndPoint(client);
   
           // Recurse
           return Request(request, client);
       }
   }
   

   If I'm right

   return Request(request, client);
   

   should be

   return Request(request, this.client);
   

   Are you sure that the Request method needs the TClient parameter at all? The class has a client field but it is never used. I guess you should use the field here and remove the method parameter.

   Furthermore, FooIntegration.ChangeEndPoint(client) looks unnecessary here, since the called Request method also calls FooIntegration.ChangeEndPoint(client).

2. If you're planning to unit-test your code, dependencies, like new ConfigReader() and new ExceptionLogger() will make it harder because you can't use mocked/dummy implementations with predefined values/behaviour. I'd have a Configuration and an ExceptionLogger etc. constructor parameter instead and create these objects in a factory method.

3. This method actually doesn't switch anything:

   internal TClient Switch(TClient client)
   

   It just returns the next client. The client parameter could be called currentClient while the return value is the nextClient. I would name the method getNextClient.

4. FaultTolerantCommunication does not seem thread-safe. (If you are planning to use it from multiple threads you should handle that.)

5. client = (TClient)new SoapClientFactory().GetSecondarySoapClient(config);
   return client;
   

   It's unnecessary to change the value of this local variable right before the function returns. The following is the same:

   return (TClient) new SoapClientFactory().GetSecondarySoapClient(config);
   

   (You might don't need the casting too.)

6. Comments like this are rather noise:

   // Read the config during construction.
   config = new ConfigReader().Read();
   
   // Constructor inject FooIntegration
   this.FooIntegration = FooIntegration;
   
   // Exception Logger
   this.exceptionLogger = new ExceptionLogger("FaultTolerantCommunication");
   

   What they say is obvious from the code itself. I'd remove them. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

7. Usage of this. doesn't seem consistent:

   config = new ConfigReader().Read();
   this.FooIntegration = FooIntegration;
   this.exceptionLogger = new ExceptionLogger("FaultTolerantCommunication");
   this.client = default(TClient);
   switcher = new SoapClientSwitcher<TClient>(config);
   

   If not necessary I'd remove them. I'm not familiar with C# IDEs but if they can show fields and local variables with different colors this. is usually unnecessary.

8. I'd use lowercase first letter for the field name here:

   private readonly FooIntegration FooIntegration;
   

   It would be consistent with the other fields and wouldn't be the same as the type.

9. The soapClient field is never read:

   public class SoapClientFactory
   {
       private ISoapClient soapCient;
   
       public ISoapClient GetPrimarySoapClient(Configuration configuration)
       {
           soapCient = new SoapClient(configuration);
           return soapCient;
       }
       ...
   }
   

   You could use a local variable or just return:

   public ISoapClient GetPrimarySoapClient(Configuration configuration)
   {
       return new SoapClient(configuration);
   }