Building car factory with custom features per car

Question

I was once asked in an interview to build a factory that makes cars. All cars has common features like price, rating and color and they have some features like fuel injection which can on specific car but not mandatory. Moreover there can be several types of the same feature like fuel-injection v1, v2 ect, and you should be able to compare these features. Features should be added with ease. (Meaning new feature)

I've also added a general car compare.

I had ~30 min, I didn't have time to finish my idea, but what I had in mind is doing a property of a list of features for my base car type.

I didn't pass and I would like to know two things:

1. Is there something wrong with the approach of List of Feature for each car?
2. Is there something I could do better with my factory design?

This is my code, I have a general CarFactory instead of manufacture based and all models in one file for the simplicity of showing the code:

BL:

Factory.cs

using System;
using System.Collections.Generic;
using System.Linq;

namespace BL
{
    public class Car
    {
        public readonly CarTypes Model;
        public readonly CarManufactures Manufacture;
        public readonly string Color;

        private float price;
        public float Price
        {
            get { return price; }
        }
        internal Car(CarTypes model, CarManufactures manufacture, string color, float price)
        {
            this.Model = model;
            this.Manufacture = manufacture;
            this.Color = color;
            this.price = price;
            this._features = new List<Feature>();
        }
        internal void AddFeature(Feature feature)
        {
            this._features.Add(feature);
            this.price += feature.Install();
        }

        internal List<Feature> _features;
        public List<Feature> Features
        {
            get
            {
                return _features;
            }
        }

        public bool HasFeature(Feature feature)
        {
            return this.Features.Any(m => m.GetType() == feature.GetType());
        }

        public Feature GetFeature(Feature feature)
        {
            return this.Features.FirstOrDefault(m => m.GetType() == feature.GetType());
        }

        public override string ToString()
        {
            return string.Format("{0} {1} {2}", this.Color, this.Model, this.Manufacture);
        }
    }

    public static class CarFactory
    {
        public static Car CreateCar(CarTypes model, CarManufactures manufacture, string color)
        {
            Car newCar = null;
            switch (manufacture)
            {
                case CarManufactures.Couppe:
                    newCar = new Car(model, manufacture, color, 30000);
                    newCar.AddFeature(CruiseControl.GCruise);
                    break;

                case CarManufactures.Mini:
                    newCar = new Car(model, manufacture, color, 40000);
                    newCar.AddFeature(CruiseControl.XCruise);
                    break;

                case CarManufactures.Swift:
                    newCar = new Car(model, manufacture, color, 20000);
                    break;

            }

            switch (model)
            {
                case CarTypes.FamaliyCar:
                    newCar.AddFeature(FuelInjection.FuelJet);
                    break;
                case CarTypes.SportCar:
                    newCar.AddFeature(FuelInjection.Ninjet);
                    break;
            }

            Console.WriteLine("Car was created:\n Model: {0}\nManufacture: {1}\nHas {2} features\nPrice:{3}\n*************************\n\n", newCar.Model, newCar.Manufacture, newCar.Features.Count, newCar.Price);

            return newCar;
        }
    }

    public enum CarManufactures
    {
        Swift,
        Couppe,
        Mini
    }

    public enum CarTypes
    {
        SportCar,
        FamaliyCar
    }

    public static class Extensions
    {

        public static List<Car> CompareCars(this List<Car> cars, bool sort= false)
        {
            Console.WriteLine("{0,-15} {1,-15} {2,-7} {3,-10}", "Manufacture", "Model", "Rating", "Price");
            Console.WriteLine("{0,-15} {1,-15} {2,-7} {3,-10}", "-----------", "-----", "------", "-----");
            if (sort)
                cars = cars.OrderByDescending(m => m.GetRating()).ToList();
            foreach (Car car in cars)
            {
                Console.WriteLine("{0,-15} {1,-15} *{2,-4} {3,-10}", car.Manufacture, car.Model, car.GetRating(), car.Price);
            }
            return cars;
        }
        public static int GetRating(this Car car)
        {
            int rating = GetBaseRating(car.Manufacture);
            rating += car.Features.Sum(m => m.Rating);
            return rating;
        }

        private static int GetBaseRating(CarManufactures manufacture)
        {
            switch (manufacture)
            {
                case CarManufactures.Swift:
                    return 30;
                case CarManufactures.Couppe:
                    return 40;
                case CarManufactures.Mini:
                    return 35;
                default:
                    return 0;
            }
        }
    }
}

