Factory Pattern, open closed principle, interfaces and generics

Question

I've tried to refactor some code based on the Open-Closed Principle, but I just don't seem to be able to get the following classes right when it comes to applying design patterns. (I apologize for the many classes outlined below - I have reduced them as much as possible, but the rest is needed to show you my design).

The set-up consists of the following classes:

public interface IPortFactory
{
    IPort CreatePort(int id, PortDetails details);
}

public class PtpPortFactory : IPortFactory
{
    public IPort CreatePort(int id, PortDetails details)
    {
        var ptpPortDetails = details as PtpPortDetails;
        if (ptpPortDetails == null)
        {
            throw new ArgumentException("Port details does not match ptp ports", "details");
        }

        return new PtpPort(id, FiberCapability.FromValue(ptpPortDetails.Capability));
    }
}

public interface IPort
{
    int Id { get; }
}

public interface IInternetPort : IPort
{
    bool OfCapability(FiberCapability capability);
}

public class PtpPort : IInternetPort
{
    private readonly FiberCapability _capability;

    public PtpPort(int id, FiberCapability capability)
    {
        _capability = capability;
        Id = id;
    }

    public int Id { get; private set; }

    public bool OfCapability(FiberCapability capability)
    {
        return capability.Equals(_capability);
    }
}

Besides PtpPort, I have PonPort, which also implements IInternetPort, whereas CatvPort just implements IPort.

Already in this code, I think there's sign of code smell. In CreatePort in PtpPortFactory, the pretty thing would be for it to accept PtpPortDetails (which inherits from PortDetails) instead of having to cast. However, if I do so, I cannot create a PonPortFactory, which also implements IPortFactory, as these ports would need PonPortDetails. Or a CatvPortFactory for that matter.

Another code smell appears when I use the port factory:

PortType portType = command.PortType;
IPortFactory portFactory = portType.GetPortFactory();
var portsToSelectFrom = ports.Select(port => (IInternetPort) portFactory.CreatePort(port.Id, port.PortDetails)).ToList();

I would really like not having to do the downcast from IPort to IInternetPort, but simply have CreatePort return IInternetPort.

The last bit of information needed to understand the above is probably the following class (based on Jimmy Bogards Enumeration class):

public abstract class PortType : Enumeration<PortType, int>
{
    public static readonly PortType Ptp = new PtpPortType();
    public static readonly PortType Pon = new PonPortType();
    public static readonly PortType Catv = new CatvPortType();

    protected PortType(int value, string description)
        : base(value, description) { }

    public abstract IPortFactory GetPortFactory();

    private class CatvPortType : PortType
    {
        public CatvPortType() : base(2, "catv") { }

        public override IPortFactory GetPortFactory()
        {
            return new CatvPortFactory();
        }
    }

    private class PonPortType : PortType
    {
        public PonPortType() : base(1, "pon") { }

        public override IPortFactory GetPortFactory()
        {
            throw new NotImplementedException("Pon ports are not supported");
        }
    }

    private class PtpPortType : PortType
    {
        public PtpPortType() : base(0, "ptp") { }

        public override IPortFactory GetPortFactory()
        {
            return new PtpPortFactory();
        }
    }
}

