current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

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?

share|improve this question
    
Just to clarify… those controller methods have empty bodies? –  200_success♦ Aug 12 '14 at 18:36
    
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? –  200_success♦ Aug 12 '14 at 18:37
2  
In those scenarios you simply create a single action with a param key. –  tokland Aug 12 '14 at 18:56
1  
Yes, all of those methods have empty bodies, which is why I'd like to change that –  Johnson Aug 12 '14 at 19:33

2 Answers 2

up vote 3 down vote accepted

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

Your code should 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
share|improve this answer
    
Thank you Uri Agassi. That's a perfect solution and it helped greatly simplify my code. –  Johnson Aug 18 '14 at 16:07
up vote 1 down vote

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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