6
\$\begingroup\$

I have a HTTP client factory that creates a separate HTTP proxy instance for each of my services - which are in the cloud. The factory creates each instance on the basis of an interface which the service implements. All of the proxies inherit from a base class called DalServiceBaseClient which you will see in my code. I don't quite like the way the factory resolves the type to be returned based on the interface.

Here's a simplified version of my code:

Here's how I call my factory to create one of the possible proxies:

var gatewayFactory = new GatewayFactory();
var masterClient = gatewayFactory.CreateMasterClient("someBaseAddress");

Here that same method in GatewayFactory:

public IMasterService CreateMasterClient(string baseAddress)
{
    return (MasterServiceClient)CreateHttpClient<IMasterService>($"{baseAddress}api/master/");
}

And here's the private create method in GatewayFactory where all of the magic happens:

private DalServiceBaseClient CreateHttpClient<T>(string baseAddress)
{
    var httpClient = new HttpClient
    {
        BaseAddress = new Uri(baseAddress)
    };

    if (typeof(T) == typeof(IMasterService))
    {
        return new MasterServiceClient(httpClient);
    }
    if (typeof(T) == typeof(ISlaveService))
    {
        return new SlaveServiceClient(httpClient);
    }
    if (typeof(T) == typeof(IRoleService))
    {
        return new RoleServiceClient(httpClient);
    }

    // A lot more ifs..

    throw new ArgumentException("No type exists for the provided interface");
}

Since there are a lot of different proxies my method ends up having a lot of ifs and I haven't been able to find a more elegant way of doing this.

\$\endgroup\$
  • 1
    \$\begingroup\$ Read up on strategy pattern \$\endgroup\$ – Nkosi Mar 20 '18 at 13:12
  • 3
    \$\begingroup\$ Instead of having a method for this, consider using DI with a DI container. You can then use T in the constructor and "automagically" get the right type. \$\endgroup\$ – Hosch250 Mar 20 '18 at 13:40
  • \$\begingroup\$ In the case you have't seen this: aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong \$\endgroup\$ – Jesse C. Slicer Mar 20 '18 at 16:45
  • \$\begingroup\$ @JesseC.Slicer That's interesting. The code I'm working on is not greenfield though. It's a big migration from WCF to Service Fabric and we aim to change as little public code as possible. With that said, I think I can include a fix for that issue. Thanks. \$\endgroup\$ – Alternatex Mar 20 '18 at 17:35
13
\$\begingroup\$

Create a strategy to find the mapped implementation of the interface provided:

IDictionary<Type, Type> mappings; //Populated with all interface to implementation types

private DalServiceBaseClient CreateHttpClient<T>(string baseAddress) {
    var httpClient = new HttpClient {
        BaseAddress = new Uri(baseAddress)
    };
    var implementationType = mappings[typeof(T)];
    if (implementationType == null)
        throw new ArgumentException("No type exists for the provided interface");

    return (DalServiceBaseClient)Activator.CreateInstance(implementationType, httpClient);
}

Note how reflection is used to create an instance of the implementation.

The mappings would be pre-populated and can look like...

mappings = new Dictionary<Type, Type>();
mappings[typeof(IMasterService)] = typeof(MasterServiceClient);
mappings[typeof(ISlaveService)] = typeof(SlaveServiceClient);

//...other types;

Another option if not too keen on using reflection, is to use a factory delegate which would allow a more strongly typed mapping.

IDictionary<Type, Func<HttpClient, DalServiceBaseClient>> mappings; //Populated with all interface to implementation types

private DalServiceBaseClient CreateHttpClient<T>(string baseAddress) {
    var httpClient = new HttpClient {
        BaseAddress = new Uri(baseAddress)
    };
    var implementationFactory = mappings[typeof(T)];
    if (implementationFactory == null)
        throw new ArgumentException("No type exists for the provided interface");

    return implementationFactory(httpClient);
}

Which would be populated like

mappings = new Dictionary<Type, Func<HttpClient, DalServiceBaseClient>>();
mappings[typeof(IMasterService)] = (httpClient) => new MasterServiceClient(httpClient);
mappings[typeof(ISlaveService)] = (httpClient) => new SlaveServiceClient(httpClient);

//...other types;
\$\endgroup\$
  • \$\begingroup\$ Nice! This is just about as elegant as I can make it. \$\endgroup\$ – Alternatex Mar 20 '18 at 14:15
  • 1
    \$\begingroup\$ or simply use Autofac or any other DI framework :-) \$\endgroup\$ – t3chb0t Mar 20 '18 at 16:30
  • 1
    \$\begingroup\$ @jpmc26 no constructors... then how does you DI work? Via properties? Methods? Well, this does not look like you would be using any DI framework... no wonder you don't like it, I wouldn't either ;-] \$\endgroup\$ – t3chb0t Mar 20 '18 at 20:24
  • 1
    \$\begingroup\$ @jpmc26 let me show you an example where I register several commands and other services in a container that then creates the entire application or another one also initializing the entire application from multiple modules nearly without any news. Now if I need a new dependency anywhere I just add it to the container, to the constructor(s) and it magically works ;-D e.g. ASP.NET-Core would be useless without DI. \$\endgroup\$ – t3chb0t Mar 20 '18 at 20:32
  • 1
    \$\begingroup\$ @t3chb0t I particularly like your ExitCode class. :p \$\endgroup\$ – Ian Kemp Mar 20 '18 at 23:06
