3

Let say I've abstracted away a data structure such as a list; should this new object perform all the calculations or just provide minimal calculation functions and then allow a new object such as ReportGenerator to perform more large-scale calculations? Example below:

THIS?

class WrappedCollection
{
  public double CalculateByYear(int year) {...}

  public double CalculateCumulativeByYear(int year) {...}

  public double CalculateTotal() {...}

  private IList<double> collection = new List<double>();
}

OR THIS?

class WrappedCalculation
{
  public double CalculateByYear(int year) {...}

  public double CalculateTotal() {...}

  private IList<double> collection = new List<double>();
}

class ReportGenerator
{
  public ReportGenerator(WrappedCollection collection) {...}

  public double TotalByYear(int year) {...}
  public double CumulativeTotalByYear(int year) {...}
  public double Total() {...}

  private WrappedCollection collection;
}

This is a greatly simplified version; however, I'm starting to notice that in my WrappedCollection I am needing more methods and I feel as if I'm violating the SRP. The WrappedCollection class should just manage what I want (e.g. someone's retirement portfolio), and the reporting should left to another class that does all the calculations.

I seem to remember an example from Uncle Bob Martin or Martin Fowler that showed pushing all the data gathering further down the hierarchy into the collections themselves. This seems to generate more than 1 responsibility.

The project is comparing retirement portfolios; so I don't believe someone's 401k should give me all the metrics like cumulative contributions, etc. It should just give me it's current total, and maybe a contribution for a given year based on type (e.g. employer vs employee contribution). Another class can them compile a list of the cumulative contributions. Yes?

  • After doing some reading here link, I'm going to assume I want to keep the public interface as small as possible and allow another class (e.g. ReportGenerator) to compile the data. – keelerjr12 Dec 13 '17 at 16:27
  • Have a look at Map functions or the Visitor Pattern. – Robert Harvey Dec 13 '17 at 16:33
  • @RobertHarvey, could you elaborate slightly? This would require me to expose the underlying data structure with a getter correct? – keelerjr12 Dec 13 '17 at 16:42
  • 1
    Not necessarily. You can pass a higher-order function to your Map function without exposing the underlying collection. – Robert Harvey Dec 13 '17 at 16:44
0

Your portfolio class violates the Single Responsibility Principle because it has methods for performing tasks from two largely orthogonal groups - namely, maintaining portfolio's content, and computing reporting metrics based on that content.

Separating out the two the way you did provides an improvement, but you could go even further by giving WrappedCalculation an interface, say, IWrappedCalculation, and coding up ReportGenerator in terms of that interface. This way you would be able to reuse the report generator that you wrote for retirement portfolios to produce reports for portfolios of other kind - say, non-retirement portfolios, or combinations of several portfolios.

| improve this answer | |
  • Well... It's actually worse than that. The name WrappedCalculation suggests something more like a generalized mapping class. ReportGenerator might as well be called ReportUtility (I know how you guys hate "utility" classes). – Robert Harvey Dec 13 '17 at 17:09
  • @RobertHarvey I don't hate "Utility". Heck, I don't even hate "Helper." I do know more than one person who fits into utility-haters category (the naming nazis). – dasblinkenlight Dec 13 '17 at 17:12
  • This is kind of the way I was leaning. It's managing someone's 401k (making contributions, calculating the total using market returns, etc), but also generating various metrics about contributions. – keelerjr12 Dec 13 '17 at 17:23
  • @keelerjr12: Or, y'know, you could just make the calculation. All you need is a method for that. – Robert Harvey Dec 13 '17 at 17:24
  • 1
    In accordance with 'Tell, Don't Ask', I think it would even be better to inject a 401kReportGenerator into 401k using I401kReportGenerator and then the client could just say 401k.GenerateReport() and it would return a 401kReport object. I don't necessarily want to do a bunch of querying on the 401k. Then I could inject a 401kCalculator into the 401k as well and it would handle the responsibility of calculating the total. Then the 401k would be more of an API object with a narrow interface. – keelerjr12 Dec 13 '17 at 18:22
3

Yet another developer torpedoed by the SOLID principles.

Responsibility doesn't mean "do only one thing." It means "have only one reason to change," or more specifically, "This is the place to go to make modifications for this area of concern." Having more methods on your class doesn't necessarily mean you're violating SRP.

A repository doesn't have four responsibilities because it has Create, Read, Update and Delete methods. It has only one: data access.

The SOLID principles exist to suggest ways to improve your software's maintainability. It is maintainability you should be striving for, not slavish adherence to arbitrary principles.

| improve this answer | |
  • But surely, a repository is divvied up into other classes. I understand it doesn't depend on a single responsibility, but should depend on, like you said, a single reason to change. What about the example of two different actors, IRS and Employee? The IRS really only cares about a snapshot of the 401k (a report basically). I don't want the IRS reaching into my 401k and being able to manipulate it. However, I could pass them a 401KReport object that was generated using the 401k. – keelerjr12 Dec 13 '17 at 17:03
  • Those are entity classes, and yes, you have one for each entity. The repository itself is a single class, or perhaps a single class for each entity. My example still stands. – Robert Harvey Dec 13 '17 at 17:10
  • Or the scenario with Employee, Employer, and 401k Objects. Let's say that our 401k manages generating contribution reports, cumulative contribution reports, and calculates itself to return a total. Part of the business (the employer) would like to use a SpecializedList instead of just List; so the developer goes in and modifies how the CumulativeContribution function works, but accidentally breaks the CalculateTotal function(). Now the Employee class logs in and sees their 401k total miscalculated. Had we exposed a narrow interface, the report (for an audit let's say), – keelerjr12 Dec 13 '17 at 17:13
  • would have been generated by another class and would have been the only class touched. An employee may not care about their cumulative contributions; they just want to see what their total 401k is at. – keelerjr12 Dec 13 '17 at 17:14
  • There's a limit to how far you can take that narrowness. The cost of having extremely narrow classes is having large numbers of classes, which becomes its own maintainability problem. This is why SOLID isn't a design methodology; it's merely a set of principles. – Robert Harvey Dec 13 '17 at 17:22

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.