Rails Best Practices

I think I would rather see each method call the private method. More explicit, not much more code.

No Eric, when it's comming to 14 methods where 12 of it call the protected method or not... totally and awful to refactor, unreadable, loosing time to find the calls or not...
Furthermore before_filters are considered as basic bast practice of Rails... It's quite the equivalent of AOP in Java, and thus a very usefull way of transparently way of reusing code.

Use it and love it ! ;)

Also, I guess it would make sense to rescue from RecordNotFound and respond 404 instead of 500.

The problem with using a before_filter is that the instance variable feels like magic. You suddenly have this @post variable available and it is not immediately obvious that this is created by a before_filter. Especially if the before_filter is in some parent controller. I prefer to use before_filters for things like access control but like the actual implementation of the methods to be more explicit and not spread all around.

This is not to say I would never use it. I just wouldn't call it a best practice. With a LOT of actions I could see it being useful. But won't a controller with 14 actions need to be refactored anyway?

I agree with Eric. I have previously used the before_filter method to do a simple finder in, but when you come back to it, it its less clear what's going on.

If it were more than a couple of lines of code then I'd probably do it. But for one line it's pointless.

Yeah, of course for one line of code, I agree, but most of the time you tests access rights, statuses, etc... but refactoring a controller because it has 14 methods... well, not obviously... sometimes you just have added a big API, or import/export methods, specific upload systems, and so on..

I'm with Eric on this. Method call with memoization is best. I would even go as far as to make that method avalible to views with helper_method. Inherited Resources' resource and collection method are great examples of this.

Ditto, with Eric. It reduces readability, does nothing to improve performance, and the refactoring argument loses steam if the common code is in a private controller function.

DRY is a principle with the purpose to make code more manageable. When DRY abused, the code becomes esoteric -- less manageable. Let's avoid going overboard. No, at a minimum I want to see an assignment operator in my function if there is an assignment operation happening in my function.

Hmmm...I think some obvious before_filters are necessary. For example we have a nested resources

resources questions do
  resources answers
end

It's useful and obvious that we should add a before_filter to find the parent @question for every actions in AnswersController

class AnswersController < ApplicationController
  before_filter :find_question
  ...
  private
    def find_question
      @question = Question.find(params[:question_id])
    end
end

I feel that is an abuse of the before filter. Rails has enough magick to begin with. We need to think of other people reading our code. That is just making it harder to understand.

+1 with Eric.
It can be helpful sometimes to reduce the amount of code and it is a good trick but should not be considered a best practice.

It depends on the number of lines saved. There is no benefit in replacing 1 line of code in 2 controller methods with 4 lines of before_filter code.

aggree with eric and other commentors, this obscures functionality in the code too much.

Would it be possible to alter this, or create a new check, that just looks for repeated code in controller actions that cause the same redirect? That way, we'll know the code is doing some sort of authentication check, and redirecting. That in my opinion, is the best use of the before filter.

Question: did you deliberately erase the def show and def edit methods, and still can see them in the before filter? I mean, shouldn't the code in this exact form throw you an error that show and edit methods does not exist?!!

@Aghyad
It won't throw an error if views for those actions exist.

@everyone disagreeing with this approach
I've been using this approach for over a year and a half, and it has worked very well for creating maintainable code. I migrated to this approach after using resources_controller for over a year. Now resources_controller was indeed obfuscating magic, but using before_filters isn't magic. It simply defines resource creation in a single location. In my experience, people don't have any problem understanding what is happening.

Using before filters to pull models into scope means that I can look at a single location and have a complete understanding of which actions find resources, and which create them. An :except block means that any new actions automatically find a resource (which is almost always what I need when adding actions). And this leaves actions to focus on the action, rather than pulling an object into scope and then performing the action on it. Here is an example for a resource that is nested within another resource:

before_filter :find_containing_resource
before_filter :new_resource, :only => [:new, :create]
before_filter :find_resource, :except => [:index, :new, :create]
before_filter :find_resources, :only => [:index]

...

private



def find_containing_resource
@post = Post.find(params[:post_id])
end

def new_resource
@comment = @post.comments.build(params[:comment])
end

def find_resource
@comment = @post.comments.find(params[:id])
end

def find_resources
@comments = @post.comments
end

Folks...

The before_filter is the right thing to do if there's duplicated code. If you can't follow the logic flow -- i.e. if you think something this simple is "magic" -- then you should learn Rails better before trying to work with it.

I can't believe anyone's seriously arguing for repetitive code.

I do not argue for duplicating code. I just argue for more explicit logic. So I would rewrite the original "bad" code as:

class PostsController < ApplicationController
  def show
    load_instance
  end

  def edit
    load_instance
    render :action => 'form'
  end

  def update
    load_instance
    if @post.update_attributes params[:post]
      redirect_to posts_url, :notice => "#{@post} successfully updated"
    else
      render :action => 'form'
    end
  end

  def destroy
    Post.destroy params[:id]
  end

  private

  def load_instance
    @post = current_user.posts.find params[:id]
  end
end

So the loading of the instance is still only written once. But:

1. It is obvious to anyone looking at the controller that an "edit" and "show" action exists (they may miss the fact that is is declared in the before filter).
2. When you look at each action it doesn't seem like the instance variables appear magically. You can clearly see something is ran before the instance variables are used. You can then do a search to find this "load_instance" method.

Obviously with our example my two points are not that big of a deal. But as the capabilities of a controller increases in complexity it becomes more difficult to see the "magic" of a before filter hidden somewhere in the app and the explicitness of method calling becomes very helpful.

Again, I am not saying loading data via before_filters is bad. Maybe for a real simple controller it is fine. Or maybe for a controller that has lots of similar non-RESTful actions it is also fine. But calling it a best practice is going too far. It is a technique that can be used in some situations. Nothing more.

The contract between a controller and view is based upon instance variables. Because of that, the instance variables themselves should be explicitly specified in actions that render a view.

class PostsController < ApplicationController
  def index
    @posts = self.posts
  end

  def show
    @post = self.post
  end

  def new
    @post = self.posts.build params[:post]
  end

  ...

  protected

  def posts
    self.current_user.posts
  end

  def post
    self.posts.find params[:id]
  end
end

Ideally, those methods would also be memoized, so you could refer to them multiple times without needing to set an instance variable or local variable, without penalty.

Wait, you guys are saying this is wrong?

I thought this was the most basic best practice out there for controllers, I guess I must be wrong.

You guys go ahead and waste time on duplicating your code in your REST controllers, I'll keep writing

class ProjectsController < InheritedResources::Base; end

and get to working on harder more interesting problems...

I am late-model,wish you a advantageous day!

Qualitative industrial equipment. We offer a full and unique set of sensors to Siemens: these equipment concern a range of various appendices and requirements. We sell thermoresistance, thermocouples, electronic converters and radiation pyrometers for contactless measurement of temperature. Our site of Sensors www.datchik-sensor.ru

employee retreats plan new vision vitamins 30-60-90 day plan for schools . ray ban glasses online trazodone teva synthroid and depression medications minocycline sun sensitivity , http://ordergenericpills.com/buy-amoxil-amoxicillin/ . viagra does compared to levitra does what is medroxyprogesterone used for metformin xl and pregnancy safety buy amoxicillin no prescription overnight delivery, file cabinet plans clomid effects ovulation kits hyzaar ds 100 25 . http://ordergenericpills.com/buy-antabuse-disulfiram order disulfiram , Tomtrewimmorb cozaar versus diovan quixtar business plan power walk plan .