3
\$\begingroup\$

Too much indirection

Your public methods, e.g. CreateMasterClient, already tell you which type the caller needs. When you design the shared private method the way you have, you add a lot of indirection that forces you to create convoluted logic just to pass along the information about which type is needed. What you gain is that you centralize the creation of the HttpClient. This is not a good trade off.

Instead, use an alternate means of centralizing the construction of HttpClient: create a factory method for that.

private HttpClient CreateHttpClient(string baseAddress)
{
    return new HttpClient()
    {
        BaseAddress = new Uri(baseAddress)
    };
}

And just construct the actual object directly in the public method:

public IMasterService CreateMasterClient(string baseAddress)
{
    return new MasterServiceClient(
        CreateHttpClient($"{baseAddress}api/master/")
    );
}

You can generalize this to any other common constructor parameters these types have by creating additional methods.

This is vastly easier for another developer to follow. No need for generics. No need for extra casts. No need for long if statements or conditions based on type parameters. If your if block actually contains more logic (like parameters specific to each type or actually choosing a different type for each interface), this also nicely separates out those pieces and makes it clear when they're actually relevant.

\$\endgroup\$
  • \$\begingroup\$ I don't have time right now, but I intend to expand later on whether you even need a factory class here. \$\endgroup\$ – jpmc26 Mar 20 '18 at 20:11
1
\$\begingroup\$

This is a wonderful case for dependency injection. You don't need any ifs, just a properly configured container. You can easily do it with a foreach loop and let the DI framework do the heavy lifting of creating your objects.


Here's a simple example how you could implement it with Autofac.


I need something to work with so I'll use this two interfaces...

interface IMasterServiceClient { }

interface ISlaveServiceClient { }

and their implementations:

class MasterServiceClient : IMasterServiceClient
{
    public MasterServiceClient(HttpClient client)
    {
        Console.WriteLine($"{GetType().Name} initialized with {client.BaseAddress}");
    }
}   

class SlaveServiceClient : ISlaveServiceClient
{
    public SlaveServiceClient(HttpClient client)
    {
        Console.WriteLine($"{GetType().Name} initialized with {client.BaseAddress}");   
    }
}

class ServiceClientUser
{
    public ServiceClientUser(IMasterServiceClient masterServiceClient)
    {
        Console.WriteLine($"{GetType().Name} initialized");
    }
}

The first step is to register all components in the container. In this case you can use tuples to define your interfaces, their implementations and base-addresses. Then you loop over this list and register each component.

At the bottom I also register one of the users that depends on the IMasterServiceClient.

IContainer InitializeContainer()
{
    var builder = new ContainerBuilder();

    var clients = new(Type InterfaceType, Type ImplementationType, string BaseAddress)[]
    {
        (typeof(IMasterServiceClient), typeof(MasterServiceClient), "http://foo/api/master/"),
        (typeof(ISlaveServiceClient), typeof(SlaveServiceClient), "http://bar/api/slave/")
    };

    foreach (var client in clients)
    {
        builder
            .RegisterType(client.ImplementationType)
            .WithParameter(new TypedParameter(typeof(HttpClient), new HttpClient
            {
                BaseAddress = new Uri(client.BaseAddress)
            }))
            .As(client.InterfaceType)
            // You only want to create it once and share the same instance among all components.
            .InstancePerLifetimeScope(); 
    }

    builder
        .RegisterType<ServiceClientUser>();

    return builder.Build();
}

The second and last step is to let Autofac construct all objects for you so that you can simply Resolve it.

void Main()
{
    using (var container = InitializeContainer())
    using (var scope = container.BeginLifetimeScope())
    {
        var user = scope.Resolve<ServiceClientUser>();
    }
}

This will print:

MasterServiceClient initialized with http://foo/
ServiceClientUser initialized

Keep in mind that this is just a simple example. You can do a lot more than that.

\$\endgroup\$
  • \$\begingroup\$ Your demonstration does not append the subpaths that the OP's factory methods does. \$\endgroup\$ – jpmc26 Mar 21 '18 at 15:32
  • \$\begingroup\$ @jpmc26 well, it doesn't have to be because it's just a demo and not a complete implementation; there is no such requirement as having to implement everything. \$\endgroup\$ – t3chb0t Mar 21 '18 at 15:35
  • \$\begingroup\$ Your choice, but that is basically the reason for having the factory methods in the first place. The OP also specifically comments on it elsewhere. It's also not entirely obvious how you would handle this with dependency injection, since you can no longer simply call the constructor. \$\endgroup\$ – jpmc26 Mar 21 '18 at 15:58
  • \$\begingroup\$ @jpmc26 why would you want to call the constructor explicitly? You know all addresses so you initialize the input array and let the framework do the rest. Here you are, adjusted... \$\endgroup\$ – t3chb0t Mar 21 '18 at 16:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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