search

rubocop / rails-style-guide

A community-driven Ruby on Rails style guide
http://rails.rubystyle.guide
6.48k stars 1.06k forks source link

7 Standard Controller Actions #290

Open mollerhoj opened 3 years ago

mollerhoj commented 3 years ago

Limiting public controller actions to the 7 standard actions prevents fat controllers and generally enforces better domain modeling, as demonstrated in this talk: https://www.youtube.com/watch?v=HctYHe-YjnE

I would like to see a rubocop that enforces that the 7 standard actions are only public methods in controllers.

This will work nicely with https://github.com/rubocop/rails-style-guide/issues/289

andyw8 commented 3 years ago

Another useful education link: http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/

pirj commented 3 years ago

  1. Can you please elaborate on the "fat controllers"? Having many actions in the controller is not the same as smudging the responsibility of the controller, which, from my perspective, makes it fat.

We strive towards never using any other actions in our controllers

And how is it going so far, what challenges you on this way?

  1. The title might mentioning RESTful in a sense that RESTful routes = 7 actions is a bit misleading. To my best memory, REST doesn't postulate exact actions, nor constrains them. It is Rails that creates 7 actions by default. Those seven actions are referred to as "resourceful" in the Rails guides. Also, in the same guide suggests to freely add more routes as you see fit:

You are not limited to the seven routes that RESTful routing creates by default. If you like, you may add additional routes that apply to the collection or individual members of the collection.

Not to mention the non-resourceful routes:

While you should usually use resourceful routing, there are still many places where the simpler routing is more appropriate. There's no need to try to shoehorn every last piece of your application into a resourceful framework if that's not a good fit.

  1. Is there evidence of using those exact seven (or less) actions per controller being beneficial?

  2. The promise of this style guide is:

A style guide that reflects real-world usage gets used, and a style guide that holds to an ideal that has been rejected by the people it is supposed to help

Is there evidence that among real-world Rails apps there's a tendency to stick to those seven default methods?

mollerhoj commented 3 years ago
  1. To my understanding, begin fat simply means that the class has many lines of code.
  2. Going great, we implement new controllers instead of adding functionality to the old ones. E.g. when some would make a sort or position action, we do Users::PositionController with an update action.
  3. I agree, I've changed the title.
  4. Refer to http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/ and https://www.youtube.com/watch?v=HctYHe-YjnE. While convincing, we'll probably never get actual evidence.. I suspect that's true for almost all guides here though.
  5. Hm.. I could run an analysis to count how many actions are standard vs non-standard? But actually, I find that the cop is valuable exactly because it is sometime not being followed, and that leads to a mess more often than not.

About non-resourceful routes: I agree that it would be silly to enforce this rule for very simple controllers. What I want to avoid is controllers were more and more custom actions are being added. How about we all the user to set a maximum number of custom actions in a controller? I'd say that any controller with more than 2 custom actions is a style violation.

pirj commented 3 years ago

For the reference, ROCA-style.

mollerhoj commented 3 years ago

For the reference, ROCA-style.

I can't seem to find the paragraphs relevant to this discussion in the link, can you help me out?

pirj commented 3 years ago

4.

Refer to http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/ and http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/.

I believe you mistakenly posted the same link (posted above) twice.

While convincing, we'll probably never get actual evidence..

Indeed, justifying a significant pivot can be hard, and it's hard to do in isolation. Still, if the guide recommends something, the "those two guys think it's the right approach" might not be sufficient.

Personally, I think there's a threshold when it makes sense to extract an action from the otherwise resourceful controller. E.g. /book/1/download from BooksController#download to Books::DownloadsController#show. It also highly depends on the logic inside the action. If there's something where a parameter would do, like ?order=desc, that's fine.

However, for e.g. /menu/items?drinks=soft vs /menu/items?vegeterian=true, it might be:

any controller with more than 2 custom actions is a style violation

This is reasonable in most cases. However, what if it's a controller with no base resourceful actions at all, and 3 custom actions?

What I'm trying to say is that applications differ a lot, and there are different programming styles and architecture patterns. I would not naturally strive to limit myself to 7 resourceful actions for just the sake of it.

5.

I find that the cop is valuable exactly because it is sometime not being followed, and that leads to a mess more often than not.

We are not discussing the cop here. We're deciding if a certain practice is widespread enough to be added as a guideline. If I'd have to guess, I'd say that the minority of the guidelines were born from cops paving the way. Some cops were surely born to enforce guidelines. And some cops and guidelines were born out of the frustration of code reviewers having to repeatedly vocalize design constraints.

ffast is a nice tool to parse and search in Ruby code. I'd suggest using it, but there might be better options out there.

mollerhoj commented 3 years ago

I believe you mistakenly posted the same link (posted above) twice.

You're right, I have fixed the link.

Indeed, justifying a significant pivot can be hard, and it's hard to do in isolation. Still, if the guide recommends something, the "those two guys think it's the right approach" might not be sufficient. True. They managed to convince me though so now we're 3 ;-)

Personally, I think there's a threshold when it makes sense to extract an action from the otherwise resourceful controller. E.g. /book/1/download from BooksController#download to Books::DownloadsController#show. It also highly depends on the logic inside the action. If there's something where a parameter would do, like ?order=desc, that's fine.

