0

Does visitor pattern lead to violation of SRP? Take this for example:

class 401k
{
  public void MakeContribution(Contribution contribution)
  {
    contributions.Add(contribution);
  }

  // using double-dispatch here (violation)?
  public Money CalculateBalance(I401kBalanceCalculator balanceCalculator)
  {
    return balanceCalculator.Calculate(contributions);
  }

  public 401kReport GenerateReport(I401kReporter reporter)
  {
    return report.GenerateReport(contributions);
  }

  private IContributions contributions = new ContributionsList();
}

This class is NOT actually doing the calculating or report generating, it's just providing an interface to be able to pass the contribution data structure to. To me, this looks like a good design and follows SOLID principles. Am I incorrect in my thinking?

  • 2
    You're evaluating these techniques the wrong way. The way to properly evaluate them is on their merits and their applicability to your specific situation, not as a cage-match between two opposing principles. – Robert Harvey Dec 18 '17 at 20:13
  • Also, SRP doesn't mean what you think it means. I think you're using the term "double dispatch" incorrectly as well. – Robert Harvey Dec 18 '17 at 20:13
  • @RobertHarvey, a single reason to change. In this case (or as I extend functionality), I can't see more than a single reason to change. Am I incorrect? – keelerjr12 Dec 18 '17 at 20:16
  • 3
    Why do you think SRP is being violated here? – Robert Harvey Dec 18 '17 at 20:17
  • 4
    The code that you've posted has no interest whatsoever in what those methods do. That's kinda the whole point of all that ceremony. – Robert Harvey Dec 18 '17 at 20:18
5

The classic visitor pattern does not necessarily violate SRP. However, the code shown in the question does. If one just needs an "interface to be able to pass the contribution data structure to", why not implement this class C401k (added C to make the class name not start with a digit) as exactly that, and not more? The class could look like this:

class C401k
{
  public void MakeContribution(Contribution contribution)
  {
      contributions.Add(contribution);
  }
  public IContributions GetContributions()
  {
      return contributions;
  }

  private IContributions contributions = new ContributionsList();

  // no report generatiom or balance calculation code here!
}

Now, your CalculateBalance method needs to look like this

 public Money CalculateBalance(I401kBalanceCalculator balanceCalculator,
                               C401k contribs401k)
 {
    // ... maybe some more logic here
    return balanceCalculator.Calculate(contribs401k.GetContributions());
 }

which leads to the question where to put this method. One possible place could be C401k, but in this example, with a report generation entry point as well as a calculation entry point in one class, and a very vague name like 401k which does not describe a clear responsibility, there is IMHO a certain risk to create a god class holding lots of different, unrelated business logic methods. This would violate the SRP. I see two other candidate places here:

  • if CalculateBalance does not contain much more more logic than we see here, (because all the logic is somewhere in I401kBalanceCalculator), it could be a method of the class which calls it. If it is not needed more than once, and there is not any more logic than shown in the example, one could even consider to remove the method completely and replace its call simply by the line balanceCalculator.Calculate(contribs401k.GetContributions()).

  • or, if in the real code, there is more logic in CalculateBalance than in this contrived example, it makes probably sense to put it into a separate controller class of its own, something like a BalanceCalculatorController. That controller then could look like this:

    class BalanceCalculatorController
    {
       public BalanceCalculatorController(I401kBalanceCalculator balanceCalculator, 
                                          C401k contribs401k)
       {
          this.balanceCalculator=balanceCalculator;
          this.contribs401k=contribs401k;
       }
       public Money Calculate()
       {
           // ... implement additional logic here ...
           return balanceCalculator.Calculate(contribs401k.GetContributions());
       }
    
       private // some helper methods here ...
    }
    

Of course, one will have to think where to create this controller, where to pass it around to reuse it, how to test it, and so on. So this makes IMHO only sense if there is really more logic there which deserves a class on its own.

  • thank you. The reason I was doing it that way was because I wanted to follow a "pure" OO paradigm and not expose any of the internals. I assume I took this to the extreme? – keelerjr12 Dec 19 '17 at 13:15
  • @keelerjr12: I am not sure, since I don't think my proposed solution exposes anything "internal" which your original code did not already had to expose (except maybe the new GetContributions method, but that does probably not matter). – Doc Brown Dec 19 '17 at 13:24
  • 2
    The wrong place is probably C401k, since this would quickly lead to a god class holding lots of different, unrelated business logic methods -- Slippery slope fallacy. – Robert Harvey Dec 19 '17 at 20:13
  • @RobertHarvey: thanks for your comment. I agree, if that class had another name like "Calculator401k" or something like that, certain business logic could make sense there, when it fits to the overall abstraction introduced by that class. However, I think in this context where the OP stated he wants just "provide an interface to be able to pass the contribution data structure", and where this contribution collection shall be reused for calculation as well as for report generation, the SRP will probably better achieved by refactoring these concerns into different classes. – Doc Brown Dec 19 '17 at 22:45
  • @RobertHarvey: edited my answer, think its better now? – Doc Brown Dec 20 '17 at 12:12
1

Does visitor pattern lead to violation of SRP?

I think those are generally an orthogonal concepts. But it is definitely a violation of arguably more important concept -- encapsulation.

The very first line in wikipedia says:

the visitor design pattern is a way of separating an algorithm from an object structure on which it operates.

And this is a vivid trait of procedural programming, contradicting basic OOP principles. So in my opinion balance calculating logic should reside in 401k class. However it doesn't mean that you can't use any other classes to help you. You can, but without exposing 401k's internal behavior.

But there is a problem of 401k becoming a God object, that Doc Brown mentioned. I would take a good look whether you need 401k object representing the same class in all those use cases where its current methods are called. First of all, consider your bounded contexts. I doubt that the same class must represent a use case of adding a contribution and generating report (moreover, I doubt that you ever need an object when all you need is data representation). Chances are that they belong to different BCs. Than, you could split your model on write and read side. Adding a contribution is a write model, your domain, while a balance calculating probably could be your read model.

Here is how your report repository could look like:

class C401kReportRepository
{
    public function report()
    {
        // query your database
        return
            [
                [
                    'year' => 2017,
                    'month' => 11,
                    'amount' => 10000,
                ]
            ];
    }
}

Your ContributionsList can be exposed only if you're using an ORM (which is not mandatory, by the way), and this can be done implicitly by the framework itself, using reflection, for example. The point is not to expose it to an arbitrary clients. So if ContributionsList's data is used within a certain set of objects (401k object in your case), it's not such a big deal. After all, your data has to be exposed somewhere. Just don't make it a exposable by default, with getters.

  • do you have an example of different 401ks (or different objects) between different boundary contexts? – keelerjr12 Dec 19 '17 at 21:23
  • I think what you might be getting at is that you have two different objects that use the data of 401k contributions, but do different things. For example, one calculates a balance, the other generates a report. This I could see, but I assume it needs to take in a 401kContributions collection (possibly via the constructor) to be able to pull that data. This would imply that the 401kcontribution list is exposed. – keelerjr12 Dec 19 '17 at 21:25
  • Updated my answer. – Zapadlo Dec 20 '17 at 9:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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