I really hope that someone can help me along the way (I've tried introducing generics, but seem to always hit the barrier of C# not supporting return type covariance).

Moreover, any other tips 'n tricks to help me along my path to write better code would be greatly appreciated.

Update

Due to request in comment, I've added more code below.

public Port Handle(TakeInternetPortCommand command)
{
    var portLocatorService = new PortLocatorService();
    IList<Port> availablePorts = portLocatorService.FindAvailablePorts(command.Pop, command.PortType);

    PortType portType = command.PortType;
    IPortFactory portFactory = portType.GetPortFactory();
    var portsToSelectFrom = ports.Select(port => (IInternetPort) portFactory.CreatePort(port.Id, port.PortDetails)).ToList();

    IPort port = _algorithm.RunOn(portsToSelectFrom);
    Port chosenPort = availablePorts.First(p => p.Id == port.Id);
    chosenPort.Take(command.Spir);
    _portRepository.Add(chosenPort);
    return chosenPort;
}

Do not be confused by the fact that all of a sudden there's also a Port type. This is an aggregate in another bounded context (in the sense of DDD). The algorithm needs to take as input a list of IInternetPort, as it uses the method OfCapability internally to select a port. However, when the algorithm has selected the correct port, we are merely interested in the Id, hence the return type is just an IPort.

Answer 1

Here is how I understand the problem you are trying to solve (the business problem, not the design problem):

You have a list of ports, you want to execute some algorithm over the list that will choose a single port from the list (based on some criteria that is specific to the algorithm).

Here I will suggest a way to model this. I am going to assume that you have the following input class:

public class PortInput
{
    public int Id { get; set; }

    public PortDetails PortDetails { get; set; }
}

This corresponds to part of your Port class in your question.

And here you have the interface for the algorithm:

public interface IPortSelectionAlgorithm
{
    int SelectPort(PortInput[] port_inputs);
}

Please note that this interface models the problem that we want to solve. i.e., given a list of ports -> we need to choose one

And here is an implementation for such algorithm interface:

public class PortSelectionAlgorithm : IPortSelectionAlgorithm
{
    private readonly ICapabilityService<PortDetails> m_CapabilityService;

    public PortSelectionAlgorithm(ICapabilityService<PortDetails> capability_service)
    {
        m_CapabilityService = capability_service;
    }

    public int SelectPort(PortInput[] port_inputs)
    {
        //Here you can use m_CapabilityService to know if a specific port has specific capability

        ...
    }
}

What this implementation is declaring is that it requires some service that knows how to obtain the capabilities of ports based on port details. Here is the definition of such service contract:

public interface ICapabilityService<TDetails> where TDetails : PortDetails
{
    bool OfCapability(TDetails port_details, FiberCapability capability);
}

For each port type we can create an implementation of such service, like this:

public class PtpPortCapabilityService: ICapabilityService<PtpPortDetails>
{
    public bool OfCapability(PtpPortDetails port_details, FiberCapability capability)
    {
        ...
    }
}

public class CatvPortCapabilityService : ICapabilityService<CatvPortDetails>
{
    public bool OfCapability(CatvPortDetails port_details, FiberCapability capability)
    {
        ...
    }
}

public class PonPortCapabilityService : ICapabilityService<PonPortDetails>
{
    public bool OfCapability(PonPortDetails port_details, FiberCapability capability)
    {
        //If such kind of port does not have any capability, simply return false
        ...
    }
}

And then, we can create another implementation that uses these individual services to be able to tell the capabilities of any port:

public class PortCapabilityService : ICapabilityService<PortDetails>
{
    private readonly ICapabilityService<PtpPortDetails> m_PtpPortCapabilityService;
    private readonly ICapabilityService<CatvPortDetails> m_CatvPortCapabilityService;
    private readonly ICapabilityService<PonPortDetails> m_PonPortCapabilityService;

    public PortCapabilityService(ICapabilityService<PtpPortDetails> ptp_port_capability_service, ICapabilityService<CatvPortDetails> catv_port_capability_service, ICapabilityService<PonPortDetails> pon_port_capability_service)
    {
        m_PtpPortCapabilityService = ptp_port_capability_service;
        m_CatvPortCapabilityService = catv_port_capability_service;
        m_PonPortCapabilityService = pon_port_capability_service;
    }

    public bool OfCapability(PortDetails port_details, FiberCapability capability)
    {
        PtpPortDetails ptp_port_details = port_details as PtpPortDetails;

        if (ptp_port_details != null)
            return m_PtpPortCapabilityService.OfCapability(ptp_port_details, capability);

        CatvPortDetails catv_port_details = port_details as CatvPortDetails;

        if (catv_port_details != null)
            return m_CatvPortCapabilityService.OfCapability(catv_port_details, capability);

        PonPortDetails pon_port_details = port_details as PonPortDetails;

        if (pon_port_details != null)
            return m_PonPortCapabilityService.OfCapability(pon_port_details, capability);

        throw new Exception("Unknown port type");
    }
}

As you can see, no class knows about the port ID except the algorithm class. The classes that determine capabilities do not know about port IDs, because the can do their jobs without them.

Another thing to note is that you do not need to instantiate a new capability service each time you run the algorithm. This is in contrast to the IInternetPort implementations described in your question, where you create a new instance every time you want to execute the algorithm. And I guess you did that because each instance is bound to a different ID.

These classes use dependency injection. You should compose them to be able to use them. You should do this in the Composition Root.

Here is how you can use Pure DI for such composition:

IPortSelectionAlgorithm algorithm =
    new PortSelectionAlgorithm(
        new PortCapabilityService(
            new PtpPortCapabilityService(),
            new CatvPortCapabilityService(),
            new PonPortCapabilityService()));

Answer 2

I think there's sign of code smell. In CreatePort in PtpPortFactory, the pretty thing would be for it to accept PtpPortDetails (which inherits from PortDetails) instead of having to cast.

No, It's fine because dependencies should be on absctration and not implementations. So in this case passing PortDetails is fine.

The smell I see is here:

public interface IPort
{
    int Id { get; }
}

public interface IInternetPort : IPort
{
    bool OfCapability(FiberCapability capability);
}

Interfaces are basically used for defining behavior. And you're using properties in an Interface looks suspicious to me.

• Inheritance describes an is-a relationship.
• Implementing an interface describes a can-do relationship.

Here you're dealing AbstractFactory pattern. Let say, you can have abstract factory BasePortFactory which can-do what IPortFactory is declaring. So You should return BasePortFactory from factory method. But it's again a choice of matter when designing a solution.

Similary the CreatePort method should expose return type either base class or interface based on if you want to use is-a over can-do stuff.

Update

This sample wouldn't be a best fit for your scenario but this is to depict the idea that I shared:

public interface IInternetPort
{
    bool OfCapability(FiberCapability capability);
}

/// <summary>
/// This class can be a replacement of (IPort) interface. Each port is enabled for query via IInternetPort.
/// As a default behavior every port is not Internet enabled so OfCapability would return false.
/// Note: If you want you can still keep the IPort interface as Marker interface. 
/// /// </summary>
public abstract class Port : IInternetPort
{
    public int Id { get; private set; }

    public Port(int Id)
    {
        this.Id = Id;
    }

    public virtual bool OfCapability(FiberCapability capability)
    {
        // Default port is not internet capable
        return false; 
    }
}

/// <summary>
/// This class is-a <see cref="Port"/> and can provide capability checker.
/// Overiding the behavior of base for "OfCapability" would enable this port for internet.
/// </summary>
public class PtpPort : Port
{
    private readonly FiberCapability _capability;

    public PtpPort(int id, FiberCapability capability) : base(id)
    {
        _capability = capability;
    }

    public override bool OfCapability(FiberCapability capability)
    {
        return capability.Equals(_capability);
    }
}

/// <summary>
/// this test class doesn't need to implement or override OfCapability method
/// still it will be act like any other port.
/// | TestPort port = new TestPort(22);
/// | port.OfCapability(capability);
/// </summary>
public class TestPort : Port
{
    public TestPort(int id): base(id) { }
}

Here's the factories that would need to change the signature of method to return Port instead of IPort.

public interface IPortFactory
{
    Port CreatePort(int id, PortDetails details);
}

public class PtpPortFactory : IPortFactory
{
    public Port CreatePort(int id, PortDetails details)
    {
        var ptpPortDetails = details as PtpPortDetails;
        if (ptpPortDetails == null)
        {
            throw new ArgumentException("Port details does not match ptp ports", "details");
        }

        return new PtpPort(id, FiberCapability.FromValue(ptpPortDetails.Capability));
    }
}

Now this line won't need any external cast.

var portsToSelectFrom = ports.Select(port => portFactory.CreatePort(port.Id, port.PortDetails)).ToList();

P.S. - These type of questions should be asked on code review or programmer.