Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move calculators into flow directories #2558

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
3 participants
@floehopper
Copy link
Contributor

commented May 31, 2016

Description

The distinction between lib/smart_answers and lib/smart_answer_flows was (originally) intended as making a distinction between flow-specific ("application") code and generic ("library") code.

Although I don't much like the way this is "implemented" in terms of the current directory structure, I do think it's a useful concept. I also think there is value in keeping all the files relating to a specific flow close together - partly because I think of an individual flow as a mini application in its own right.

However, there are currently a bunch of flow-specific classes under lib/smart_answers/calculators. This PR moves these "calculator" classes into the relevant flow's directory in order that all the files relating to a specific flow are in one place.

For "calculator" classes used in multiple flows, I would envisage moving them to the lib/smart_answer_flows/shared directory c.f. lib/smart_answer_flows/shared/minimum_wage.rb.

I should add that the reason I actually started working on this, is because I'm about to start moving the remaining flows towards the new style approach. This will (at least in the short term) mean introducing more flow-specific "calculator" classes and it started feeling increasingly odd to me that these are all some way away from the flows to which they relate.

Example

lib/smart_answer/calculators/commodity_code_calculator.rb -> lib/smart_answer_flows/additional-commodity-code/calculator.rb

SmartAnswer::Calculators:CommodityCodeCalculator -> SmartAnswer::AdditionalCommodityCodeFlow::Calculator

This leaves us with the following directory structure:

lib
└── smart_answer_flows
    ├── <flow-name>.rb # flow ("controller")
    └── <flow-name>
        ├── <flow-name>.govspeak.erb # start page template
        ├── calculator.rb # policy/rules/calculations ("model")
        ├── questions
        │   ├── question_1.govspeak.erb # question page template
        │   └── question_2.govspeak.erb # question page template
        └── outcomes
            ├── outcome_1.govspeak.erb # outcome page template
            └── outcome_2.govspeak.erb # outcome page template

See the individual commits for more examples.

Scope

There are many things wrong with the project's directory structure and name-spacing in the app, but I do not plan to fix them all in this PR. The aim of this PR is to improve matters in a good direction by taking a relatively small step. I'm not suggesting that the directory structure implemented in this PR is without flaws, just that it is an incremental improvement on what we have at the moment.

Future plans

The following directory structure illustrates the direction in which I'm heading. However, none of this is set in stone and I don't think this PR ties us into any particular approach. While I think it's useful to discuss our plans so that we're all on the same page, I'm slightly wary about thinking too far ahead.

lib
└── smart_answer_flows
    └── <flow-name>
        ├── models
        │   └── policy.rb
        ├── controllers
        │   └── flow.rb
        └── views
            ├── start.govspeak.erb
            ├── questions
            │   ├── question_1.govspeak.erb
            │   └── question_2.govspeak.erb
            └── outcomes
                ├── outcome_1.govspeak.erb
                └── outcome_2.govspeak.erb

For what it's worth, here are some of the other many things that are wrong with the directory structure and name-spacing:

  • Directory structure not mirroring namespaces (breaks Rails auto-loading).
  • Directory names with hyphens instead of underscores (also breaks Rails auto-loading).
  • Some generic ("library") code is under the app directory and some is under the lib directory.

floehopper added some commits May 31, 2016

Move calculator to flow directory - calculate-agricultural-holiday-en…
…titlement

This calculator is specific to the calculate-agricultural-holiday-entitlement
flow and so should be co-located.

* SmartAnswer::Calculators:AgriculturalHolidayEntitlementCalculator
  -> SmartAnswer::CalculateAgriculturalHolidayEntitlementFlow::Calculator

* SmartAnswer::Calculators::AgriculturalHolidayEntitlementCalculatorTest
  -> SmartAnswer::CalculateAgriculturalHolidayEntitlementFlow::CalculatorTest

For some reason the calculator was not in the regression test checksums file, so
I've added it in the new location.
Move calculator to flow directory - additional-commodity-code
This calculator is specific to the additional-commodity-code flow and so should
be co-located.

* SmartAnswer::Calculators:CommodityCodeCalculator
  -> SmartAnswer::AdditionalCommodityCodeFlow::Calculator

* SmartAnswer::Calculators::CommodityCodeCalculatorTest
  -> SmartAnswer::AdditionalCommodityCodeFlow::CalculatorTest

Since these changes do not affect the behaviour and all the regression tests
pass, I've updated the checksums with the new location of the calculator.
Move calculator to flow directory - childcare-costs-for-tax-credits
This calculator is specific to the childcare-costs-for-tax-credits flow and so
should be co-located.

* SmartAnswer::Calculators:ChildcareCostCalculator
  -> SmartAnswer::ChildcareCostsForTaxCreditsFlow::Calculator

* SmartAnswer::Calculators::ChildcareCostCalculatorTest
  -> SmartAnswer::ChildcareCostsForTaxCreditsFlow::CalculatorTest

Since these changes do not affect the behaviour and all the regression tests
pass, I've updated the checksums with the new location of the calculator.
Move calculator to flow directory - calculate-your-child-maintenance
This calculator is specific to the calculate-your-child-maintenance flow and so
should be co-located.

* SmartAnswer::Calculators:ChildMaintenanceCalculator
  -> SmartAnswer::CalculateYourChildMaintenanceFlow::Calculator

* SmartAnswer::Calculators::ChildMaintenanceCalculatorTest
  -> SmartAnswer::CalculateYourChildMaintenanceFlow::CalculatorTest

Since these changes do not affect the behaviour and all the regression tests
pass, I've updated the checksums with the new location of the calculator.
Move calculator to flow directory - help-if-you-are-arrested-abroad
This calculator is specific to the help-if-you-are-arrested-abroad flow and so
should be co-located.

* SmartAnswer::Calculators:ArrestedAbroad
  -> SmartAnswer::HelpIfYouAreArrestedAbroadFlow::Calculator

* SmartAnswer::Calculators::ArrestedAbroadCalculatorTest
  -> SmartAnswer::HelpIfYouAreArrestedAbroadFlow::CalculatorTest

Since these changes do not affect the behaviour and all the regression tests
pass, I've updated the checksums with the new location of the calculator.
Move calculator to flow directory - calculate-your-holiday-entitlement
This calculator is specific to the calculate-your-holiday-entitlement flow and
so should be co-located.

* SmartAnswer::Calculators:HolidayEntitlement
  -> SmartAnswer::CalculateYourHolidayEntitlementFlow::Calculator

* SmartAnswer::Calculators::HolidayEntitlementTest
  -> SmartAnswer::CalculateYourHolidayEntitlementFlow::CalculatorTest

Since these changes do not affect the behaviour and all the regression tests
pass, I've updated the checksums with the new location of the calculator.
Move calculator to flow directory - landlord-immigration-check
This calculator is specific to the landlord-immigration-check flow and so should
be co-located.

* SmartAnswer::Calculators:LandlordImmigrationCheckCalculator
  -> SmartAnswer::LandlordImmigrationCheckFlow::Calculator

* SmartAnswer::Calculators::LandlordImmigrationCheckCalculatorTest
  -> SmartAnswer::LandlordImmigrationCheckFlow::CalculatorTest

Since these changes do not affect the behaviour and all the regression tests
pass, I've updated the checksums with the new location of the calculator.
@pmanrubia

This comment has been minimized.

Copy link
Contributor

commented May 31, 2016

Hi @floehopper, a big 👍 to the future plans section. I am looking forward to having the new folders structure. I also agreed with tackling it step by step. Great work!

Very minor thing. I am not a fan of having prefixes unless they really add value (especially when they share the same name as the application): smart_answer_flows would be very similar to flows.

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

@pmanrubia: Thanks for your comments.

Very minor thing. I am not a fan of having prefixes unless they really add value (especially when they share the same name as the application): smart_answer_flows would be very similar to flows.

I agree. I'm not convinced that the above directory structures should end up in lib/smart_answer_flows (or even anywhere under lib!), but that's another thing that we can think about separately.

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2016

@leenagupte: Does this PR make what I'm suggesting any clearer? I'd really appreciate your input.

@leenagupte

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2016

What I find confusing is having both smart_answer and smart_answer_flows directories.

I keep expecting all the controller code specific to a smart answer to be smart_answers not in smart_answer_flows

I'd rather the future looked like this:

lib
└── smart_answers
    └── <smart-answer-name>
        ├── models
        │   └── policy.rb
        ├── controllers
        │   └── flow.rb
        └── views
            ├── start.govspeak.erb
            ├── questions
            │   ├── question_1.govspeak.erb
            │   └── question_2.govspeak.erb
            └── outcomes
                ├── outcome_1.govspeak.erb
                └── outcome_2.govspeak.erb

If the current smart_answers directory is to only contain generic code, it should probably be renamed to something that reflects that e.g. "helpers".

So, to do this in baby steps, I would first:

  1. Rename smart_answer to helpers
  2. Rename smart_answer_flows to smart_answers
  3. Move the calculators from helpers to smart_answers/models/<smart_answer_name>

So in the interim we would have this directory structure:

lib
└── helpers # contains generic code
└── smart_answers
    ├── <flow-name>.rb # flow ("controller")
    └── <flow-name>
        ├── <flow-name>.govspeak.erb # start page template
        ├── models
        │   └── calculator.rb # policy/rules/calculations ("model")
        ├── questions
        │   ├── question_1.govspeak.erb # question page template
        │   └── question_2.govspeak.erb # question page template
        └── outcomes
            ├── outcome_1.govspeak.erb # outcome page template
            └── outcome_2.govspeak.erb # outcome page template
@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2016

@leenagupte:

Thanks for taking the time to explain your thinking. It sounds as if we agree on the rough directory structure for an individual flow, which is good to know.

What I find confusing is having both smart_answer and smart_answer_flows directories.

I agree that this is confusing. I think it relates to what @pmanrubia was saying in his comment above. In my reply I said:

I'm not convinced that the above directory structures [for an individual flow] should end up in lib/smart_answer_flows (or even anywhere under lib!), but that's another thing that we can think about separately.

Hopefully you can see by my reply that like you I'm also definitely not convinced that the individual flow directory structures should end up in lib/smart_answer_flows.

The problem that I've started trying to address in this PR is that files for a single flow are currently not close together. I feel as if what I've done so far moves us towards the directory structure for a single flow discussed above. I think the idea of the sub-directories like models, views & controllers can be a later step.

If it's ok with you, I'd like to treat the issue of having both lib/smart_answer and lib/smart_answer_flows as a separate problem. I don't feel as if anything in this PR makes that situation any worse than it already is.

So I think I'm inclined to press on with the current approach, although I've now started a bunch of other work and may not come back to this for a while. I'm going to close this PR for now, but feel free to add more comments to it if you have any more feedback.

@floehopper floehopper closed this Jun 1, 2016

@leenagupte

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2016

@floehopper My main objection is putting model code (calculator.rb # policy/rules/calculations ("model")) in the same folder and at the same level as the templates. At the very least one or both should be separated into a sub folder.

@barrucadu barrucadu deleted the move-calculators-into-flow-directories branch Apr 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.