Ruby on Rails Controllers: Best Structural Practices

I'm the lead developer at Taaalk. Right now my controllers are a bit of a mess, everything is included and they do way more than CRUD. Is that wrong or ok? I don't know. This Taaalk will hopefully teach me.
Freelance developer that just can’t stop enjoying Ruby on Rails
I’m a web developer with just a little over 10 years of experience. Almost 7 of those years have been spent writing Ruby on Rails. 

I currently work on a healthcare SaaS built with Rails and Angular.js. 
Follow this Taaalk

4 followers

945 views

Martijn Storck
17:59, 25 Jun 20
I’ve been a Rails dev for over ten years and I’ve tried all the stuff that came along that supposed to improve on the MVC model in Ruby on Rails, such as DCI and Service Objects for example. In the end I found none of them worth the extra hassle. Do you have an example from your codebase where ‘vanilla’ Rails controllers fall short in your view?
Joshua Summers
18:04, 25 Jun 20
Yes I do have one. For me it's not that the controller falls short... It's that I keep filling it up! It is doing the opposite of falling short, it's fat and big! Let me get an example up...
Joshua Summers
18:17, 25 Jun 20
This is my UsersController:
require 'action_view'

class UsersController < ApplicationController
  skip_before_action :authenticate_user!, only: :show
  before_action :set_user, only: [:show, :edit, :tlk_with_request, :destroy_tlk_with_me, :destroy_tlk_with_me_user_page, :destroy]
  
  def show
    ...
  end

  def edit
    ...
  end

  def update
    ...
  end

  def tlk_with_me
    ...
  end

  def tlk_with_request
    ...
  end

  def send_tlk_request
    ...
  end

  def destroy_tlk_with_me
    ...
  end

  def destroy_tlk_with_me_user_page
    ...
  end

  def destroy
    ...
  end

  def update_password
    ...
  end

  private

  def set_user
    ...
  end

  def set_user_tlks
    ...
  end

  def user_params
    ...
  end

  def user_params_twm_new_page
    ...
  end

  def user_twm_params
    ...
  end

  def password_param
    ...
  end

  def update_password_params
    ...
  end

  def tlk_request_user_params
    ...
  end

  def tlk_request_params
    ...
  end

  def create_tlk_request
    ...
  end

  def update_user_profile_with_tlk_request
    ...
  end

  def send_tlk_request_mail
    ...
  end

  def spkrs_update_after_user_update
    ...
  end
end
So you can see I've stripped out all of the methods so we can focus on structure.
It all works, but this is a CRUD+ controller...
Martijn Storck
18:37, 25 Jun 20
That is indeed a big controller.
First off: the actions. I always find it a good idea to base controllers on a single resource. It seems that there are a few methods in here that actually deal with talks instead of users (or maybe some intermediate join model). If your UsersController serves a single purpose (dealing with users), these talk methods should move to a TalkController or TalkRequestController.
Then: the _params. It’s good that you care about strong parameters, but this is really something you should think about as a global policy, not something on the controller level. For example: If a user is always allowed to change his name and password, why would you disallow those params outside of the update_password action? It’s better to keep these decisions in one place. Remember: strong params was mainly introduced to prevent users from doing stuff like changing their own ‘role’ or ‘admin’ attributte.
Finally is it possible that there’s some redundancy/repetition in the various set_ and update_ private methods?
Joshua Summers
20:14, 25 Jun 20
If your UsersController serves a single purpose (dealing with users), these talk methods should move to a TalkController or TalkRequestController.
Yes, I have a Talks (called Tlks for ease) Controller and a TlkRequestsController. I can see that I have put some stuff in this UsersController which should not be there, for example:
def tlk_with_request
  @tlk_request = TlkRequest.new()
  @title = "Request to Taaalk with #{@user.username}"
end
Should clearly be the `def new` of my TlkRequestsController, however there are some things which I find more confusing. Take:
def tlk_with_me
  current_user.update!(user_params_twm_new_page)
  redirect_to new_tlk_path
end

...

private

...

