design_question.md

Design Problem

I'm struggling to balance encapsulation with managing side effects in a specific scenario described here.

Classes

• PriceSettings < ActiveRecord::Base (side-effect-based class methods, value-based instances) stores user parameters for price calculations
• APIClient (adapter object) fetches statistics from an external API to use in price calculations
• PriceMatrix (value object) creates a matrix of prices for a range of days and quantities
• PriceAdjuster (value object) determines a total price adjustment for a specific day and quantity by combining several adjustments
• Adjustment::A (value object) calculates an adjustment factor from PriceSettings data only
• Adjustment::B (value object) calculates an adjustment factor from PriceSettings data, plus data from the APIClient
• GeneratePrices action (shell/integration object) Create and render a PricingMatrix

Hierarchy

• GeneratePrices loads a PriceSettings
• GeneratePrices creates PriceMatrix
  • PriceMatrix(PriceSettings, ranges) creates a PriceAdjuster for each combination of date/quantity range values
    • PriceAdjuster(PriceSettings, date, quantity) creates several Adjustment objects that require different inputs
      • Adjustment::B(PriceSettings, date)
        • Needs values from Price Settings
        • Needs API data from APIClient for the specific date

Problem

From an encapsulation perspective, it makes sense for Adjustment::B to do its own API calls in order to fulfill its specific responsibilities. Other objects shouldn't do Adjustment::B's job for it. But this could be wildly inefficient, as the API calls would be scoped at the lowest level and therefore, might need to be repeated for each date/quantity combination.

From a value object perspective, it makes sense for Adjustment::B NOT to do its own API calls because value objects shouldn't have side effects. This could also solve potential efficiency issues because it would do a larger API call at the top level of the heirarchy and pass that data through the stack. But this would also seem to violate encapsulation since GeneratePrices would know about data that Adjustment::B needs and therefore would be passing all that data through PriceMatrix to PriceAdjuster, then through PriceAdjuster to Adjustment::B. Do you agree?

How would you structure things differently to avoid both the issue of value objects having side effects and the encapsulation problems?

Potential Solution

• Have GeneratePrices query Adjustment to get the aggregate data that all the individual adjustments need, so each Adjustment can specify its own dependencies that GeneratePrices can look up (via a new adapter) and pass in.
• Rather than individually passing in a PriceSettings, API data for Adjustment::B, and whatever data other adjustments need, wrap all this up in a new AdjustmentData value object container.
• Each Adjustment, having provided keys for the data it depends on (e.g. :pricing_input or :"weekly_historical_prices(2015:5)"), can look in this AdjustmentData container for data it needs by these keys. Multiple adjustments can use the same keys to share data.

Clarifications:

• There are many Adjustment classes, not just 2
• All of them will eventually need to access various API data

This is a well-stated example of design tensions, thank you for writing it. The work that went into my 2015 RailsConf talk was trying to formalize my intuition for why I don't like the idea of Adjustment::B being "encapsulated" by having it talk to the API, that felt messy and unpredictable, and I found side effects as the underlying cause of my concern.

I think the answer is probably lurking in your description here:

• Adjustment::B (value object) calculates an adjustment factor from PriceSettings data, plus data from the APIClient

This "data from the APIClient" doesn't have an explicit name and representation, there is a "primitive obsession" design smell. You probably didn't notice yet because your code grabs that raw data and processes it in one place, so it seems weird to write a whole class for a thing that only exists for a line or two. Maybe it's even harder to spot because you don't even name a variable; your code could use it directly from the response like total = response[:prices][current_year] * adjustment_factor.

Based on that name I'd look at where it fits into your hierarchy - it is an aspect of a PriceMatrix, a PriceAdjuster, or (seems most likely) Adjustment::B that GeneratesPrices (which I think is what pulls data from APIClient?) would pass into the appropriate constructor of a value object.

Current Direction

New Classes

• Create a new AdjustmentData value object that wraps a Hash containing all data that any Adjustment might need, including the PriceSettings object. The keys are simple and need not contain parameters, as suggested above in the Potential Solution.
• Create a new ScopedAPILookup(PriceSettings) adapter that makes calls to APIClient scoped using the provided settings (we do not have enough information to do the API calls without the PriceSettings). ScopedAPILookup can aggregate data over multiple API calls to provide data formatted in a way that our app wants to consume it. It provides methods that make API calls to look up particular data sets, and one method that returns an AdjustmentData value object that aggregates all data sets that any Adjustment would want.

Sequence

1. GeneratePrices looks up PriceSettings using the id in params.
2. GeneratePrices uses ScopedAPILookup to generate an AdjustmentData object containing all data needed by PriceMatrix, PriceAdjuster, and each Adjustment.
3. GeneratePrices instantiates PriceMatrix(adjustment_data, ranges)
   • PriceMatrix instantiates many PriceAdjuster(adjustment_data) over the ranges, which each can use adjustment_data to do calculations for each Adjustment.
4. GeneratePrices renders the price matrix

In simpler terms:

1. Look up the scope of the query
2. Query data from the API for that scope
3. Generate prices using that data
4. Display those prices

Result

• PriceMatrix, PriceAdjuster, and each Adjustment remain value objects with no side effects (API or DB calls).
• There is coupling between ScopedAPILookup and the Adjustment objects, in that ScopedAPILookup must provide all data that every Adjustment needs. They will be coupled using a key that must match between the two classes, which will make dependencies easy to trace.
• ScopedAPILookup can optimize API queries by batching and caching.