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.

Answer 1

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.

Answer 2

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
    {

    }

Answer 3

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.