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

Fail fast on missing methods in state object #2099

Merged
merged 17 commits into from
Nov 17, 2015

Conversation

chrisroos
Copy link
Contributor

This supersedes PR #2071

The SmartAnswer::State object is an OpenStruct which means that it responds to undefined methods with nil. This can lead to hard to find bugs in next_node blocks (particularly larger ones, e.g. this one in pay-leave-for-parents) as a simple typo will mean that the program runs but doesn't return the desired result.

The main change in this PR overrides SmartAnswer::State#method_missing so that it raises a NoMethodError exception if we try to call a method that's not previously been defined.

# Behaviour before this change
>> state = SmartAnswer::State.new(start_node = nil)
>> state.foo
=> nil
>> state.foo = 'bar'
>> state.foo
=> 'bar'

# Behaviour after this change
>> state = SmartAnswer::State.new(start_node = nil)
>> state.foo
=> NoMethodError: undefined method 'foo' for SmartAnswer::State
>> state.foo = 'bar'
>> state.foo
=> 'bar'

I've also updated all Smart Answer flows where the code was relying on the old behaviour of the State object, by explicitly setting the variables to nil that are used later in the flow.

I started out with the commit titled "Implement State#method_missing" as the first commit in order to identify the flows that needed updating. I moved it to become the final commit once I was happy that I'd updated all flows.

Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Caught by the integration/regression tests.
Ensure the color variable is set on the state before trying to call it.
Ensure the accommodation_charge variable is set on the state before trying to
call it.
I'm planning to override `State#method_missing` so that it raises a
`NoMethodError` if you attempt to read an attribute value without first setting
it. We always want to be able to read `error` and `response` irrespective of
whether they've been set.

I initially tried using `attr_reader` but that means that `error` and `state`
are left out of the hash returned from `State#to_hash`. Setting them in this
call to `super` ensures that `#to_hash` works as expected.
So that it raises a NoMethodError unless the method being called is a writer
(i.e. ends in =).

This should help avoid errors in our code by failing fast if we try to access a
method that's not previously been set on the `State`.
chrisroos added a commit that referenced this pull request Nov 17, 2015
…n-state-object

Fail fast on missing methods in state object
@chrisroos chrisroos merged commit 011199a into master Nov 17, 2015
@chrisroos chrisroos deleted the fail-fast-on-missing-methods-in-state-object branch November 17, 2015 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant