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

Simplify date question #1935

Merged
merged 22 commits into from Sep 21, 2015

Conversation

Projects
None yet
2 participants
@chrisroos
Contributor

chrisroos commented Sep 10, 2015

This all came out of the work on the new HMRC Smart Answer (calculate-your-part-year-profits). I found the implementation of Question::Date to be somewhat more complicated than it needed to be. This pull request aims to simplify the class.

In summary, I've:

  • Removed the unused Question::Date#default method.
  • Simplified the implementations of Question::Date#from, #to, #default_day, #default_month, #default_year so that they can only be set by providing a block rather than by providing a block or a value argument.
  • Removed Question::Date#defaulted_day?, #defaulted_month? and #defaulted_year? and replaced their use with Date#default_day, #default_month and #default_year.
  • DRYed up calls to the blocks stored in @from_func and @to_func.
  • Restricted the type of exceptions we rescue in Question::Date#parse_input and #to_response. We were previously rescuing everything that inherits from StandardError, which isn't ideal as it means we might end up swallowing genuine exceptions.
  • Simplified the implementation of Question::Date#parse_input when it's parsing a date supplied as a hash.
@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Sep 10, 2015

Contributor

Other than my minor comments/suggestions, this LGTM 👍

Contributor

floehopper commented Sep 10, 2015

Other than my minor comments/suggestions, this LGTM 👍

@floehopper floehopper self-assigned this Sep 14, 2015

chrisroos added some commits Sep 9, 2015

Remove Question::Date#default
This was added in 1ac12b6 to allow a default
date to be specified for the maternity Smart Answer. It no longer appears to be
in use so I think it's safe to remove.

I've had to amend the _question_date partial so that it no longer calls
`question.default`. This shouldn't cause any problems as `question.default`
would have always been returning `nil`.
Always pass blocks to Question::Date#from
I want to be able to simplify Question::Date#from and hope that consistently
calling it with a block will help me do that.
Always pass blocks to Question::Date#to
I want to be able to simplify Question::Date#to and hope that consistently
calling it with a block will help me do that.
Always pass blocks to Question::Date#default_day
I want to be able to simplify Question::Date#default_day and hope that
consistently calling it with a block will help me do that.
Always pass blocks to Question::Date#default_month
I want to be able to simplify Question::Date#default_month and hope that
consistently calling it with a block will help me do that.
Always pass blocks to Question::Date#default_year
I want to be able to simplify Question::Date#default_year and hope that
consistently calling it with a block will help me do that.
Update vat-payment-deadlines-files.yml
This was necessary as I updated the use of `default_day` in the
vat-payment-deadlines flow.

Updated using:

    $ rails r script/generate-checksums-for-smart-answer.rb \
      vat-payment-deadlines
Remove calling of Question::Date#from with a date argument
We're not calling Question::Date#from with a date argument so we can simplify
the method implementation.
Remove calling of Question::Date#to with a date argument
We're not calling Question::Date#to with a date argument so we can simplify the
method implementation.
Remove calling of Question::Date#default_day with a date argument
We're not calling Question::Date#default_day with a date argument so we can
simplify the method implementation.
Remove calling of Question::Date#default_month with a date argument
We're not calling Question::Date#default_month with a date argument so we can
simplify the method implementation.
Remove calling of Question::Date#default_year with a date argument
We're not calling Question::Date#default_year with a date argument so we can
simplify the method implementation.
Remove Question::Date#defaulted_day?
The `#default_day` method will return either `nil` or the result of the block
stored in `@default_day_func` if set. Using this means that we can remove the
`#defaulted_day?` method.

This was added in 8b58286 but it's not clear
why the additional method might be necessary.
Remove Question::Date#defaulted_month?
The `#default_month` method will return either `nil` or the result of the block
stored in `@default_month_func` if set. Using this means that we can remove the
`#defaulted_month?` method.

This was added in 8b58286 but it's not clear
why the additional method might be necessary.
Remove Question::Date#defaulted_year?
The `#default_year` method will return either `nil` or the result of the block
stored in `@default_year_func` if set. Using this means that we can remove the
`#defaulted_year?` method.

