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

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(this.serviceCaller);
FooResponse response = faultTolerantCommunication.Request(request, this.soapClient);

Fault Tolerant Communication

```
///
/// Fault Tolerant Communication. Recurses to fall over web service endpoints in event of timeout.
///
public class FaultTolerantCommunication
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 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 Cl

-
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).

-
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.

-
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.

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

-

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.)

-
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)

-
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(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.

-
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.

-
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);
}

StackExchange Code Review Q#43292, answer score: 5