def user_params_twm_new_page
  params.require(:user).permit(:image, :username, :tlk_with_me)
end
Users have their standard details (handled by normal `create` and `update` methods in the controlller), and then can add a special :tlk_with_me rich_text attachment in certain places within the application (e.g. the Start a Conversation page). It's in these situations where I feel I need to break CRUD.
Before we get onto the params, which I think you've said some very interesting things about, how would you handle a situation like that?
Martijn Storck
06:44, 26 Jun 20
Great example. The `tlk_with_me` action updates a User record, so why not use the update action you already have? You could add an extra parameter to the request to decide where to redirect the user to. For example post your form to user_path(redirect_to: :new_talk) and use the following code:
def update
  user.update!(user_params)
  if params[:redirect] == "new_talk"
    redirect_to new_tlk_path
  else
    redirect_to user, notice: "Profile updated!"
  end
end
I choose to not include the actual path to redirect to in the param as a slightly paranoid precaution against things like CSRF/XSS attacks.
The _params method is also important in the example, you could just add the :tlk_with_me field to the user_params whitelist and use that, instead of defining another _params method. Consider the attack vector here: the only thing doing that enables is letting the user hack an additional 'tlk_with_me' field into another form and update their details from there. Not a security risk, so I'd say stick with one user_params. You can authorize the different fields in there based on user role (for later).
Joshua Summers
13:52, 28 Jun 20
Right, I see. So it seems like a theme is emerging: having wide roads for different forms of traffic. That probably sounds a bit vague, but what I mean is now the update action works for many different paths, and the _params method works for different paths too. Everything is a bit wider, but because of that there are less things, and the controller is neater.
I actually used the method you suggested already since you suggested it. You might notice that you now have note taking functionality:

image.png 15.7 KB

`notes` are attached to `Spkrs` (Tlk participants), so can be set when the Spkr updates. Instead of creating a new action in the controller I just added an if statement to the `update` method. 
So maybe flipping things backwards, are there any good reasons to break the CRUD design of a controller?
[P.S. feel free to get involved Hassanin.] 
Joshua Summers
21:32, 29 Jun 20
And in addition, in my Ruby on Rails world I am comfortable with controllers, and don't really have any good ideas of how to remove things from them.
For example 'modules' and 'helpers' - I don't really know what they are or when/how to use them either. It feels like I can get whatever I want done by bulking up my controllers with many public and private methods, but I always feel things are not done well, even if they work...
Maybe it would be sensible to discuss some of these concepts and the ideas around using them so we can add a few more solutions to the discussion.
Martijn Storck
06:52, 02 Jul 20
So maybe flipping things backwards, are there any good reasons to break the CRUD design of a controller?
In my opinion, once a controller is dedicated to exposing a RESTful resource you shouldn't think lightly about breaking that. However, it is perfectly fine to have 'helper' controllers for tasks that do not operate on a specific resource or maybe trigger some business logic.
Again, these are just my opinions. Rails leaves room to structure your app the way you feel fit. I like to stick close to the basics as explained in the guides, though, because it makes it easy to onboard new developers or revisit old code.
Martijn Storck
06:55, 02 Jul 20
Helpers are a more controversial topic. Some believe them to be an antipattern and prefer to use something like a presenter or even cell. Helpers have two distinct properties: they deal with presentation only and are only available in views. If you have methods that you'd like to use from multiple controllers you can use a Concern like that. Both Helpers and Concerns are implemented as modules; a Ruby primitive that allows groups of methods to be pulled into a class.
Joshua Summers
11:55, 15 Jul 20
However, it is perfectly fine to have 'helper' controllers for tasks that do not operate on a specific resource or maybe trigger some business logic.
So let's start with the 'helper' controller idea. Say you were building a simple blogging application with a Post model and a Comment model.
Let's say you were to add a feature to that that might need a 'helper' controller. What kind of feature might that be, and what methods would be in this new (non-RESTful?) controller?
Follow this Taaalk

4 followers

945 views

Start your own Taaalk, leave your details or meet someone to Taaalk with.