0

Though I have asked this question in Code Review but the original code is now creeping. Yes, I am also a big fan of Clean code talks just watched those awesome videos, I have also seen this another question. This is the same problem I was originally having.

I have a class say human. Human based on some decision in its Travel method can either call Horse, Camel or Ship To Travel or It can also ask all of them(under some situation) to Travel.

All of the Horse, Camel, Ship have ITransport interface and of course this interface is having Travel method.

initial problem is that it is a possibility that during the life time of my project I may get some new Transportation such as Plane, Rocket, Submarine etc.

So I cannot just Simple pass them in constructor as ITransport ship, ITransport horse..... and so on, as my constructor parameter will keep on swelling.

So I came to a solution as suggested (I think) that HumanFactory should be having an event and that event should be passed in the constructor of Human class.

Although I have somehow removed my large Transportation list, but as You know that interfaces can have lots of methods. So now I will need to pass lots of delegates each corresponding to a method of an interface and ofcourse on a need basis.

I even tried to solve this problem by creating a Human Mapper class whose sole responsibility is to map to right transport, call the right event. This works !

Now, since this is an imaginary example, in real world example the interface's methods accept parameters, so how will I deal with it?

I think the direction in which I am going is creating a Maintenance Nightmare.

I am pasting the code for a quick reference.

interface ITransport
{
    void Travel();
}

My Transport Factory is as:

public class TransportFactory
{
....
    internal ITransport ProvideTransport(TransportTypes transportType)
    {
        switch (transportType)
        {
            case TransportTypes.Camel: return new Camel();
            case TransportTypes.Horse: return new Horse();
            case TransportTypes.Ship: return new Ship();
            default:
                return null;
        }
    }
}

My Human class after suggestion has become as:

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());
        }
    }
}

I have a human class factory now as suggested which is as:

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?
        }
    }
}

I talked about a Mapper class which maps correct transport to correct methods as(it looks ugly but that was the best I came up with :) ):

class Human_Transport_MethodMapper
{
    Dictionary<GroundTypes, ITransport> _availableTransports;
    List<EventTypes> _availableEvents;
    event Action<Human, GroundTypes, EventTypes> transportRequested;
    internal Action<Human, GroundTypes, EventTypes> transportRequesteddel;
    public Human_Transport_MethodMapper(Dictionary<GroundTypes, ITransport> availableTransports, List<EventTypes> availableEvents)
    {
        _availableEvents = availableEvents;
        _availableTransports = availableTransports;
        transportRequested += human_OnAnyEventReceived;
        transportRequesteddel = transportRequested;
    }
    internal void human_OnAnyEventReceived(Human human, GroundTypes groundType, EventTypes eventType)
    {
        if (_availableTransports.ContainsKey(groundType))
        {
            ITransport suitableTransport = _availableTransports[groundType];
            switch (eventType)
            {
                case EventTypes.Travel: suitableTransport.Travel();
                    break;
                default:
                    break; //meaning interface's correct method has not been mapped.
            }

        }
        else
        {
            //code for handling below conditions goes here
            //I don't know what to do for this type of plain?
        }
    }
}

Now see that in this event, For Travel method if there were two arguments, then delegate signature would had changed, If there were four or five methods in ITransport interface, then may God help me.

I hope I have explained my problem here. Thanks

Edit: I am removing some obvious code from this question in order to make it more readable and also it is getting more verbose

1
  • what will happen if I simply pass a dictionary of Itransports along with their types(camel, horse, ship......etc), does it violate Law of Demeter or Dependency Injection?
    – Devesh
    Jul 15, 2013 at 16:21

1 Answer 1

0

First of all, the way you deal with standart events is confusing and overcomplicated. You dont need to pass events to constructor See my last edit in previous topic on how it can be simplified.

Also, as i mentioned in previous topic, the common way to deal with complicated event network in large applications is to implement EventsAggregator pattern (http://codebetter.com/jeremymiller/2009/07/22/braindump-on-the-event-aggregator-pattern/). There are zillions of various implenetations availible on the web, so i leave it up to you to pick one. I will use this interface for example purposes:

interface IEventsAggregator
{
    //sends message to network
    void Publish(object message);
    //adds object to the list of handlers
    void Subscribe(object listener);
    //removes object from the list of handlers
    void Unsubscribe(object listener);
} 

//listeners should implement this interface
interface IListener<TMessage>
{
    //handling logic for particular message
    void Handle(TMessgae message);
}

Then your code can be refactored:

//you do not need human factory in this example
public class Human
{  
    private readonly IEventsAggregator _events;

    //see Handle implementation for details
    //public ITransport Transport { get; set; }

    public Human(IEventsAggregator events)
    {
        _events = events;
    }

    public void Travel(GroundTypes type)
    {
        _events.Publish(new TransportRequest(this, type));
        //see Handle implementation for details
        //if (Transport != null) Transport.Travel();
    }
}

public class TransportRequest
{
    public Human Sender { get; set; }
    public GroundTypes Ground { get; set; }

    public TransportRequest(Human sensder, GroundTypes ground)
    {
        Sender = sender;
        Ground = ground;
    }
}

public class TravelAgency : IListener<TransportRequest>, IDisposable
{
    private readonly IEventsAggregator _events;
    private readonly TransportFactory _tFactory;

    public TravelAgency(IEventsAggregator events, TransportFactory tFactory)
    {
        _events = events;
        _events.Subscribe(this);
        _tFactory = tFactory;
    }

    public void Handle(TransportRequest request)
    {
        var transort = _tFactory.ProvideTransport(...);
        //insert the handling logic here
        //there are two ways to handle this message:
        //1) you give human no control over (and no knowledge of) Transport 
        //and simply call transport.Travel(request.Sender); here
        //2) or you add ITransport property to Human class
        //and do only the assignation here 
        //request.Sender.Transport = transport;
        //and then let the human manage Transport object
    }

    public void Dispose()
    {
        _events.Unsubscribe(this);
    }
}

This is probably how i would do it, if i needed to stricktly separate logic for some reason. It might be a bit too complicated for such trivial task, but i think it is a good basement for larger application. :) There are probably other approaches tho.

3
  • "See my last edit in previous topic", I can not navigate to Your previous topic?
    – Devesh
    Jul 15, 2013 at 16:07
  • what will happen if I simply pass a dictionary of Itransports along with their types(camel, horse, ship......etc), does it violate Law of Demeter or Dependency Injection? (also please fix the above mentioned link)
    – Devesh
    Jul 15, 2013 at 16:22
  • Does every human you create own camel, horse, ship, etc. from the moment of birth in your model of world? If yes, then sure, you can aggregate all tranports into human class. If no, then that doesnt make much sense. Just think logically.
    – Nikita B
    Jul 18, 2013 at 6:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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