5
\$\begingroup\$

One of my rails controllers is horribly overcrowded with a bunch of methods that link to static web pages.

Controller

def adventure
end

def cooking
end

def dancing
end

def programming
end

def reading
end

def running
end

def sports
end

def writing
end

For each of the above actions I have seperate view files and routes defined for them.

View Files

adventure.html.erb
cooking.html.erb
dancing.html.erb
programming.html.erb
reading.html.erb
running.html.erb
sports.html.erb
writing.html.erb

Routes

match '/adventure', to: 'pages#adventure', via: 'get'
match '/cooking', to: 'pages#fighting', via: 'get'
match '/dancing', to: 'pages#first_person_shooter', via: 'get'

match '/programming', to: 'pages#programming', via: 'get'
match '/reading', to: 'pages#reading', via: 'get'
match '/running', to: 'pages#running', via: 'get'
match '/sports', to: 'pages#sports', via: 'get'
match '/writing', to: 'pages#writing', via: 'get'

It is quite embarrassing to have code that horrible. Is there any way I can optimize that?

\$\endgroup\$
  • \$\begingroup\$ Just to clarify… those controller methods have empty bodies? \$\endgroup\$ – 200_success Aug 12 '14 at 18:36
  • \$\begingroup\$ What determines the existence of those routes? The fact that a similarly named method exists on the Pages controller? The fact that a similarly named .erb file exists? \$\endgroup\$ – 200_success Aug 12 '14 at 18:37
  • 2
    \$\begingroup\$ In those scenarios you simply create a single action with a param key. \$\endgroup\$ – tokland Aug 12 '14 at 18:56
  • 1
    \$\begingroup\$ Yes, all of those methods have empty bodies, which is why I'd like to change that \$\endgroup\$ – Johnson Aug 12 '14 at 19:33
7
\$\begingroup\$

You don't need to define empty actions in your Controller.

Your code should work even if you delete all the empty actions, leaving you only with the views and routes:

page_controller.rb:

class PageController < ApplicationController
end

views:

adventure.html.erb
cooking.html.erb
dancing.html.erb
programming.html.erb
reading.html.erb
running.html.erb
sports.html.erb
writing.html.erb

routes:

match '/adventure', to: 'pages#adventure', via: 'get'
match '/cooking', to: 'pages#cooking', via: 'get'
match '/dancing', to: 'pages#dancing', via: 'get'

match '/programming', to: 'pages#programming', via: 'get'
match '/reading', to: 'pages#reading', via: 'get'
match '/running', to: 'pages#running', via: 'get'
match '/sports', to: 'pages#sports', via: 'get'
match '/writing', to: 'pages#writing', via: 'get'

or, if you really want things super DRY:

%w(adventure cooking dancing programming reading running sports writing).each do |page|
  match "/#{page}", to: "pages##{page}", via: 'get'
end
\$\endgroup\$
1
\$\begingroup\$

An possible solution is use just one action in the controller so yo can do the following:

config/routes.rb

get '/:static_page', to: 'pages#show', constraints: {static_page: /\A(adventure|cooking|dancing|programming)\z/ }

Be aware that this route should be the last route, also note the constraint to prevent errors trying to render pages that does not exists

Then in you controller just def show method

app/controllers/pages.rb

def show
  render params[:static_page]
end

Then when you need to add a new page just add the page name in the routes constraint and add the respective view

\$\endgroup\$
  • \$\begingroup\$ Be aware that this changes your routing helpers. \$\endgroup\$ – user3162553 Jan 4 '17 at 1:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.