0

Sorry for the lenghty post. I tried to show my attempts and thought process as much as possible.

I got an interface exposing several possible behavior, but there is only one implementation of this interface that is instantiated and only one of the exposed method that can be called in each context where the interface is realized. This interface will be used in very different context of an application and I wish to avoid exposing method that can't be called. I wish to find a way so that the caller of IRescheduler would only know one behavior despite different method signatures. I'll detail and example and what I tried so far

public interface IRescheduler
{
    AmountByTimeInterval RescheduleTomorrow(Amount amount);
    AmountByTimeInterval RescheduleAtGivenDate(Amount amount, DateTime rescheduleDate);
    // there will probably be more date strategies in the future
}

AmountByTimeInterval contains an Amount and a TimeInterval associates a string with a timespan from the current date. For example "1Day" would be the timespan from tomorrow to tomorrow and "1Year" would starts a year from now and ends a year later.

public class AmountByTimeInterval
{
    public Amount Amount { get; private set; }
    public TimeInterval TimeInterval { get; private set; }

    public AmountByTimeInterval(Amount amount, TimeInterval timeInterval)
    {
        Amount = amount;
        TimeInterval = timeInterval;
    }
}

public class Amount
{
    public double Value { get; private set; }
    public string Currency { get; private set; }

    public Amount(double amount, string currency)
    {
        Value = amount;
        Currency = currency;
    }
}

public class TimeInterval
{
    public string Name { get; private set; }
    public DateTime StartDate { get; private set; }
    public DateTime EndDate { get; private set; }

    public TimeInterval(string name, DateTime startDate, DateTime endDate)
    {
        Name = name;
        StartDate = startDate;
        EndDate = endDate;
    }
}

For the sake of this example, let's suppose an IRescheduleAmountCalculator interface that takes Amount to make other Amount

public interface IRescheduleAmountCalculator
{
    Amount ComputeRescheduleAmount(Amount amount);
}

Here is an example implementation of my IRescheduler interface. I got a repository pattern that gets me the TimeInterval associated to the DateTime.

public interface ITimeIntervalRepository
{
    TimeInterval GetTimeIntervalByName(string name);
    TimeInterval GetTimeIntervalByDate(DateTime date);
}

public class Rescheduler : IRescheduler
{
    private const string _1Day = "1Day";
    private readonly ITimeIntervalRepository _timeIntervalRepository;
    private readonly TimeInterval _tomorrow;
    private readonly IRescheduleAmountCalculator _calculator;

    public Rescheduler (ITimeIntervalRepository timeIntervalRepository, IRescheduleAmountCalculator calculator)
    {
        _calculator = calculator;
        _timeIntervalRepository = timeIntervalRepository;
        _tomorrow = timeIntervalRepository.GetTimeIntervalByName(_1Day);
    }

    public BucketAmount RescheduleTomorrow(Amount amount)
    {
        Amount rescheduledAmount = _calculator.ComputeRescheduleAmount(amount);
        return new TimeInterval(_tomorrow, transformedAmount);
    }

    public AmountByTimeInterval RescheduleAtGivenDate(Amount amount, DateTime reschedulingDate)
    {
        TimeInterval timeInterval = _timeIntervalRepository.GetTimeIntervalByDate(reschedulingDate);
        Amount rescheduledAmount = _calculator.ComputeRescheduleAmount(amount);
        return new TimeInterval(timeInterval, transformedAmount);
    }
}

I don't know beforehand the context in which IRescheduler would be called, it is meant to be used by many components. Here is an abstract class I intend to provide and an example of specific implementation

public abstract class AbstractReschedule<TInput, TOutput>
{
    private readonly ITransformMapper<TInput, TOutput> _mapper;
    protected readonly IRescheduler Rescheduler;

    protected AbstractReschedule(IMapper<TInput, TOutput> mapper, IRescheduler rescheduler)
    {
        _mapper = mapper;
        Rescheduler = rescheduler;
    }

    public abstract TOutput Reschedule(TInput entityToReschedule);

    protected TOutput MapRescheduledEntity(TInput input, TimeInterval timeInterval)
    {
        return _mapper.Map(input, timeInterval);
    }
}



public class RescheduleImpl : AbstractReschedule<InputImpl, OutputImpl>
{
    public RescheduleImpl(IRescheduleMapper<InputImpl, OutputImpl> mapper, IRescheduler rescheduler) : base(mapper, rescheduler)
    {
    }

