Implementing a generic factory method

Question

I've implemented a Vehicle service that is responsible for servicing vehicles such as cars and trucks:

public interface IVehicleService
{
    void ServiceVehicle(Vehicle vehicle);   
}

public class CarService : IVehicleService
{
    void ServiceVehicle(Vehicle vehicle)
    {
        if (!(vehicle is Car))
            throw new Exception("This service only services cars")

       //logic to service the car goes here
    }
}

I also have a vehicle service factory that is responsible for creating a vehicle service according to the type of vehicle passed in to the factory method:

public class VehicleServiceFactory 
{
    public IVehicleService GetVehicleService(Vehicle vehicle)
    {
        if (vehicle is Car)
        {
            return new CarService();
        }

        if (vehicle is Truck)
        {
            return new TruckService();
        }

        throw new NotSupportedException("Vehicle not supported");
    }
}

The issue I had was with the CarService.ServiceVehicle method. It's accepting a Vehicle when ideally it should accept a Car, as it knows that it will only service cars. So I decided to update this implementation to use generics instead:

public interface IVehicleService<T> where T : Vehicle
{
    void ServiceVehicle(T vehicle); 
}

public class CarService : IVehicleService<Car>
{
    void ServiceVehicle(Car vehicle)
    {
        //this is better as we no longer need to check if vehicle is a car

        //logic to service the car goes here 
    }
}

public class VehicleServiceFactory 
{
    public IVehicleService<T> GetVehicleService<T>(T vehicle) where T : Vehicle
    {
        if (vehicle is Car)
        {
            return new CarService() as IVehicleService<T>;
        }

        if (vehicle is Truck)
        {
            return new TruckService() as IVehicleService<T>;
        }

        throw new NotSupportedException("Vehicle not supported");
    }
}

The issue I currently have is when calling this factory as follows:

var factory = new VehicleServiceFactory();
Vehicle vehicle = GetVehicle();
var vehicleService = factory.GetVehicleService(vehicle);  // this returns null!
vehicleService.ServiceVehicle(vehicle);