Features.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace BL
{

    public abstract class Feature : IComparable
    {
        internal Feature(string name, int rating, float cost)
        {
            this._name = name;
            this._rating = rating;
            this._cost = cost;
        }

        internal string _name;

        public string Name
        {
            get { return _name; }
        }

        internal int _rating;

        public int Rating
        {
            get
            {
                return _rating;
            }
        }

        internal float _cost;

        public float Cost
        {
            get
            {
                return _cost;
            }
        }

        public virtual float Install()
        {
            Console.WriteLine("Base installing Feature of type: {0}.\nName of Feature: {1}", this.GetType(), this.Name);
            Thread.Sleep(200);

            return Cost;
        }

        public int CompareTo(Object obj)
        {
            if (obj == null) return 1;
            Feature otherObj = obj as Feature;
            if (otherObj == null || otherObj.GetType() != this.GetType())
                return 1;
            return this.Rating.CompareTo(otherObj.Rating);
        }
    }

    public class FuelInjection : Feature
    {
        public FuelInjection(string name, int rating, float cost) : base(name, rating, cost) { }

        private static FuelInjection _fuelJet;
        public static FuelInjection FuelJet
        {
            get
            {
                if (_fuelJet == null)
                {
                    _fuelJet = new FuelInjection("FuelJet1", 4, 3000);
                }
                return _fuelJet;
            }
        }


        private static FuelInjection _ninjet;

        public static FuelInjection Ninjet
        {
            get
            {
                if (_ninjet == null)
                {
                    _ninjet = new FuelInjection("NinJet2", 17, 9000);
                }
                return _ninjet;
            }
        }

        public override float Install()
        {
            Console.WriteLine("Adding current specific feature {0}...", this.Name);
            Thread.Sleep(400);
            //DO STUFF
            return base.Install();
        }


    }



    public class CruiseControl : Feature
    {
        public CruiseControl(string name, int rating, float cost) : base(name, rating, cost) { }

        private static CruiseControl _gCruise;
        public static CruiseControl GCruise
        {
            get
            {
                if (_gCruise == null)
                {
                    _gCruise = new CruiseControl("G.Cruise", 7, 1000);
                }
                return _gCruise;
            }
        }


        private static CruiseControl _xCruise;
        public static CruiseControl XCruise
        {
            get
            {
                if (_xCruise == null)
                {
                    _xCruise = new CruiseControl("X.Cruise", 15, 3000);
                }
                return _xCruise;
            }
        }

        public override float Install()
        {
            Console.WriteLine("Adding current specific feature {0}...", this.Name);
            Thread.Sleep(500);
            //DO STUFF
            return base.Install();
        }


    }


}

Console APP:

using BL;
using System;
using System.Collections.Generic;
using System.Linq;

namespace Test
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Start building cars\n------------------");
            List<Car> myCars = new List<Car>();
            myCars.Add(CarFactory.CreateCar(CarTypes.FamaliyCar, CarManufactures.Couppe, "Red"));
            myCars.Add(CarFactory.CreateCar(CarTypes.SportCar, CarManufactures.Couppe, "Red"));
            myCars.Add(CarFactory.CreateCar(CarTypes.SportCar, CarManufactures.Mini, "UK FLAG"));
            myCars.Add(CarFactory.CreateCar(CarTypes.SportCar, CarManufactures.Swift, "Blue"));
            myCars.Add(CarFactory.CreateCar(CarTypes.FamaliyCar, CarManufactures.Swift, "Black"));
            Console.WriteLine("\n\n---------------------");
            Console.WriteLine("/**** Compare cars: *****/");
            Console.WriteLine("---------------------");
            myCars = myCars.CompareCars(true);


            Console.WriteLine("\n\n---------------------");
            Console.WriteLine("/**** Specific feature vs car -  {0}: *****/", myCars[0]);
            Console.WriteLine("---------------------");
            foreach (Feature feature in myCars[0].Features)
            {
                Console.WriteLine("\nComparing feature - {0}: ",feature.GetType());
                foreach (Car car in myCars.Skip(1))
                {
                    if (car.HasFeature(feature))
                    {
                        Console.WriteLine("Feature exist in {1} and is better: {0} [{2} vs {3}]", car.GetFeature(feature).CompareTo(feature)==1, car, car.GetFeature(feature).Rating, feature.Rating);
                    }
                    else
                    {
                        Console.WriteLine("Feature does not exist in {0}",car);
                    }
                }
            }

        }
    }
}

Answer 1

In 30 min I could only create a draft. It's crazy what they requrie in an interview. It usually takes hours to come up with a resonable design.

Anyways this design has one very bad blocker that prevents it from being extendable. It's the CarManufactures switch. You have a fixed number of those types and you cannot add new ones without modifying the switch and the enum.

I would drop the Family/Sport-Car stuff and replace it with Coupe, Combi, SUV etc as they are the real car types. Those types should be classes derived from the Car so that you can add new ones easily.

Whether a car is a family or a sport car is usually defined by the features it has - sport seats, engine with more power etc. and it's rather subjective.

abstract class Car
{
    protected Car(string make) { ... }
    public string Make { get; }
    public Feature Feature { get; set; }
}

abstract class Combi : Car {}

abstract class Hatchback : Car {}

class FocusCombi : Combi
{
    public Focus() : base("Ford") {}
}

class FocusHatchback : Hatchback
{
    public Focus() : base("Ford") {}
}

With this structure you can create new car types anytime.

The feautures could be created with the composition pattern.

Then I would reduce the factory to just:

public static class CarFactory
{
    public static Car CreateCar<TCar>(Feature feature) where TCar : Car, new()
    {
        return new TCar() 
        {
            Feature = feature
        };        
    }
}

and use it like this:

CarFactory.CreateCar<FocusCombi>(Feature.Family);

You could configure the feature set by composition like:

abstract class Feature 
{
    public static Feature Family => Feature.Empty
        .Add<NinjectInjection>()
        .Add<PowerWindows>();
}

---

Feature composition

First we need an abstract class for other features and a helper Empty property. ForEach will us allow to get all features.

abstract class Feature
{
    public static Feature Empty => new CompositeFeature();

    public virtual void ForEach(Action<Feature> feature) { feature(this); }    
}

Next we need some contrete features:

class PowerWindows : Feature { }

class InjectionA : Feature { }

To build a feature chain we need a composite feature that will store the chain and it will also enumerate it:

class CompositeFeature : Feature
{
    private readonly Feature[] _features;

    public CompositeFeature(params Feature[] feautures)
    {
        _features = feautures;
    }

    public override void ForEach(Action<Feature> feature) 
    { 
        foreach (var f in _features)
        {
            f.ForEach(feature);
        }        
    }
}

With an extension like this we can easily build the chain:

static class FeatureComposition
{
    public static Feature Add<TFeature>(this Feature feature) where TFeature : Feature, new()
    {
        return new CompositeFeature(feature, new TFeature());
    }
}

Usage:

var feature = Feature.Empty
    .Add<PowerWindows>()
    .Add<InjectionA>();

feature.ForEach(x => /* do somehting with x */ );

It's a one big feature that is composed of many other features.

I've added the ForEach so that we can get them all and do something later with each of them.

One of my favourite patterns ;-)

---

Decorator

You can also build different cars with a decorator pattern instead of an inheritance.

You leave the Car untouched:

abstract class Car
{
    protected Car(string make) { ... }
    public string Make { get; }
    public Feature Feature { get; set; }
}

but then instead of more abstract classes you wrap a Focus as a Combi

class Focus : Car
{
    public Focus() : base("Ford") {}
}

class Combi : Car 
{
    private readonly _car;
    public Combi(Car car) { _car = car; }
}

Example:

var focus = new Focus();
var focusCombi = new Combi(focus);

---

Finding features

To find a feature I would create an extension like this one:

static class FeatureExtensions
{ 
    public static T Find<T>(this Feature feature) where T : Feature
    {
        var result = default(T);
        feature.ForEach(f =>
        {
            if (f.GetType() == typeof(T)) 
            {
                result = (T)f;
                return;
            }
        });
        return result;
    }
}

Example:

var injectionA = feature.Find<InjectionA>();
var injectionB = feature.Find<InjectionB>();
...do the comparison

You can virtually add anything you want.

Answer 2

The list of features is a very flexible solution that follows the "composition over inheritance" rule and allows to add features that are not even known yet.

However, the downside of that approach is, that different groups of features exist where items from one group are not compatible to other ones of the same group (e.g. it makes no sense to add FuelJet and Ninjet), but the design allows the creation of that state. Another downside is, the the Car class has to search a feature in the list of features (by type or name or whatever) if it want to use it.

Another approach (that is not so flexible) is to create an abstract Car class with the abstract property CruiseControl of type CruiseControl, FuelInjection of type FuelInjection and so on. Than create one concrete car implementation for each combination and choose the correct implementation in the CarFactory.

Both approaches have pros and cons. However, that task was to create a car factory and the CarFactory of the second approach feels more like a factory whereas the CarFactory of the first approach feels more like a builder...

Answer 3

Well we can't read the interviewers' minds.... My initial guess: Build to the requirements at hand. Do these guys use the "agile" buzzword to describe themselves I wonder?

---

Is there something I could do better with my factory design?

myFactory.BuildCar(make, model, trimLevel, options). That factory instantiates a FordFactory, BmwFactory, etc. Each manufacturer-factory knows it's own models and associated trim levels. So there is some kind of "data set", a common structure with data specific to each manufacturer.

---

Car Meta Data

Off the top of my head...

A trim level is a standard set of features. And higher trim levels are supersets of lower ones. Then options are a-la carte choices outside of trim levels.

Next we need structure to hold that meta data. I imagine a CarModel class containing a collection of available trim levels and another of available options. Each manufacturer-factory has a "data set" of their specifics.

There must be some global stuff, especially so we can pass arguments to the factory. There will be "collections" - makes, models, trim, options. But what kind? Any expedient kind for now. And maybe even enums!

---

What matters is the API

API is evolving because we know almost nothing about what we're building at this point. But API is relatively stable and that allows the implementation to morph.

Our goal is not to achieve maximum design pattern density. What we know at this point is simple so design simply. But I was hinting at the abstract factory pattern above.

.net collection classes inform us about our likely API. "contains", "find", "add", "remove", etc. so I'll go with that.

---

Domain Specific Language, API, and Fungible Implementation

I tend to write custom collections - very DSL-ish - that contains a List or whatever. That way the actual type of collection can change at will and I totally control the API, I hide all the .net public methods, wrapped/exposed as needed in DSL custom methods.

In terms of DSL, individual manufacture factories is better than a single monolithic factory. It expresses the car-making domain better. It is not a question of "the single factory is simple enough." That's never the right answer. Well, never say never.

---

Is there something wrong with the approach of List of Feature for each car?

I do not know what "compare cars" is, but implementing IComparable is not it. As your code shows, it orders features. But off hand ranking (sorting essentially) makes no sense to me. Which is greater "leather seats" or "air conditioning"? Are "child locks" and "electric windows" equal?

Web sites I've been to compare things by selecting specific features and then filtering items that meet that selection.

---

Edit

A more detailed code review and addressing comments and answers in this thread.

---

Car.AddFeature()

I really like that the Car client does not need to know any internal structure, only telling the car (asking?) to add a feature.

---

Big thumbs up for the ToString override. I almost never see anyone take advantage of that.

---

You know "BL" for a namespace is horrible.

---

private float price;

Use Decimal for money. MSDN documentation explains why.

---

public readonly CarTypes Model; public readonly CarManufactures Manufacture;

Just "Type", "Manufacturer". And not plural. We have context - this is in the Car class.

---

I'm on the fence about the extension methods. This smells like we have non-cohesive classes. Why are GetRating and GetBaseRating not in Car.

As I read over all the code I seeing a need for a car compare class. This class will consolidate code and make the idea of comparing cars cohesive.

• main does the hard detail work of comparing.
• and so does CompareCars extension
• Why are these different? What's the context?
• Feature.CompareTo - is exposed directly in what looks like a complex ToString method inside of main. The client, main, must know everything in that method. Stuff like CompareTo() == 1 should be encapsulated with a method name that tells us what ==1 means.

---

Make Useful Collections

Given the current code I think this is the most significant change that can be made. It has broad implications. And it's easier to implement at this point than the meta data idea, and the meta data idea needs these collections anyway.

internal List<Feature> _features; 
//should be a domain specific named  list/collection. Then write meaningful methods. 

So in Car, not this...

internal void AddFeature(Feature feature) {
   this._features.Add(feature);
   this.price += feature.Install();
}

... but this:

internal void AddFeature(Feature feature) {
   this._features.AddFeature(feature);
   // the price calc should go too, see below
}

This goes to single responsibility, DSL, and reuse. The features collection should know how to add a feature to itself. I should not have to instantiate a car just to do that. AND we can check for nulls, duplicates, or anything else to ensure we have a clean, valid collection.

Calculating price should be dynamic. I guarantee that it gets buggy otherwise. And the various collections should do their part:

public Decimal Price() {
   return this.BasePrice + Features.TotalPrice() + Options.TotalPrice();
}

So What?

• Solves the "I can call whatever method I want and screw things up" issue raised by @Paparazzi comment