    public override OutputImpl Reschedule(InputImpl entityToReschedule)
    {
        AmountByTimeInterval rescheduledAmountByTimeInterval = Rescheduler.RescheduleTomorrow(entityToReschedule.AmountByTimeInterval.Amount);
        return Map(entityToReschedule, rescheduledAmountByTimeInterval);
    }
}

public interface IMapper<T, TDto>
{
    TDto Map(T input, AmountByTimeInterval amountByTimeInterval);
}

Forcing an interface on TInput generic parameter is out of question, as the component is meant to be used in a large number of bounded contexts. Each future user of this whole rescheduling component would implement its own implementation of AbstractReschedule and IMapper.

I tried a strategy pattern but the different method argument blocked me as I couldn't define an interface contract that would allow all behaviour without exposing the actual implementation of IRescheduler.

Then I implemented a visitor pattern, where IRescheduler would have an Accept method and an implementation by behavior :

public interface IRescheduler
{
    AmountByTimeInterval Accept(IReschedulerVisitor visitor, Amount amount);
}

public class RescheduleTomorrow : IRescheduler
{
    public AmountByTimeInterval Accept(IReschedulerVisitor visitor, Amount amount)
    {
        return visitor.Visit(this, amount);
    }
}

public class RescheduleAtGivenDate : IRescheduler
{
    public AmountByTimeInterval Accept(IReschedulerVisitor visitor, Amount amount)
    {
        return visitor.Visit(this, amount);
    }
}

As you noticed, the DateTime is not present here, because I actually inject it in the visitor, which is built by a Factory

public interface IReschedulerVisitor
{
    AmountByTimeInterval Visit(RescheduleTomorrow rescheduleTomorrow, Amount amount);
    AmountByTimeInterval Visit(RescheduleAtGivenDate rescheduleAtGivenDate, Amount amount);
}

public class ReschedulerVisitor : IReschedulerVisitor
{

    private readonly ITimeIntervalRepository _timeIntervalRepository;
    private readonly DateTime _chosenReschedulingDate;
    private readonly IRescheduleAmountCalculator _rescheduleAmountCalculator;
    private const string _1D = "1D";

    public ReschedulerVisitor(ITimeIntervalRepository timeIntervalRepository, IRescheduleAmountCalculator rescheduleAmountCalculator)
    {
        _timeIntervalRepository = timeIntervalRepository;
        _rescheduleAmountCalculator = rescheduleAmountCalculator
    }

    public ReschedulerVisitor(ITimeIntervalRepository timeIntervalRepository, IRescheduleAmountCalculator rescheduleAmountCalculator, DateTime chosenReschedulingDate)
    {
        _timeIntervalRepository = timeIntervalRepository;
        _chosenReschedulingDate = chosenReschedulingDate;
        _rescheduleAmountCalculator = rescheduleAmountCalculator
    }

    public AmountByTimeInterval Visit(RescheduleTomorrow rescheduleTomorrow, Amount amount)
    {
        TimeInterval reschedulingTimeInterval = _timeIntervalRepository.GetTimeIntervalByName(_1D);
        Amount rescheduledAmount = _rescheduleAmountCalculator(amount);
        return new AmountByTimeInterval(reschedulingTimeInterval, rescheduledAmount); 
    }

    public AmountByTimeInterval Visit(RescheduleAtGivenDate rescheduleAtGivenDate, Amount amount)
    {
        TimeInterval reschedulingTimeInterval = _timeIntervalRepository.GetTimeIntervalByDate(_chosenReschedulingDate);
        Amount rescheduledAmount = _rescheduleAmountCalculator(amount);
        return new AmountByTimeInterval(reschedulingTimeInterval, rescheduledAmount); 
    }
}

public interface IRescheduleVisitorFactory
{
    IRescheduleVisitor CreateVisitor();
    IRescheduleVisitor CreateVisitor(DateTime reschedulingDate);
}

public class RescheduleVisitorFactory : IRescheduleVisitorFactory
{
    private readonly ITimeIntervalRepository _timeIntervalRepository;

    public RescheduleVisitorFactory(ITimeIntervalRepository timeIntervalRepository)
    {
        _timeIntervalRepository = timeIntervalRepository;
    }

    public IRescheduleVisitor CreateVisitor()
    {
        return new RescheduleVisitor(_timeIntervalRepository);
    }

