5
\$\begingroup\$

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);
  }
\$\endgroup\$
2
\$\begingroup\$

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

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

More info on MSDN.

\$\endgroup\$
  • 3
    \$\begingroup\$ Being a Java user, I don't have any other choice than to down-vote :) Although it is correct, I think that's a bad way of phrasing it. I also find that this should not be the accepted answer. \$\endgroup\$ – Simon Forsberg May 30 '14 at 15:15
5
\$\begingroup\$

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");
\$\endgroup\$
5
\$\begingroup\$

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) {}
}
\$\endgroup\$

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.