Accessing the view's #render method from a service object

Ruby on Rails
1

Hi guys,

I’ve got the following code I want to extract from a view, as I feel it’s violating Tell Don’t Ask (as it’s asking @manager a question and then using @manager’s state):

- if @manager.can_create_tags?
    = render("question_tags/management_links", question: @manager.question,
      tag: tag)

@manager is an instance of a service object PORO that I’ve extracted. I could just try including ActionView::Helpers if that’s a way forward, but was wondering if anyone had solved this problem before. I know Draper jumps through a lot of hoops to get access to helpers, passing through the context from the Controller. I guess I could just extend my class from Draper::Decorator, but that also feels a bit hacky too.

Any thoughts would be welcome, thanks!

2

I actually ignore Tell, Don’t Ask in the views all the time. If you’re following MVC, it’s tricky to not ask questions of your models in the view.

We did a Weekly Iteration episode about this, and I think we covered why you might not follow it in the view layer.

3

Thanks for the input @benorenstein! Yeah, I rewatched the video and tried my first exercise on it this evening. I did go with extending from Draper::Decorator if only to see where all my dependencies are - @manager has quite a few concerns with lots of delegation going on to paper over LOD violations (it’s worth a review on it’s own, here it is: https://gist.github.com/11440146) and so this particular code could well end up being put back into the view when I’ve got everything tidied.

4

Thanks for providing a code sample. Just curious - any reason why you went for Ruby’s Forwardable module as opposed to using Rails’ in built delegate method?

I hadn’t seen the underscores in front of the ivars before either which was interesting. From what I’ve read you’re using them to signify private ivars only to be used within the class?

5

@ralphwintle Hey, I used Forwardable as it allows me to set a method alias (delegating task_id to task.id, for example), maybe some variant of delegate allows this too, I dunno.

Yep, you’re right about that, it helps reinforce that I’m following Sandi Metz’s single instance variable rule. It might help to see the related Controller: question_tags_controller.rb · GitHub.

I do need to move the delegations out of this class, but I’m not sure how to solve that whilst still following the single instance variable rule.

6

Cool, so I had a look at the Delegate docs. You can use delegate as

delegate :type to: :task, prefix: :task

which gives you a task_type method similar to what you had.

I also found out if there’s a relation in place, Rails actually gives you a task_id method without having to use delegate, which is pretty neat.

I think you’re right - I’m not sure the delegated methods make it easier to understand what the responsibilities of that class are. Rather than delegate, perhaps a more standard OOP approach would be to inject those dependencies so you can use them within the class? I’m not sure, I need to spend more time looking at it but gotta get back to work!

1 Like
7

@ralphwintle Nice. This app uses an API so I can’t use ActiveRecord relations.

I’ve been thinking that would be best, but I’m trying to figure out the best way to do that from the Controller. It doesn’t help that a lot of the dependencies are there simply to render the breadcrumbs either. If you could help me try some ideas that’s be really awesome!

8

@ralphwintle Another nice side effect of using Forwardable means I’m not depending on Rails, which is pretty clean and has the side effect of allowing the spec to run without booting Rails, which would be faster even though I use Spring. The Jim Weirich talk on Hexagonal Architecture demonstrates this.

1 Like
9

Your QuestionTagsManager is kind of a god object. It fetches stuff from external resources, mutates certain things, destroys tags, renders view elements and does some data crunching for the view. Since you’re using it for every controller action, that’s just bound to happen. God objects are a violation of the Interface Segregation principle, btw.

Second note: question_tags_manager.rb · GitHub What is going on here? Do these listings not need to have access to the tags? It seems kinda odd constructing them without passing a list of things into them.

Third note: You have plenty of constant references in QuestionTagsManager. I count 8 of them. This class will not only be hard to test, but also violates the Open/closed principle. If you want to change the way questions or tags are found you need to change this class even though the render logic doesn’t actually care where the data comes from.

Fourth note: You don’t need to follow Sandi’s rule religiously.

As a start, I would delete QuestionTagsManager and push everything back into the controller in order to find a better abstraction. A page object (which gets the dependencies passed in and doesn’t query for them) could be used for these render calls for example. The breadcrumbs should live in it as well. A page object would also obey the single instance rule, because it has everything the view needs.

1 Like
10

@timhabermaas You’re absolutely right about the God Object, I wanted to move all the dependencies to one place so I could see them all and then work gradually to a better design. This is definitely not finished yet!

I’m wondering how best to inject the dependencies into a Page/Presenter object. It’d be awesome if you could help sketch out a few ideas to get me started :smile:

11

A pattern I use when a PRO needs to interact with rails is inject the needed object as a lambda:
(I didnt over normalize the render params, but they could be)

### in the view
= @manager.render_management_links(-> {
  render("question_tags/management_links", question: @manager.question, tag: tag) })

### in the object
def render_management_links(renderer)
  return unless can_create_tags?
  renderer.call
end

This gets rids of the need for your PRO to know &^ about rails and still get its job done via TDAP. It also makes the boundary stupid easy to mock and test in a unit.

Some people hate this style, but I am not afraid of the principle that in good OO, everything happens somewhere else.

And yep, anything with Manager in it’s name…

1 Like
12

@Dreamr Thanks for your input! I’ve moved the conditional back into the view for now as there’s a few other problems that need sorting with this class first. And yes, calling it Manager was bound to attract loads of responsibilities but, as the original controller wasn’t written by me (it was a junior colleague), I moved all the dependencies to one place in order to start breaking it apart again, if that makes sense at all :laughing:

13

It does. I “shovel” code around, and change names constantly. I call it “polishing the ruby” :smile:

Instead of decomposing your manager object, take EACH of it’s methods, and extract them into a PRO. So something like…

class Manager
...
  def  notify_subscriber_of_something(*params)
    ...
    ...
    ...
    ...
    ...
    ...
    ...
    ...
    ...
    ...
    ...
    ...
    ...
    ...
    ...
  end
...
end

becomes

class Manager
...
  def  notify_subscriber_of_something(*params)
    SubscriberNotifier.queue(:something, *params)
  end
...
end
14

@Dreamr Haha I’m stealing that! Thanks again for your input!

I’m wondering about the balance between having references to constants though, I’d like to try something with dep injection maybe? I need to have a play and see what works :smile:

15

steal away brotha. that one is mine, however I stole 90% of the good shit I know or use daily. :smile:

1 Like
16

You reduce reuse and testability tremendously by referencing class constants in your code. I try to avoid it.

The Dependency-Inversion principle is usually a good rule to follow[1]. Referencing MySQLAdapter[2] from your User entity is obviously bad, but referencing User from the RegisterUser service isn’t too bad.

[1]

A. High-level modules should not depend on low-level modules. Both should depend on abstractions.
B. Abstractions should not depend on details. Details should depend on abstractions.

[2] Calling Post.find_by_user_name is not much different btw!

17

Like so for example?

task = Task.find params[:id]
@page = TaskPage.new(UserPresenter.new(current_user), TaskPresenter.new(task), CommentsPresenter.new(task.comments))

This is a simplified version, of course. If your page becomes more complex you might want to compose it of several other page objects (aka widgets). Looking at the natural hierarchies of the DOM might be a decent hint for a page object graph.

That being said: Going overboard with this page object thing probably isn’t worth it.

1 Like
18

@timhabermaas Thanks, I think I get the idea, I’ll try it out!

I could look at something like GitHub - trailblazer/cells: View components for Ruby and Rails. but that seems like a lot of effort to keep to a strict component-based architecture. My main problem is that the design dictates that the page maintain a lot of context. In this case a set of Tags which are on a Question which has a Task which has a Project. And I can’t use AR relation niceties because my app is using an ActiveAttr/ActiveModel API.

19

Hi @timhabermaas, @Dreamr

I’ve tried to extract a Repository object that my page content and breadcrumbs can use. I’ve tried to use dependency injection, could you guys have look to see if I’ve got the right idea? Here’s the gist: repo_dep_injection.rb · GitHub