This was added in 8b58286 but it's not clear
why the additional method might be necessary.
@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Sep 21, 2015

Contributor

I've just rebased this on master and force pushed to this remote branch.

Contributor

chrisroos commented Sep 21, 2015

I've just rebased this on master and force pushed to this remote branch.

chrisroos added some commits Sep 9, 2015

Add tests for Question::Date#range
The `DateQuestionPresenter#start_date` and `#end_date` methods rely on
`Question::Date#range` returning `false` when the range isn't defined. This
wasn't tested so I'm adding these so that I have confidence to make changes to
the `#range` method.
DRY up calls to @from_func and @to_func
The previous implementation of `#range` was essentially duplicating some the
behaviour of `#from` and `#to`. We can remove this duplication by reusing
`#from` and `#to`.

NOTE. I've had to switch from `and` to `&&` to avoid this method returning
`nil` in some cases.
Add tests for Question::Date#parse_input
I'm looking to make some small changes to this method and wanted some tests in
place first to give me some confidence.

There appears to be some overlap between these tests and some other tests in
this file that use `#transition` to indirectly test the `#parse_input` method.
I'm happy to live with that duplication for now although I may remove it later.
Avoid rescuing all StandardError exceptions in parse_input
I'm fairly confident that the intention is to raise an `InvalidResponse`
exception when the date can't be parsed. We raise that exception when we detect
an invalid date, or when `Date.parse` or `Date.new` raises an `ArgumentError`.
Now that I have tests in place around this method I'm able to make the `rescue`
statement stricter which should hopefully make the code fail fast if it's truly
used in exceptional circumstances.
Add tests for Question::Date#to_response
I'm looking to modify the behaviour of this method but wanted some tests in
place first to give me some confidence.
Avoid rescuing all StandardError exceptions in to_response
The only exception we're expecting from `parse_input` is `InvalidResponse`. I
think we should be treating anything else as an actual exception by not rescuing
it.
Simplify parsing of dates supplied as hashes
This is probably somewhat subjective but I find this implementation slightly
easier to understand.

NOTE. I've removed the "Please enter a complete date" error message as it wasn't
serving any purpose. This string would've become the `#message` of the
`InvalidResponse` instance which is used as a key to lookup the actual error
message in the YAML file associated with the Smart Answer. The "Please enter a
complete date" string isn't a key and so `NodePresenter#error` would've fallen
back to finding either the default error message for the question or the default
error message for all flows.
@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Sep 21, 2015

Contributor

I've force pushed again now that I've fixed the failures in date_question_test.rb.

Contributor

chrisroos commented Sep 21, 2015

I've force pushed again now that I've fixed the failures in date_question_test.rb.

@chrisroos chrisroos merged commit 540b514 into master Sep 21, 2015

1 check passed

default "Build #2930 succeeded on Jenkins"
Details

chrisroos added a commit that referenced this pull request Sep 21, 2015

Merge branch 'simplify-date-question'
PR #1935.

This all came out of the work on the new HMRC Smart Answer
(calculate-your-part-year-profits). I found the implementation of
`Question::Date` to be somewhat more complicated than it needed to be. This pull
request aims to simplify the class.

In summary, I've:

* Removed the unused `Question::Date#default` method.

* Simplified the implementations of `Question::Date#from`, `#to`,
  `#default_day`, `#default_month`, `#default_year` so that they can _only_ be
  set by providing a block rather than by providing a block or a value argument.

* Removed `Question::Date#defaulted_day?`, `#defaulted_month?` and
  `#defaulted_year?` and replaced their use with `Date#default_day`,
  `#default_month` and `#default_year`.

* DRYed up calls to the blocks stored in `@from_func` and `@to_func`.

* Restricted the type of exceptions we rescue in `Question::Date#parse_input`
  and `#to_response`. We were previously rescuing everything that inherits from
  `StandardError`, which isn't ideal as it means we might end up swallowing
  genuine exceptions.

* Simplified the implementation of `Question::Date#parse_input` when it's
  parsing a date supplied as a hash.

@chrisroos chrisroos deleted the simplify-date-question branch Sep 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment