5

I would like to ask if i should keep the empty methods in my controller (Its a question about code style):

before_action :set_project, only: [:show,:new]

  def show
  end

  def new
  end

Should i keep it like this or simpy remove show and new action

class ProjectController < ApplicationController
before_action :set_project

def index
#indexaction
end

def create
#createaction
end

is it more Railish way? Rails Styleguide doesnt indicate any sollution to it, only that:

def show
end

is better than

def show; end
Sravan
  • 18,467
  • 3
  • 30
  • 54
Hoggie
  • 174
  • 3
  • 12

3 Answers3

9

If you're not defining any data in those methods, you can remove them.

Rendering By Default:

Rails automatically infers the view from action names in routes.

If you're not defining data (only to be done in the controller), you'll can rely on the view being loaded without the action being present:

You've heard that Rails promotes "convention over configuration". Default rendering is an excellent example of this. By default, controllers in Rails automatically render views with names that correspond to valid routes. For example, if you have this code in your BooksController class:

class BooksController < ApplicationController
end

And the following in your routes file:

resources :books

And you have a view file app/views/books/index.html.erb:

<h1>Books are coming soon!</h1>

Rails will automatically render app/views/books/index.html.erb when you navigate to /books and you will see "Books are coming soon!" on your screen.

Good ref: Rails doesn't need index method in controller defined?

Community
  • 1
  • 1
Richard Peck
  • 76,116
  • 9
  • 93
  • 147
  • Thanks, that was the answer i was looking for. I still need those methods ?(set_project) (i use nested controllers in this one). – Hoggie Feb 12 '16 at 09:25
  • If you need to set the `@project` variable, you should be able to get away with the `set_project` method only, although I've not tested the nested routes with this setup. – Richard Peck Feb 12 '16 at 09:26
3

If you want to use those methods in future, keep those methods, else remove them. There will not be any problem even if they are kept.

Also remove the routes to those methods if they are created and you dont want to use them. Code should be as simple as it can.

Sravan
  • 18,467
  • 3
  • 30
  • 54
  • These method are routed, but set_project makes all logic for this action (finds the project which will be displayed). So these methods 'do' something. So i should keep them? (I use them), but the point is that the show method works without these methods (before_action) redirects it to set_project which does whole logic - thats why they are empty. – Hoggie Feb 12 '16 at 09:19
  • You can remove the methods, if you dont want to use it. – Sravan Feb 12 '16 at 09:24
1

If it's truly about style first keep your comments above, so that if you ever run rdoc or generate docs, they take whatever comments come before the def show and use that to build out the paragraph describing what this method "does".

Example


class ProjectController &lt ApplicationController
before_action :set_project


#indexaction is awesome and will be used to do stuff like look at all the projects
def index
end

# will hopefully create your project
def create
end

Yeah but don't keep it if it isn't used...

If routes aren't used, don't have them lying around. By default you get things like blah.com/projects/1.json and if you haven't locked this down (or even if you have and a user is inside the app) they could easily get the json result, assuming you left everything there for later. Let alone if you have a before_filter that does 'stuff' to projects on load`.

commit it once into git and then delete it and commit again. If you ever need to reinstate it, at least github/bitbucket history or git history if you are sadistic, will have it to copy and paste.

pjammer
  • 9,489
  • 5
  • 46
  • 56