Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is my interface, all of my transport concrete classes will be implementing this interface.

interface ITransport
{
    void Travel()
}

These are my existing implementation class:

class Horse : ITransport
{
    public Horse()
    {
    }

    public void Travel()
    {
        Console.Writeline("I am travelling on a horse");
    }
}

class Camel : ITransport
{ 
    public Camel()
    {
    }

    public void Travel()
    {
        Console.Writeline("I am travelling on a Camel");
    }
}

class Ship : ITransport
{
    public Ship()
    {
    }

    public void Travel()
    {
        Console.Writeline("I am travelling on a Ship");
    }
}

This is my Factory class I create instance of my transport classes from this factory class.

class TransportFactory
{
    public TransportFactory()
    {
    }

    ITransport ProvideTransport(string transportType)
    {
        switch(transportType)
        {
            case "Camel": return Camel; break;
            case "Horse": return Horse; break;
            case "Ship": return Ship; break;
        }
    }
}

This is my creational class meaning I configure my client classes or consumer classes through this class:

class ConfigurationBuilder
{
    ITransport camel;
    ITransport ship;
    ITransport horse;

    public ConfigurationBuilder()
    {
        TransportFactory tFactory = new TransportFactory();
        horse = tFactory.ProvideTransport("horse");
        camel = tFactory.ProvideTransport("camel");
        ship = tFactory.ProvideTransport("ship");
    }

    Human ConfigureHuman()
    {
        return new Human(camel, ship, horse);
    }
}

This is my client code the actual caller of these transport class:

class Human
{
    ITransport camel;
    ITransport ship;
    ITransport horse;

    public Human(ITransport camel, ITransport ship, ITransport horse)
    {
        this.camel = camel;
        this.ship = ship;
        this.horse = horse; 
    } 

    void Travel()
    { 
        //TransportFactory tFactory = new TransportFactory(); 

        if(ground =="plain")
        { 
            horse.Travel(); 
        } 
        if(ground =="desert")
        {
            camel.Travel(); 
        } 
        if(ground =="sea")
        { 
            ship.Travel(); 
        } 
    }
}

Now my question is What will happen when I have a new means of traveling: such as Plane for Air and then Rocket for Space etc.

My Constructor will change and will keep on changing as more and more means of traveling keep on adding throughout application life time. Is this the correct design or is there a better way.

As for the question is considered I couldn't come up with intuitive words to explain my problem :)

Update: Commented TransportFactory out of Human class

Update2:

I guess I now understood how it will be done. I will be needing two factories now.TransportFactory which is already implemented above, the second one will be HumanFactory which will create a human but also has additional responsibility of calling travel method of appropriate ITransport.

public class HumanFactory
{
    ITransport camel;
    ITransport ship;
    ITransport horse;
    Human _human;
    Dictionary<string, ITransport> _availableTransports;
    event Action<Human, string> transportRequested;

    public HumanFactory(TransportFactory tFactory)
    {

        horse = tFactory.ProvideTransport(TransportTypes.Horse);
        camel = tFactory.ProvideTransport(TransportTypes.Camel);
        ship = tFactory.ProvideTransport(TransportTypes.Ship);
    }

    public Human ConfigureHuman()
    {
        if (_availableTransports == null)
        {
            _availableTransports = new Dictionary<string, ITransport>();
            _availableTransports.Add(GroundTypes.Desert.ToString(), camel);
            _availableTransports.Add(GroundTypes.Sea.ToString(), ship);
            _availableTransports.Add(GroundTypes.Plains.ToString(), horse);
        }

        transportRequested += new Action<Human, string>(_human_transportRequested);
        _human = new Human(transportRequested);

        return _human;
    }

    void _human_transportRequested(Human human, string groundType)
    {
        if (_availableTransports.ContainsKey(groundType))
        {
            ITransport suitableTransport = _availableTransports[groundType];
            suitableTransport.Travel();
        }
        else
        {
            //code for handling below conditions goes here
            //I don't know what to do for this type of plain?
        }
    }
}

Here is the Human class now:

public class Human
{
    Action<Human, string> _transportRequested;

    public Human(Action<Human, string> transportRequested)
    {
        _transportRequested = transportRequested;
    }

    public void Travel()
    {
        if (_transportRequested != null)
        {
            var ev = _transportRequested;
            ev.Invoke(this, GroundTypes.Plains.ToString());
        }
    }
}

We will call the Human class now as:

TransportFactory tFactory=new TransportFactory();
HumanFactory humanFactory = new HumanFactory(tFactory);

Human human = humanFactory.ConfigureHuman();
human.Travel();

One More thing: I guess I should be having a lock in the HumanFactory class in _human_transportRequested method? In case there is a multithreaded scenario. I guess now my code is following Law of Demeter :)

Special Thanks to Nik( for that marvelous solution) and Gleb( for those wonderful videos)

share|improve this question
2  
You should indent your code properly. That makes it more pleasant to read, and increases the chances of a good answer. –  Lstor Jul 12 '13 at 8:07
    
return Camel This won't compile, did you mean return new Camel()? –  svick Jul 12 '13 at 10:13
    
