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

Refactor SmartAnswer::Question::Base #2319

Merged
merged 17 commits into from
Mar 7, 2016
Merged

Conversation

floehopper
Copy link
Contributor

These changes were prompted by me preparing the ground for a new version of #2311.

There are still lots of improvements that could be made to this code, but I think the changes in this PR should make the work I want to do on automatic detection of permitted next nodes easier.

@floehopper floehopper force-pushed the refactor-question-base branch from c9116cb to f8c206b Compare March 1, 2016 11:25
@floehopper
Copy link
Contributor Author

I've just rebased this against master, added one extra commit on the end, and force-pushed. It is again ready for review.

@floehopper floehopper force-pushed the refactor-question-base branch from f8c206b to 6e26664 Compare March 5, 2016 11:35
@floehopper
Copy link
Contributor Author

I've just rebased this against master, added a couple of small commits, and force-pushed.

@floehopper
Copy link
Contributor Author

@chrisroos: Can you review this when you have a chance? I'm basing another branch on the changes in this one, so it would be good to get feedback on it as soon as possible. The changes in this one are all pretty minor.

@erkde
Copy link
Contributor

erkde commented Mar 7, 2016

Other than the question about the change from proc to lambda and how it effects next_node blocks that don't pass response, looks good.

Also, might be worth verifying that the existing documentation on next_node rules include any changes or restrictions introduced by this PR https://github.com/alphagov/smart-answers/blob/master/doc/next-node-rules.md

@floehopper
Copy link
Contributor Author

@erikse: Thanks. I'll have a look to see whether any of the docs need updating.

@floehopper
Copy link
Contributor Author

I've just pushed some updates/improvements to the docs for next_node as suggested by @erikse.

@floehopper
Copy link
Contributor Author

I'm going to rebase this against master and force-push in preparation for merging.

Nothing seems to be relying on the "writer" behaviour of this method.

Furthermore, I think that this kind of dual-purpose method (both reader and
writer) are confusing.

So by converting the method into an attribute reader, we're simplifying the
code.
This method does not appear to be used anywhere.
The name `default_next_node_function` confused me for two reasons:

1. The instance variable doesn't just store the *default* value; it also stores
non-default values.
2. It's more common to talk about methods and blocks in Ruby rather than
functions. In this case, the instance variable stores a reference to a block.
I don't think this method was adding anything to the code.
* Consistently use `ArgumentError` if `next_node` is called with incorrect
arguments.

* Add unit test coverage for the two possible errors.

* Add/improve error messages.
There's nowhere else these instance variables could've been assigned prior to
the class' constructor, so it doesn't seem necessary to include the `||` check
to see whether they are already assigned.
* Calls to `lambda` produce less surprising procs than calls to `proc` or
`Proc.new` (in terms of their return behaviour).

* Procs produced by calls to `lambda` are stricter about the number of arguments
they are passed and so we have to declare a dummy input argument like we do in
the constructor.

* We were already using `lambda` to set the default value of `@next_node_block`
in the constructor and it seems better to be consistent.
There are two ways that a `NextNodeUndefined` exception can be raised:

1. The call to `next_node` does not return a truth-y value
2. No call to `next_node` is made

The second of these was not being unit tested.
In the case where no call to `next_node` is made for a question, the code which
raises `NextNodeUndefined` relies on the default value for `@next_node_block`
returning a false-y value when invoked. This makes that more explicit.
I think this improves the readability, because the line is so long it's easy to
miss the conditional. Also it's consistent with the `if` statement immediately
below it.
I'm pretty sure we don't have any places where `next_node` is called multiple
times per question and I can't see any sensible justification for allowing it.

Constraining the DSL so that it can only be called once means that we can
simplify the code and make it easier to reason about.

With this guard condition in place, there's no longer any need to add the
`permitted_next_nodes` to any stored in a previous call.
Previously we were relying on detecting a side-effect of `next_node` having
been called previously. Now we rely on whether or not `@next_node_block` has
been set which is much more direct and explicit.
Also added full-stop to second sentence for consistency.
Most of these changes document behaviour that existed prior to this
pull request, although some of the error messages have changed slightly.

The only additional behaviour that was added in this pull request is the
check which prevents multiple calls to `next_node`.
@floehopper floehopper force-pushed the refactor-question-base branch from 3a59c2b to f441f7c Compare March 7, 2016 16:13
floehopper added a commit that referenced this pull request Mar 7, 2016
Refactor SmartAnswer::Question::Base
@floehopper floehopper merged commit 2f3d538 into master Mar 7, 2016
@floehopper floehopper deleted the refactor-question-base branch March 7, 2016 16:16
@erkde
Copy link
Contributor

erkde commented Mar 7, 2016

This looks good to me

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.

None yet

2 participants