I agree, there's a threshold.

However, for e.g. /menu/items?drinks=soft vs /menu/items?vegeterian=true, it might be:

  • hard to abstract code out in a way that a single action processes both
  • there are distinct additional parameters that those expect, ?drinks=soft&sweet=false vs ?vegeterian=true&eggs=false&fish=false Adding separate /menu/items/vegeterian /menu/items/drinks endpoints makes sense. But this doesn't necessarily propagate to adding them as separate controllers.

any controller with more than 2 custom actions is a style violation

This is reasonable in most cases. However, what if it's a controller with no base resourceful actions at all, and 3 custom actions?

I would always prefer either:

I have never seen a case where I would prefer 3 custom actions to splitting into more controllers with standard actions. If such a case exists that would make a very convincing case for not doing the cop.

That's why I think this is a good idea for a guide - It's not obvious before you do it, but always it turns out to be the right thing to split up into more controllers and use standard actions.

What I'm trying to say is that applications differ a lot, and there are different programming styles and architecture patterns. I would not naturally strive to limit myself to 7 resourceful actions for just the sake of it.

Like a true believe, I would preach that not following ones natural urges, and restricting oneself to the 7 resourceful actions would lead to better, more idiomatic code.

We are not discussing the cop here. We're deciding if a certain practice is widespread enough to be added as a guideline. If I'd have to guess, I'd say that the minority of the guidelines were born from cops paving the way. Some cops were surely born to enforce guidelines. And some cops and guidelines were born out of the frustration of code reviewers having to repeatedly vocalize design constraints.

There's a bit of a Chicken and Egg problem here though - Having the cop would probably lead to more adoption of the pattern.

I've found using the standard actions to lead to better code. I don't have any evidence. I suggest we leave this thread here for a while and see if it gets attention from people with the same conviction as me. Otherwise, I guess it's too much of a fringe opinion to be included in the style guide.

ffast is a nice tool to parse and search in Ruby code. I'd suggest using it, but there might be better options out there.

Very interesting, I'll play around with this.

pirj commented 3 years ago

@mollerhoj Have you had a chance to digest real-world-rails? I've recently found it comes with a tool, bin/rwr that can be userful in your research.

andrey-skat commented 2 years ago

In curious search of that kind of cop I found this thread. I don't feel that I can fully involve in the discussion now. So I say only some points why we use only standard actions:

matteeyah commented 11 months ago

I agree with @mollerhoj that we should make this the recommended approach. My reasoning is a little bit different, though.

We've been able to greatly simplify a production Rails app just by separating stuff out into smaller, more focused controllers. This is essentially the idea behind limiting the number of actions to just the most basic CRUD ones. If the number of actions is limited, you're forced to think about resource operations in a way that requires separating them into sub-resources. The last sentence might be hard to grok without a concrete example:

Here's an example where custom actions are used:

class RecordsController
  def index
  end

  def update_positions
  end

  def bulk_destroy
  end
end

The problem with this is not the actions themselves, it's the instantiation and all auxiliary code required to make those actions work. They most likely forces a violation of https://rails.rubystyle.guide/#one-method. Here's a couple of reasons why I think this is problematic:

  1. Likely, you have a different way to instantiate the instance variables you want to use in this action.
  2. Secondarily, it's also plausible that you have different authorization logic and might have different logic for capturing analytics data, or something else that's auxiliary to the primary logic of the action.
  3. These things lead to one another and result in having more and more logic in your controller, adding up to the controllers not being skinny - https://rails.rubystyle.guide/#skinny-controllers.
  4. Having mixed approaches to dealing with this creates ambiguity for newcomers to the codebase. Consistency is valued more than spot quality, so it's unclear which approach should be taken where. Where do you draw the line between - "it's okay to add a custom action" and "separate it into a sub-resource controller"?
module Records
  class PositionsController
    def update
    end
  end
end

module Records
  class BulkController
    def destroy
    end
  end
end

Spacing things out solves all four problems I outlined above. I also toyed with the idea of being lenient on whether or not we should fundamentalistically (yes that's a word, I checked) adhere to this rule. My knee jerk reaction based on common sense was to be lenient, but that only lead to more confusion than the other option, so we adopted the fundamentalistic approach here.

This is a whole other are of discussion, but I'd like to briefly draw a parallel between controllers and models. It's idiomatic rails to separate code for one piece of functionality into a concern and include it in the main model. This is exactly the same as separating parts of the controller, specific to a subset of functionality into a separate controller.

Finally it's also related to the Rails doctrine. Even though both "Convention over Configuration" and "Push up a big tent" are part of it. "Convention over Configuration" is at second place and "Push up a big tent" is last. This is related to the discussion here, because we're discussing adopting a convention that has objective benefits, versus being lenient on it to not discourage other ways to approach the problem.

I'm strongly opinionated that we should implement this cop. I'm not excluding the possibility that there's certain cases where this might not be practical, but that's exactly why we have the ability to disable a cop.

matteeyah commented 11 months ago

@koic Are we looking to reach a consensus here in order to be able to move forward with the associated cop (https://github.com/rubocop/rubocop-rails/pull/827)?