Update controller organization rules

[AELSchauer]

There is some confusion about whether or not to organize standard controller methods alphabetically or according to the Rails official order. I propose using the rails official order.

[jeromedalbert]

Is this the Rails official order? When I scaffold a new controller on a brand new Rails app, it looks like the order is: index, show, new, edit, create, update, destroy.

class BananasController < ApplicationController
  before_action :set_banana, only: %i[ show edit update destroy ]

  # GET /bananas or /bananas.json
  def index
    @bananas = Banana.all
  end

  # GET /bananas/1 or /bananas/1.json
  def show
  end

  # GET /bananas/new
  def new
    @banana = Banana.new
  end

  # GET /bananas/1/edit
  def edit
  end

  # POST /bananas or /bananas.json
  def create
    @banana = Banana.new(banana_params)

    respond_to do |format|
      if @banana.save
        format.html { redirect_to @banana, notice: "Banana was successfully created." }
        format.json { render :show, status: :created, location: @banana }
      else
        format.html { render :new, status: :unprocessable_entity }
        format.json { render json: @banana.errors, status: :unprocessable_entity }
      end
    end
  end

  # PATCH/PUT /bananas/1 or /bananas/1.json
  def update
    respond_to do |format|
      if @banana.update(banana_params)
        format.html { redirect_to @banana, notice: "Banana was successfully updated." }
        format.json { render :show, status: :ok, location: @banana }
      else
        format.html { render :edit, status: :unprocessable_entity }
        format.json { render json: @banana.errors, status: :unprocessable_entity }
      end
    end
  end

  # DELETE /bananas/1 or /bananas/1.json
  def destroy
    @banana.destroy!

    respond_to do |format|
      format.html { redirect_to bananas_path, status: :see_other, notice: "Banana was successfully destroyed." }
      format.json { head :no_content }
    end
  end

  private
    # Use callbacks to share common setup or constraints between actions.
    def set_banana
      @banana = Banana.find(params.expect(:id))
    end

    # Only allow a list of trusted parameters through.
    def banana_params
      params.expect(banana: [ :name, :weight ])
    end
end

[AELSchauer]

@jeromedalbert That's a good callout. I always assumed that the method order on the controller matched the routes order. https://guides.rubyonrails.org/routing.html#crud-verbs-and-actions

Personally, I like the routes order because it puts it into two groups: one where the resource ID is not needed (index, new, create) vs is needed (show, edit, update, destroy). And then within those two groups starts with the read, then then form, then followed by the action. The fact that the default ordering on the route vs the controller is different is weird.

@tute What do you think?

[tute]

Suggested change

	- Standard methods should be organized using the official Rails order: index, new, create, show, edit, create, destroy
	- Standard methods should be organized using the official Rails order: index, new, create, show, edit, update, destroy

On the other comment, I wouldn't mention public methods for controllers (assuming 98% of our cases are REST and already mentioned as standard, and the other 2% won't follow the general guides for whatever reason they need).

[AELSchauer]

Oop! Fixed

[tute]

+1 to Rails official order. As for method ordering on other objects I typically go from higher level to less relevant implementation details, which has subjectivity and is not as simple of a rule to describe, but results in easy to read classes, top to bottom, avoiding distractions with minor details in the middle.

[arzezak]

Should we reference https://github.com/rubocop/rails-style-guide?tab=readme-ov-file#macro-style-methods instead, as it seems more specific, hopefully leaving less room for debate?

[jeromedalbert]

I just read through this thread and learned that there is a Rails/ActionOrder cop that can automatically check and fix actions order, and it can be configured it with whatever order we want.

In #117, @joshsaintjacque mentions that some engineers like the idea of removing rules and using linters instead when possible. So I am wondering if we still want some written instructions in the guides, or if we should use that cop instead.

[joshmfrankel]

In #117, @joshsaintjacque mentions that some engineers like the idea of removing rules and using linters instead when possible. So I am wondering if we still want some written instructions in the guides, or if we should use that cop instead.

+1 to adding the rule for code based enforcement

[AELSchauer]

@jeromedalbert @joshmfrankel

So, I'm good with adding the cop, but I noticed in the BuoyRails repo that @joshsaintjacque disabled this cop a few months ago because there was a bug. I'm going to add the cop, but also keep the rules written in the guide

[jeromedalbert]

Looks good to me, thanks for looking into this!