Image interactors à la ben #949
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a pattern we hope to use for storing the business logic of controllers.
This demonstrates a demo of how the ImagesController could work using the interactor gem. There were some small changes I made here compared to the existing code: 1. We no longer preview on image creation. There isn't any point doing this as it won't succeed - as initial image is invalid 2. We round the number as part of cropping an image. This saves this frequently being off by 1 pixel, fixing this made tests easier.
Following feedback on the first draft of interactors for images this makes a number of changes: - Changes the class naming to be suffixed with interactor. As these are mostly named with verbs this is a bit weird as it sounds like they create an interactor, however it is consistent with most other app level naming patterns. - Makes clearer the properties in play on a context by delegating each of them to context in the interactor class. - Drops using an update_context method to set context at the end of an interactor and instead uses an approach where context properties are set in the call method. - Run an interactor in an explicit transaction rather than using the `Edition.find_and_lock_current` method. Since we're setting context.edition we'd still have a line of code so we don't really benefit from the yield. - Uses a suggestion from Ben to define each instance variable we use from a result in a controller, by using values_at method on the result object (once converted to a hash) - Rather than checking on failure of a result check on a particular problem, such as issues.
This replaces the use of context with setting and passing variables in the call method of the Update and Destroy Interactors. Although passing variables can be repetitious, in both classes I feel like it helps to convey the intention of eah method call better, with the compromise that setting the context must still be done as a final step. I've tried to limit the #call method so that all the work is done in private methods, which seems to have helped, as each line reads with a similar level of clarity and abstraction, comparable to our features. The context now mainly serves as an abort mechanism, as we no longer rely on it within or without the interactor classes.
This takes the Update and Destroy interactor even closer to the way we right features by using instance variables to avoid passing them between private methods. Using instance variables is more appropriate than attribute readers, as the attributes are not always well defined. Using instance variables takes us even further away from the use of the interactor context, which still provides an early abort mechanism. Arguably the context is just an extention of the instance variable feature provided by the Ruby language.
Well I didn't see this coming! This reimplement the Update and Destroy interactors using the business_process gem, which seems to be a better fit for the previous version of the code, which was largely avoiding the use of the interactor context; it was getting to the point where the context introduced more confusion, with little gain in functionality. The only place which has arguably suffered is that we have to manually short-circuit in case of failure, although doing this has turned out to remove the duplication around setting things like the edition and image revision on the context in both the success and faiure scenarios.
kevindew
force-pushed
the
image-interactors
branch
3 times, most recently
from
1c78552 to
5faf0bf
Compare
Merged
|
Superseded by #960 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://trello.com/c/vf6xQW7w/739-prototype-images-controller-with-interactors-pattern
To quote my final commit: "I didn't see this coming!". I've spread my tinkering, and my reasoning over several commits, each of which could be useful in its own right; the end result was kind of inevitable, but I hope I haven't strayed too far from the overall goal of making a net improvement :-).