@svick Yes it should be return new camel. Sorry, was in a hurry just write it in a notepad :) –  shankbond Jul 12 '13 at 12:07
    
I would suggest you name your interface ITransportable, following a bit of a naming convention. –  Abbas Jul 12 '13 at 13:11
    
@Abbas That wouldn't make much sense. A ship is not something that can transported, it's something that transports other things. So I think ITransport is a good name here. –  svick Jul 12 '13 at 20:05

3 Answers 3

up vote 3 down vote accepted

I assume that this is just an example and I don't need to tell you that you should use enums instead of strings, remove breaks after returns, indent your code, etc. :) So, to answer your question: I think you should refactor your Human class:

class Human
{
    private readonly TransportFactory _tFactory;

    public Human(TransportFactory tFactory)
    {
        if (tFactory == null) 
        {
            throw new ArgumentNullException("tFactory");
        }
        _tFactory = tFactory;
    }

    void Travel(string ground)
    { 
        var transport = _tFactory.ProvideTransport(ground);
        transport.Travel();
    }
}

class TransportFactory
{
    ITransport ProvideTransport(string groundType)
    {
        switch(groundType)
        {
            case "desert":
                return new Camel();
            case "plain":
                return new Horse();
            case "sea":
                return new Ship();
            default:
                return new ArmsAndLegs();
        }
    }
}

You will still need to modify factory method when you add new transport though. This can be avoided by using reflection.

Edit: here is another example:

class Human
{
    public event Action<Human, ПкщгтвЕнзуы> TransportRequested;

    void Travel(GroundTypes ground)
    { 
        var ev = TransportRequested;
        if (ev != null)
        {
            ev(this, ground);
        }
    }
}

interface ITransport
{
    void Transport(object cargo);
}


class TravelAgency
{
    private readonly TransportFactory _tFactory;

    public TravelAgency(TransportFactory tFactory)
    {
        if (tFactory == null) 
        {
            throw new ArgumentNullException("tFactory");
        }
        _tFactory = tFactory;
    }

    //this method at some point subscribes to TransportRequested event
    private void OnTransportRequested(Human human, GroundTypes ground)
    {
        var transport = _tFactory.ProvideTransport(ground);
        transport.Transport(human);
    }
}

Edit2

Subscription using your TransportFactory logic:

public class HumanFactory
{
     Dictionary<GroundTypes, ITransport> _availableTransports;

     public HumanFactory(TransportFactory tFactory)
     {
        _availableTransports = new Dictionary<string, ITransport>();
        _availableTransports.Add(GroundTypes.Desert, tFactory.ProvideTransport(TransportTypes.Horse));
        _availableTransports.Add(GroundTypes.Sea, tFactory.ProvideTransport(TransportTypes.Camel));
        _availableTransports.Add(GroundTypes.Plains, tFactory.ProvideTransport(TransportTypes.Ship));
     }

     public Human CreateHuman()
     {
         var h = new Human();
         h.TransportRequested += OnHumanTransportRequested;
         return h;
     }

     //you can incapsulate this logic in some other object
     //you should also consider implementing Events Aggregator
     //or those subscriptions might become quite complicated as their number increases
     void OnHumanTransportRequested(Human human, GroundTypes groundType)
     {
         if (_availableTransports.ContainsKey(groundType))
         {
             ITransport suitableTransport = _availableTransports[groundType];
             suitableTransport.Travel();
         }
         else
         {
             //code for handling below conditions goes here
             //I don't know what to do for this type of plain?
         }
     }
 }
share|improve this answer
    
Actually I was trying to avoid passing factory in the constructor as I was following Law of Demeter. Is there some other way? –  shankbond Jul 12 '13 at 12:11
    
@shankbond, well, your initial code contains factory reference in Human class, so i thought its not a problem for you. :) But it can be removed, sure. To do so, you should delegate this logic to higher level. The easiest way do it is to rise an event (public event Action<Human, string> RequestTransport;, for example) in your Travel(string ground) method and do the handling in the object which has the knowledge about factories (Society? :)). You can also use EventsAggregator pattern for easier event handling. –  Nikita Brizhak Jul 12 '13 at 12:28
    
I've added a somewhat simplified example. –  Nikita Brizhak Jul 12 '13 at 12:40
1  
I'd also mark those constructor-injected, assigned to member variables as readonly to signify intent as well as check them for null before assignment. –  Jesse C. Slicer Jul 12 '13 at 13:20
    
@JesseC.Slicer, good point. :) –  Nikita Brizhak Jul 12 '13 at 17:23

Sticking to your example, I came up with the following:

1) Interface for Transports

interface Transport
{
    void Travel();
    bool isSuitableForGround(Ground ground);
}

2) Possible implementation

class Horse:Transport
{
    HashSet<Ground> suitableGrounds = new HashSet<Ground> { Ground.Plain };
    public bool isSuitableForGround(Ground ground)
    {       
            return suitableGrounds.Contains(ground);
    }

    public void Travel()
    {

    }
}

So. The Horse knows best, upon which grounds it would walk.

3) The Factory should know nothing of the abilities of what it delivers except the type. The ground which a transport is suitable for is up to the transport itself. It knows best.

class TransportationsFactory
{
    Dictionary<Type, TransportFactory> transporters=new Dictionary<Type,TransportFactory>();

    public Transport GetTransportType<T>(){
        if (transporters.ContainsKey(typeof(T)) == false) throw new Exception();
        return transporters[typeof(T)].produce();
    }

    public TransportationsFactory(){
        transporters.Add(typeof(Horse),new HorseBreeder());
    }

}

If you want a Horse, you get a Horse by

transportationsFactory.GetTransportType<Horse>()

4) The implementation of a human looks like the following:

class Human
{
    List<Transport> transports = new List<Transport>();

    public void TravelOn(Ground ground){
        try
        {
            Transport possibleTransports = transports.Where(t => t.isSuitableForGround(ground)).First();
            possibleTransports.Travel();
        }
        catch (InvalidOperationException ex) { 
            //Do something useful
        }

    }

    public Human(IEnumerable<Transport> transports){
        this.transports = transports.ToList();
    }
}

Of course you could choose another strategy to get a suitable transport. For the sake of the example I chose the strategy to get the first one able to do the job.

Since you asked for constructor injection, I implemented it this way, taking a collection of transports. I would prefer setter injection.

If there is the need to travel on a (different) ground, the human asks its collection of transports, whether one of it is capable of doing the job - so the human too has not to have any knowledge at all what is good for what job.

If you ever come up with a car, you add it to your factory and ask your factory for that. And the car knows best, for what grounds it is made for.

share|improve this answer

At first there are a lot of random "new" operators. It is best to compose application on it entry point like:

 public class Program 
 {
         public static void Main()
         {
            IDictionary<GroundType, ITransport> transports = new Dictionary<GroundType, ITransport> 
            {
                { GroundType.Plain, new Horse() },
                { GroundType.Desert, new Camel() },
                { GroundType.Sea, new Ship() }
            };


            TransportFactory transportFactory = new TransportFactory(transports);

            Human hero = new Human(transportFactory);
            hero.Travel();
        }
}

    public class Human
    {
        private readonly TransportFactory _transportFactory;

        private GroundType _ground;

        public Human(TransportFactory transportFactory)
        {
            _transportFactory = transportFactory;
        }

        public void Travel()
        {
            var transport = _transportFactory.GetTransport(_ground);
            transport.Travel();
        }
    }

    public class TransportFactory
    {
        private readonly IDictionary<GroundType, ITransport> _transports;

        public TransportFactory(IDictionary<GroundType, ITransport> transports)
        {
            _transports = transports;
        }

        // virtual will allow to override this method with test stub\mock
        public virtual ITransport GetTransport(GroundType groundType)
        {
            ITransport transport;
            if (!_transports.TryGetValue(groundType, out transport)) {
                // this way we can find any issues earlier 
                throw new NotSupportedException("The current ground of type '{0}' is not supported");
            }

            return transport;
        }
    }

The full DI Composition Root might look after some time:

public static void Main()
{
    IDictionary<GroundType, ITransport> transports = new Dictionary<GroundType, ITransport> 
    {
        { GroundType.Plain, new Horse() },
        { GroundType.Desert, new Camel() },
        { GroundType.Sea, new Ship() }
    };


    TransportFactory transportFactory = new TransportFactory(transports);

    Human hero = CreateHuman(transportFactory);
    Human enemy = CreateHuman(transportFactory);

    Continent continent = Continent.LoadFromResource(DefaultContinent);
    continent.Place(hero, new Vector3(0, 0, 0));
    continent.Place(enemy, new Vector3(110, 30, 0));

    Continent demonicContinent = Continent.LoadFromResource(DemonicContinent);

    World world = new World(new List<Continent> { continent, demonicContinent });
    Game game = new Game(world, new DefaultRulesProvider());
    game.Start();
}

I also suggest to read Mark Seemann's "Dependency Injection in .NET" book.

There are also a lot of talks about testability, DI, and OOP by Misco Hevery - I would have watch them 4 years earlier!

share|improve this answer
1  
I think You should get +1000( unfortunately I don't have that right to up-vote You) for those awesome links Misco rocks :) –  shankbond Jul 13 '13 at 6:27
    
Don't You think that 'var transport = _transportFactory.GetTransport(_ground); transport.Travel();'(It is in Human class Travel method) is violating Law of Demeter? –  shankbond Jul 13 '13 at 6:31
    
Probably in this specific example Human is not an ITransport himself. The more real world example might be AggregateLogger when it implements ILogger interface, and just iterates through each attached logger: public void Info() { foreach (var logger in _loggers) {logger.Info();}}; –  Gleb Sevruk Jul 15 '13 at 8:10
    
I must agree - factory inside Human class is over-complicated. It is better to inject list of Transports directly into human constructor in this case. –  Gleb Sevruk Jul 16 '13 at 11:54
    
do I violate Law of Demeter or Dependency Injection in any case if I pass a list / dictionary of dependencies into the constructor? –  shankbond Jul 16 '13 at 15:20

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.