• Appropriate responsibilities makes a class re-usable.

• OO principles are fractal. As we go down the call stack, it all reads abstractly (in DSL terms!) until we get to the class(es) with the source data.

• Decoupled classes. I don't need a car object to calculate the cost of my features list.

---

Domain Data

I really cannot over emphasize how important it is to define basic, foundational data. It prevents problems and has a very strong tendency to really simplify code. It will prevent some problems as shown and discussed on this thread.

Here is my first draft of that idea. Don't get hung up on access modifiers. enums vs string vs whatever. That's implementation details to tweak later:

public class CarConfiguration {
  public Manufacturer WhoMakesMe { get; protected set; }
  public Model model { get; protected set; }
  public FeatureCollection Features { get; protected set; }
  public OptionCollection Options { get; protected set; }
}

enum Manufacturer { undefined, Ford, Chevrolet, BMW }
enum Model {F150, Corvette, i330 }
public Dictionary<Manufacturer, ModelCollection> MakerModels;
public Dictionary<Model, FeatureCollection> ModelFeatures;
public Dictionary<Model, OptionCollection> ModelOptions;

• This is a hurried sketch. "Rating" and "Type" has to be in here somewhere. I may be redefining your domain inappropriately when I talk about "model", for example.

• A factory will a "data set" of this stuff. More structure/heirarchy may be desired. We'll see how the coding goes.

• Totally re-usable. I can see instances of this data bound to a UI. Maybe also to carry the data for unique car specifications - for comparing or purchasing.

• The Dictionaries and enums are declared and populated beforehand. Hard code the dictionaries for expediency. We can rig up a database connection later, but guess what. This configuration API hides that.

• Sounding really messy? That's what factories are for, hiding the messy building details. But all that messiness is HERE in the factory construction. The client class code will be a pleasure to write, I promise.

So What?

Solves the controlling features issue raised by @Paparazzi comment

We won't hard code constructor parameters

class FocusCombi : Combi{
   public Focus() : base("Ford") {}
}

Prevents an explosion of car subclasses with the concomitant tortured class names that is sure to happen. We don't need a subclass for every combination of Manufacturer, Model, (Car)Type, whatever.

Subclassing Car is a design decision. It's not wrong but weigh the pros and cons. In any case hard coding constructor parameters is a code smell. (A smell means it might be bad, but I say it is).

Ditto for Feature sub-classing.

---

Console App

Everything after the car list instantiation should be in a class. main is not part of the domain model. main should be the application driver only. If you give me the domain model code I would have to re-invent the code left behind in main. From a design/modeling perspective an explicit requirement of the coding exercise is missing.

Ideally all the involved classes could override ToString so the output is just a bunch of ToString calls. And most of that is implicit. Console.WriteLine know to call ToString.

public class CarComparer {
  // TODO: create a compare function. It will output in a form
  // that we can use directly in the format string as seen in ToString.
  // comparing inside ToString is a single responsibility principle violation
  // but for now at least we got the building of comparison report
  // out of main. main should read very high level. Grunt work belongs
  // in appropriate classes. 
  // TODO: make a "comparison results" structure that is easy to use, 
  // particularly using the string formatting in ToString.

  private CarCollection selectedCars;

  public CarComparer(CarCollection cars) {
     selectedCars = cars;
  }

  public overide string ToString() {
     // Leverage the custom collections!
     FeaturesCollection matchingFeatures;
     Car targetCar = myCars.RemoveCar(0); 
     StringBuilder me = new StringBuilder ();

     me.AppendLine("\n\n---------------------");
     me.AppendLine("/**** Compare cars: *****/");
     me.AppendLine("---------------------");
     me.AppendLine("/**** Specific feature vs car -  {0}: *****/", myCars[0]);
     me.AppendLine("---------------------");

     foreach (Car compareCar in myCars) {
       // FeatureCollection.MatchingFeatures() is called in here
       matchingFeatures = targetCar.MatchingFeatures(compareCar.Features);

       foreach(Feature feature in matchingFeatures) {
           me.AppendLine("Comparing feature - {0}: ",feature);
           ("Feature exist in {1} and is better: {0} [{2} vs {3}]",
                         targetCar.GetFeature(feature).CompareTo(compareCar.GetFeature(feature))==1,
                         compareCar, 
                         targetCar.GetFeature(feature).Rating, compareCar.GetFeature(feature).Rating);
        }
     }
     return me.ToString();
   }
}

end Edit