Abstract factory design for different cars

Question

Is there anything wrong with this?

    namespace WindowsFormsApplication1
    {
      public interface ICarfactory
      {
        ICar getCar(CarType carType);
      }

      public interface ICar
      {
      }

      public class Sedan: ICar
      {
      }

      public class Hatchback : ICar
      {
      }

      public class SUV : ICar
      {
      }

      public class HondaSUV : SUV
      {
      }

      public class HondaSedan : Sedan
      {
      }

      public class HondaHatchback : Hatchback
      {
      }

      public class HyundaiSUV : SUV
      {
      }

      public class HyundaiSedan : Sedan
      {
      }
      public class HyundaiHatchback : Hatchback
      {
      }

      public class MarutiHatchback : Hatchback
      {
      }

      public class MarutiSUV : SUV
      {
      }

      public class MarutiSedan : Sedan
      {
      }


      public class Hyundai : ICarfactory
      {   
        public ICar getCar(CarType carType)
        {
          switch (carType)
          {
            case CarType.Sedan:
              return new HyundaiSedan();
            case CarType.HatchBack:
              return new HyundaiHatchback();
            case CarType.SUV:
              return new HyundaiSUV();
            default:
              return null;
          }
        }

      }

      public class Maruti : ICarfactory
      {
        public ICar getCar(CarType carType)
        {
          switch (carType)
          {
            case CarType.Sedan:
              return new MarutiSedan();
            case CarType.HatchBack:
              return new MarutiHatchback();
            case CarType.SUV:
              return new MarutiSUV();
            default:
              return null;
          }
        }
      }

      public class Honda : ICarfactory
      {
        public ICar getCar(CarType carType)
        {
          switch (carType)
          {
            case CarType.Sedan:
               return new HondaSedan();
            case CarType.HatchBack:
               return new HondaHatchback();
            case CarType.SUV:
              return new HondaSUV();
            default:
              return null;
          }
        }
      }

      public enum CarType
      {
        Sedan,
        HatchBack,
        SUV
      }
    }

 private void button4_Click(object sender, EventArgs e)
  {      
  ICar myCar;
  ICarfactory carfactory;
  carfactory = new Hyundai();
  myCar = carfactory.getCar(CarType.SUV);
  myCar = carfactory.getCar(CarType.HatchBack);
  myCar = carfactory.getCar(CarType.Sedan);

  carfactory = new Honda();
  myCar = carfactory.getCar(CarType.SUV);
  myCar = carfactory.getCar(CarType.HatchBack);
  myCar = carfactory.getCar(CarType.Sedan);

  carfactory = new Maruti();
  myCar = carfactory.getCar(CarType.SUV);
  myCar = carfactory.getCar(CarType.HatchBack);
  myCar = carfactory.getCar(CarType.Sedan);
  }

Answer 1

First thing that pops in my head is that camelCase for methods (getCar) is not actually usable in c#, in my opinion it looks really ugly and makes it look like java code. You should use PascalCase for all public members:

public interface ICarfactory
{
    ICar GetCar(CarType carType);
}

More info on MSDN.

Answer 2

The factory in itself is nice (except for one thing), but I have one big problem with your code.

Approach:

You are heavily relying on inheritance over composition. Instead of something so complicated as running that against a Factory, where you return types after a switching an enum you should IMO do the following:

public abstract class Car 
{
    private Manufacturer _manufacturer;
    public Manufacturer Manufacturer 
    {
        get 
        { 
            return _manufacturer; 
        }
        set 
        {
            _manufacturer = value; 
        }
    }

    private CarType _carType;
    public CarType CarType
    {
        get
        {
            return _carType;
        }
        //You can even make that stuff read-only by removing the setter
        set
        {
             _carType = value;
        }
     }
}

Implementation:

When you pass an invalid CarType to the Factory you silently return null. That's somewhat unfriendly to the user. Factories are expected to either return a meaningful value or throw an exception. If you allow returning of null, you make the user of the factory do additional null-checks. That is not nice...

Instead you should do the following:

default:
    throw new IllegalArgumentException("CarType not found");

Answer 3

I would take a slightly different approach and consolidate your factory stuff by an extra parameter.

I'd also compose and base class your definition of a car differently. Rather than having base classes for each car type and inheriting, car type would just be another property of a BaseCar abstract class.

Finally, if you end up with a massive catalog of cars, then those switch/case statements are going to get very unwieldy.

You could decide on a naming convention for your concrete types, and drastically simplify your factory get method using some basic reflection/activator code.

void Main()
{
    var hondaHatchBack = CarFactory.GetCar(Manufacturer.Honda, CarType.HatchBack);

    Console.WriteLine(hondaHatchBack);

    var toyotaSedan = CarFactory.GetCar (Manufacturer.Toyota, CarType.Sedan);

    Console.WriteLine(toyotaSedan);

}

public static class CarFactory {
    public static ICar GetCar (Manufacturer m, CarType c)
    {
        var stype = m.ToString() + c.ToString();
        var type = Type.GetType("UserQuery+"+stype); //LinqPad - your namespace will be different
        ICar car = (ICar)System.Activator.CreateInstance(type);
        return car;
    }
}

public interface ICar
{
    Manufacturer Manufacturer {get; }   
    CarType CarType {get; }
}

public abstract class BaseCar : ICar {
    protected BaseCar (Manufacturer _m, CarType _c)
    {
        Manufacturer = _m;
        CarType = _c;
    }
    public Manufacturer Manufacturer {get; private set;}    
    public CarType CarType {get; private set;}

    public override string ToString()
    {
        return "I am a " + Manufacturer + " " + CarType;
    }
}

public enum Manufacturer
{
    Honda,
    Toyota
}

public enum CarType
{
    HatchBack,
    Sedan
}

public class HondaHatchBack : BaseCar { 
    public HondaHatchBack () : base(Manufacturer.Honda, CarType.HatchBack) {}
}

public class HondaSedan : BaseCar{ 
    public HondaSedan () : base(Manufacturer.Honda, CarType.Sedan) {}
}

public class ToyotaHatchBack : BaseCar { 
    public ToyotaHatchBack () : base(Manufacturer.Toyota, CarType.HatchBack) {}
}

public class ToyotaSedan : BaseCar { 
    public ToyotaSedan () : base(Manufacturer.Toyota, CarType.Sedan) {}
}