    public IRescheduleVisitor CreateVisitor(DateTime reschedulingDate)
    {
        return new RescheduleVisitor(_timeIntervalRepository, reschedulingDate);
    }
}

Finally (sorry for lengthy post), the RescheduleImpl that every user would have to implement would become like this :

public class RescheduleImpl : AbstractReschedule<InputImpl, OutputImpl>
{
    public RescheduleImpl(IRescheduler rescheduler, IRescheduleVisitorFactory visitorFactory, IRescheduleMapper<InputImpl, OutputImpl> mapper)
        : base(cancel, visitorFactory, mapper) {}

    public override OutputImpl Reschedule(InputImpl entityToReschedule)
    {
        AmountByTimeInterval rescheduledAmountByTimeInterval = rescheduler.Accept(visitorFactory.CreateVisitor(), entityToReschedule.AmountByTimeInterval.Amount);
        // the second case would be :
        // AmountByTimeInterval rescheduledAmountByTimeInterval = rescheduler.Accept(visitorFactory.CreateVisitor(entityToReschedule.Date), entityToReschedule.AmountByTimeInterval.Amount);
        return Mapper.Map(entityToReschedule, rescheduledAmountByTimeInterval);
    }
}

While this works, I'm quite unhappy with the solution. I feel like the implementer of my solution would decide of the rescheduling strategy twice. The first time when chosing the implementation of IRescheduler to use build the last RescheduleImpl class I showed, and a second time when deciding which method of the factory to call. I'm currently out of ideas and open to any that could solve the original problem. I'm also open to totally different implementation than my visitor + factory attempt.

Thank you for taking the time to read or answer my problem.