GetVehicleService returns null, I guess because I'm passing in the base type Vehicle into this method, so T would evaluate to Vehicle and it isn't possible to cast from CarService (which implements IVehicleService<Car>) to the effective return type which would be IVehicleService<Vehicle> (please correct me if I'm wrong).

Would appreciate some guidance on how to resolve this.

@oerkelens It's null because `T` is `Vechicle` so the as cast to `IVehicleService` fails and returns `null`. — juharr, Oct 30 '17 at 17:17
Why do you cast to IVehicleService? What happens if you remove the `as IVehcileService` from your return statements? @juharr So I noticed afterwards, hence I removed my comment. Question remains, what does `GetVehicle()` return? — oerkelens, Oct 30 '17 at 17:17
The problem now is that you don't have a common type that both `CarService` and `TruckService` can be cast to, and for good reason. Simply put a `IVehicleService` is not a `IVehicleService` — juharr, Oct 30 '17 at 17:21
@oerkelens Doesn't matter since it's either `Vehicle` or it's being implicitly cast to `Vehicle`. — juharr, Oct 30 '17 at 17:22
@Dale yes it is. It can return any derived class instance such as a `Car` or a `Truck` — user8851548, Oct 30 '17 at 17:50
I've been told that one possible approach to resolve this is to inherit IVehicleService from a non-generic version but not sure how to implement this approach. — user8851548, Oct 30 '17 at 17:51
Ugh, I am getting a serious case of SRP headache here! You're trying to cram too much responsibility into a single, poor factory class. In reality, you should have a facade factory class which serves as a traffic controller to individual factories which will serve up the specific class that you need. (... sighs deeply, then takes an Advil). — code4life, Oct 30 '17 at 19:17
To insert a common complaint from when I do code reviews: you should never be using `as` without checking the result. Doing this leads to mysterious null reference exceptions, just like this. If you mean to say "this thing really is of this type", then just cast it - at least then when it fails you get an invalid cast exception exactly where it occurs rather than a hard to debug null reference exception at a different location. — Philip Kendall, Oct 30 '17 at 20:55

score 2 · Answer 1 · answered Oct 30 '17 at 18:29

There's a way you can avoid having to use Generics on IVehicleService and Avoid the problem of passing a Truck into the CarService and vise versa. You can first change IVehicleService to not be generic or pass a vechicle in:

public interface IVehicleService
{
    void ServiceVehicle();
}

instead we pass the vehicle into the constructor of the CarService/TruckService:

    public class CarService : IVehicleService
    {
        private readonly Car _car;

        public CarService(Car car)
        {
            _car = car;
        }

        public void ServiceVehicle()
        {
            Console.WriteLine($"Service Car {_car.Id}");
        }
    }

And have the factory pass the vehicle in:

    public class VehicleServiceFactory
    {
        public IVehicleService GetVehicleService(Vehicle vehicle)
        {
            if (vehicle is Car)
            {
                return new CarService((Car)vehicle);
            }

            if (vehicle is Truck)
            {
                return new TruckService((Truck)vehicle);
            }

            throw new NotSupportedException("Vehicle not supported");
        }
    }

This is the way I would implement this

    public static void Main(string[] args)
    {
        var factory = new VehicleServiceFactory();
        Vehicle vehicle = GetVehicle();
        var vehicleService = factory.GetVehicleService(vehicle);
        vehicleService.ServiceVehicle();

        Console.ReadLine();

    }

    public static Vehicle GetVehicle()
    {
        return new Truck() {Id=1};

        //return new Car() { Id = 2 }; ;
    }


    public interface IVehicleService
    {
        void ServiceVehicle();
    }

    public class CarService : IVehicleService
    {
        private readonly Car _car;

        public CarService(Car car)
        {
            _car = car;
        }

        public void ServiceVehicle()
        {
            Console.WriteLine($"Service Car {_car.Id}");
        }
    }

    public class TruckService : IVehicleService
    {
        private readonly Truck _truck;

        public TruckService(Truck truck)
        {
            _truck = truck;
        }

        public void ServiceVehicle()
        {
            Console.WriteLine($"Service Truck {_truck.Id}");
        }
    }

    public class VehicleServiceFactory
    {
        public IVehicleService GetVehicleService(Vehicle vehicle)
        {
            if (vehicle is Car)
            {
                return new CarService((Car)vehicle);
            }

            if (vehicle is Truck)
            {
                return new TruckService((Truck)vehicle);
            }

            throw new NotSupportedException("Vehicle not supported");
        }
    }


    public abstract class Vehicle
    {
        public int Id;

    }

    public class Car : Vehicle
    {

    }

    public class Truck : Vehicle
    {

    }

Thanks, really nice simple solution. I've accepted Timo's answer only because it's closer to my current implementation. — user8851548, Oct 30 '17 at 19:42

John Wu · Answer 2 · 2017-10-30T19:54:17.420

You're asking for compile-time type safety. Yet you are using code where the type is not known at compile time. In this example....

var factory = new VehicleServiceFactory();
Vehicle vehicle = GetVehicle();  //Could return any kind of vehicle
var vehicleService = factory.GetVehicleService(vehicle);  
vehicleService.ServiceVehicle(vehicle);

...the type of vehicle is simply not known when you compile the code.

Even if you could pull it off, you couldn't do anything with the class that was returned, because again, you don't know the type at compile time:

CarService s = new CarSevice();
Vehicle v = new Car();
s.ServiceVehicle(v); //Compilation error

If you want compile-time checking, you need to declare the type at compile time. So just change it to this:

var factory = new VehicleServiceFactory();
Car vehicle = GetCar();  //<-- specific type
var vehicleService = factory.GetVehicleService(vehicle);  
vehicleService.ServiceVehicle(vehicle);

Or if you insist on holding the vehicle with a variable of type Vehicle, you could use

var factory = new VehicleServiceFactory();
Vehicle vehicle = GetCar();  
var vehicleService = factory.GetVehicleService<Car>(vehicle);   //Explicit type
vehicleService.ServiceVehicle(vehicle);

And the factory will return the appropriate service class.

Either that, or stick with runtime checking, which is implemented in your first example.

Timo · Accepted Answer · 2017-10-30T21:34:45.187

The problem

The problem you face has to do with the generic type C# deduces.

Vehicle vehicle = GetVehicle();

This line causes you trouble, because the vehicle variable type you pass in

var vehicleService = factory.GetVehicleService(vehicle);  // this returns null!

is of type Vehicle and not of type Car (or Truck). Therefore the type that your factory method GetVehicleService<T> deduces (T), is Vehicle. However, in your GetVehicleService method, you do a safe cast (as) which returns null, if the given type cannot be cast as you wished. If you change it to direct cast

return (IVehicleService<T>) new CarService();

you will see, that the debugger will catch an InvalidCastException at this line. This is because your CarService implements IVehicleService<Car> but the program actually tries to cast it to IVehicleService<Vehicle> which is not implemented by your CarService and therefore throws the exception.

If you remove the cast at all to

return new CarService();

you will even get an error at compile time, telling you that those types cannot be cast to each other.

A solution

Unfortunately, I'm not aware of a neat solution that can be handled by C#. However, you could create an abstract base class for your services, implementing a non-generic interface:

public interface IVehicleService
{
    void ServiceVehicle(Vehicle vehicle);
}

public abstract class VehicleService<T> : IVehicleService where T : Vehicle
{
    public void ServiceVehicle(Vehicle vehicle)
    {
        if (vehicle is T actual)
            ServiceVehicle(actual);
        else
            throw new InvalidEnumArgumentException("Wrong type");
    }

    public abstract void ServiceVehicle(T vehicle);
}

public class CarService : VehicleService<Car>
{
    public override void ServiceVehicle(Car vehicle)
    {
        Console.WriteLine("Service Car");
    }
}

public class TruckService : VehicleService<Truck>
{
    public override void ServiceVehicle(Truck vehicle)
    {
        Console.WriteLine("Service Truck");
    }
}

public class VehicleServiceFactory
{
    public IVehicleService GetVehicleService(Vehicle vehicle)
    {
        if (vehicle is Car)
        {
            return new CarService();
        }

        if (vehicle is Truck)
        {
            return new TruckService();
        }

        throw new NotSupportedException("Vehicle not supported");
    }
}

As you can see, the factory is now non-generic, aswell as the interface (like you had it before). The abstratc base class for the services, however, can now handle the types and throw an exception (unfortunately only at runtime) if the types does not match.

A (maybe) helpful addition

If your factory has a lot of different types and you want to save dozens of if statements, you could do a little workaround with attributes.

First, create a ServiceAttribute class:

[AttributeUsage(AttributeTargets.Class)]
public class ServiceAttribute : Attribute
{
    public Type Service { get; }

    public ServiceAttribute(Type service)
    {
        Service = service;
    }
}

Then attach this attribute to your vehicle classes:

[Service(typeof(TruckService))]
public class Truck : Vehicle
// ...

And change your factory like this:

public class VehicleServiceFactory
{
    public IVehicleService GetVehicleService(Vehicle vehicle)
    {
        var attributes = vehicle.GetType().GetCustomAttributes(typeof(ServiceAttribute), false);

        if (attributes.Length == 0)
            throw new NotSupportedException("Vehicle not supported");

        return (IVehicleService) Activator.CreateInstance(((ServiceAttribute)attributes[0]).Service);
    }
}

~~This methology doesn't use reflection and therefore shouldn't be that slow compared to the if statements.~~

Thanks, really nice solution. Marking as the accepted answer. — user8851548, Oct 30 '17 at 19:41
You say the last option isn't using reflection, but you are! — DavidG, Oct 30 '17 at 20:40
@DavidG it actually isn't. Since .Net 4.7 you don't need reflection for my sample. You can try to compile it without using the System.Reflection namespace and it will still compile. — Timo, Oct 30 '17 at 20:50
Any time you do `GetType()` you are using reflection. You don't need the namespace for that. — DavidG, Oct 30 '17 at 20:53
@DavidG OK you may be right about that. But didn't MS remove the heavy stuff from System.Type to extension methods located in System.Reflection? So that low performance cost functions can be accessed outside the Reflection namespace? — Timo, Oct 30 '17 at 21:02
@DavidG Idk. I know there used to be a different handling of reflection in .Net Core but there are differences between pre .Net 4.7 and post 4.7 in terms of reflrection. I thought they adopted it from .Net Core. — Timo, Oct 30 '17 at 21:31
There's no way they would change an existing interface in the existing framework, it would break to many apps. — DavidG, Oct 30 '17 at 21:58

codebender · Answer 4 · 2017-10-30T17:57:09.650

1

I'd use something like the following within the Factory class:

public T GetVehicle<T>(T it) {
    try {
        Type type = it.GetType();                                   // read  incoming Vehicle type
        ConstructorInfo ctor = type.GetConstructor(new[] { type }); // get its constructor
        object instance = ctor.Invoke(new object[] { it });         // invoke its constructor
        return (T)instance;                                         // return proper vehicle type
    } catch { return default(T); }
}

edited Oct 30 '17 at 17:57

answered Oct 30 '17 at 17:41

codebender

469
1
6
23

Thanks for the suggestion, but I'd prefer not to use reflection if possible. – user8851548 Oct 30 '17 at 18:08
Is the concern about speed? https://stackoverflow.com/questions/3377576/if-reflection-is-inefficient-when-is-it-most-appropriate – codebender Oct 30 '17 at 19:15

score 0 · Answer 5 · edited Oct 30 '17 at 18:40

For factory implementation you can use MEF

This will allow you to implement with Export and Import attributes with unique names and you will not need a if else / witch statements to create the factory.

class Program
{
    private static CompositionContainer _container;

    public Program()
    {

        var aggList = AppDomain.CurrentDomain
                               .GetAssemblies()
                               .Select(asm => new AssemblyCatalog(asm))
                               .Cast<ComposablePartCatalog>()
                               .ToArray();

        var catalog = new AggregateCatalog(aggList);

        _container = new CompositionContainer(catalog);
        _container.ComposeParts(this);
    }

    static void Main(string[] args)
    {
        var prg = new Program();

        var car = _container.GetExportedValue<IVehicle>("CAR") as Car; 
        var carService = _container.GetExportedValue<IVehicleService<Car>>("CARSERVICE") as CarService;
        carService.ServiceVehicle(car);

        var truck = _container.GetExportedValue<IVehicle>("TRUCK") as Truck;
        var truckService = _container.GetExportedValue<IVehicleService<Truck>>("TRUCKSERVICE") as TruckService;
        truckService.ServiceVehicle(truck);

        Console.ReadLine();
    }
}

public interface IVehicleService<in T> 
{
    void ServiceVehicle(T vehicle);
}

public interface IVehicle
{
}

[Export("CARSERVICE", typeof(IVehicleService<Car>)), PartCreationPolicy(System.ComponentModel.Composition.CreationPolicy.NonShared)]
public class CarService : IVehicleService<Car>
{
    public void ServiceVehicle(Car vehicle)
    {
    }
}

[Export("TRUCKSERVICE", typeof(IVehicleService<Truck>)), PartCreationPolicy(System.ComponentModel.Composition.CreationPolicy.NonShared)]
public class TruckService : IVehicleService<Truck>
{
    public void ServiceVehicle(Truck vehicle)
    {

    }
}

public abstract class Vehicle : IVehicle
{

}

[Export("CAR", typeof(IVehicle)), PartCreationPolicy(System.ComponentModel.Composition.CreationPolicy.NonShared)]
public class Car : Vehicle
{

}

[Export("TRUCK", typeof(IVehicle)), PartCreationPolicy(System.ComponentModel.Composition.CreationPolicy.NonShared)]
public class Truck : Vehicle
{

}

`MEF` slows instantation drastically. – FCin Oct 30 '17 at 17:48 — FCin, Oct 30 '17 at 17:48

Implementing a generic factory method

5 Answers5

The problem

A solution

A (maybe) helpful addition