FooBar
  • 132
  • 1
  • 9
  • This might be better suited for Code Review or even Software Engineering. Just notice that the `Amount` class, which I understand represents a value someone will pay, should **not** use `double Value` but instead `decimal Value`, you can read about that [here](https://stackoverflow.com/questions/1165761/decimal-vs-double-which-one-should-i-use-and-when) – Camilo Terevinto Apr 12 '19 at 17:05
  • Thanks for the reply. My apologies if I posted to the wrong stack exchange, I didn't know about these ones. How can I reference my problem to those stack exchange without doing forbidden crossposting ? I am aware of the decimal vs double problem. This is more an artificial example of my need than actual production code. Let's just imagine Amount is equatable and got the appropriate operator overload to handle a 4 digit accuracy :p (just kiddin, you're absolutely right here) – FooBar Apr 12 '19 at 17:17
  • I think the reschedule interface should take a value and change it to another time. Many different things can implement that interface -- interfaces called schedule at date or schedule tomorrow don't sound like interfaces, they sound like objects that implement a reschedule interface. – Hogan Apr 12 '19 at 17:34
  • @Hogan Thanks for the reply ! I'm not sure I understood your answer ? In my original interface, schedule at date and schedule tomorrow are methods I want to get rid of because there is no point knowing one if you use the other. My proposed but too complicated and flawed solution, they are object that implement a reschedule interface, as you said. Sorry If I didn't understand what you meant. – FooBar Apr 12 '19 at 18:00
  • I guess then we agree. An interface for "reschedule" should not look like what you posted. – Hogan Apr 12 '19 at 18:02

2 Answers2

2

I think the fundamental reason why this has gotten so complicated is this:

I got an interface exposing several possible behavior, but there is only one implementation of this interface that is instantiated and only one of the exposed method that can be called in each context where the interface is realized.

Here's a way of rephrasing that:

I need different behaviors in different contexts, but I want them to all be in one interface.

The answer is not to do that. If you need one behavior here and a different behavior there, it's better to define one interface for the behavior you need here and another for what you need there.

This relates to the Interface Segregation Principle. Roughly it says that we shouldn't have one class depend on an interface but only use some if its members. When a class depends on an interface, that interface should only contain what the class needs.

If you put all of these behaviors in one interface, then it's likely to be implemented in one big class. And then every time you need another behavior, you add it to that interface, which means the class that implements it has to change. If that one class is used (to do entirely different things) by lots of other classes, then every change to the one class has potential to affect the others.

Or you might get part of the way through and realize that you want to re-architect this. You might see some way to simplify or improve. But then the same thing happens. Lots of classes depend on this interface for different reasons, so now your one change impacts lots of classes.

Or, in plainer terms: I wrote this class. I use parts of it in ten other classes. The next class I want to use it with needs something slightly different. So to meet the needs of one class, I'm going to modify the interface (and implementation) that ten other classes depend on. That might mean having to change all those classes, and I shouldn't have to change ten classes because of one. Or the change might accidentally break the other ten classes.

Those are ripple effects, and the ISP helps us to minimize them so that changing one thing doesn't affect or make us change other things.

If there are distinct behaviors and different classes need different ones, then it's better to "segregate" those interfaces, giving each class only what it needs. One way to accomplish that is to define each interface from the perspective of the class or classes that need them.

Sometimes we might try to pile more into one class so that the different types of behaviors can share some functionality or code, but there are other ways to accomplish that. If we find that two of these implementations need something similar or identical then we can just that duplicated part into a separate class and then both implementations depend on that.

Another reason why that approach is helpful is because it leads us to only write the code we need right now. Then as we write one, two, three classes, we might discover the commonalities and opportunities for reuse and refactor. That goes much more smoothly than if we try to plan for that commonality up front, write code based on that, and then once we start using it in other classes we discover that it wasn't what we needed.

Scott Hannen
  • 27,588
  • 3
  • 45
  • 62
  • Thanks for the reply. I think we agree that I shouldn't have all this behaviour in one interface. The goal of this post is to get rid of it ! I don't want to expose so much behaviour, which is why I tried some patterns like visitor or strategy to pick the algorithm I need in a better way. I am not very motivated to reimplementing an isolated, appropriate scheduling strategy in each context because of DRY principle. To reuse a fact used as an example by Uncle Bob, if I have 300 apps using Security Number check, I don't want each to implement their own check, I want a centralized service – FooBar Apr 12 '19 at 17:30
  • I was going to say "This has a bad smell to it because it is more complicated than it needs to be" but your points get to the root of the problem. – Hogan Apr 12 '19 at 17:31
  • What determines whether a class needs one strategy or a different one? When is that decision made? Does a given class always need the same behavior? If so you can provide it with the specific interface it needs. Or does one class sometimes need one behavior and sometimes another? If so, I'd start by solving that problem for just one class. – Scott Hannen Apr 12 '19 at 17:39
  • @ScottHannen Thanks for the reply. Each context is isolated from the others (DDD) and each got the need for rescheduling, each would call its own service object that would do the appropriate algorithm for them. This lead to considerable code duplication (the entities of each context change but there is 3/4 strategies), I'm trying to define in one place the possible strategies and have each context pick the one it needs (should sound like the strategy pattern). Except I don't know beforehand the entities of each context. There is only one possible strategy per context, chosen by business people – FooBar Apr 12 '19 at 17:55
0

@ScottHannen is correct that the problem relates to interface segregation. You are lucky that you have current requirements that demonstrate the problem right now, instead of finding out later and having to change a whole lot of deployed code.

But identifying the violated SOLID principle is not the same as fixing the problem, and I think you need a more practical answer, so:

Since different contexts can require different services from the reschedulers they use, you shouldn't try to force all reschedulers to use the same interface. You have identified two different kinds right now, but there's no reason there can't be more later.

Now, you may be thinking that you should split your IRescheduler into INextDayRescheduler and IFutureRescheduler, or whatever, and leave room for arbitrary others later on, BUT an arbitrary interface provides no value, so what this really means is that you should remove the requirement for the IRescheduler interface, since there is really no such interface at all.

You consume this interface in the AbstractReschedule constructor, but AbstractReschedule doesn't use it. So just stop that. Remove the constructor argument or (if you're omitted important code) take a different interface that gives it just what it needs.

With this one change, implementors of AbstractReschedule can just do it however they want and your problem is solved.

If there are many implementors that could use an INextDayRescheduler, or whatever, then go ahead and make a some handy utility classes that can make those common use cases easier to handle, but always bear in mind that these are utility classes that clients can chose to use, or not, according to their whims, instead of being requirements of your API.

Matt Timmermans
  • 53,709
  • 3
  • 46
  • 87
  • Thank you for the answer ! I ended up separating my two concerns, the multiple strategies on one hand and the lack of knowledge of the objects I would be working with on the other. Instead of trying to make something that can fit every contexts, I made the whole rescheduling itself into its own context. As it is its own context without outside world knowledge, it got its own inputs and outputs types and the users would need to define mappers between their types and rescheduling own entities. From here on, the problem is reduced to a simple Strategy. – FooBar Apr 15 '19 at 10:58