views:

55

answers:

2

I wrote the code below which doesn't seem to work, but If I delete the last if statement, it does work, obviously for the first two use cases only. Is there a better way to write this code? In my routes file I have

map.resources :galleries
map.resources :users, :has_many => :galleries

A user clicks on the link "galleries" and see a list of all published galleries. (mysite.com/galleries)

A user can click on a link "my galleries" and sees all her own galleries. (mysite.com/users/21/galleries)

A user can clicks on a link on some other user's profile and see that persons published galleries. (mysite.com/users/35/galleries)

if params[:user_id].blank?
   @galleries = Gallery.find(:all, :conditions => ['visibility_status=  ?', true])
   end

if (params[:user_id] && current_user.id.to_s == params[:user_id])
    @galleries = current_user.galleries
end

if params[:user_id]
    @galleries = Gallery.find(:all, :conditions => ['user_id=? and
    visibility_status = ?', params[:user_id], true])
end
+1  A: 

OK - firstly - I'm not a Ruby programmer.

But - shouldn't you be using if - else if - else if - end, etc?

Your third condition will always be true if the second condition is true, so you're always overwriting @galleries with the third test even when you've already set it in the second test.

Alnitak
Thanks for trying to help. Even if I do that, I get an error "Called id for nil, which would mistakenly be 4 -- if you really wanted the id of nil, use object_id"
quoting the error message in the original question is the generally accepted practise around here ;-)
Alnitak
+1  A: 

The routes you have seem appropriate, using those routes you have a bunch of helper methods available in your controllers and views: such as user_galleries_url(@user) and user_galleries_url(@user, @gallery) and others. Take a look at resource routing for what you can do.

The only suggestion is to rework your logic a bit and put it in a before filter in
app/controllers/galleries_controller.rb

class GalleriesController < ActionController::Base
  before_filter :get_galleries, :only => 'index'

  private
    def get_galleries
      if params[:user_id]
        if params[:user_id] == current_user.id.to_s
          @galleries = current_user.galleries
        else
          @galleries = Gallery.find(:all, :conditions => ['user_id=? and visibility_status = ?', params[:user_id], true])
        end
      else
        @galleries = Gallery.find(:all, :conditions => ['visibility_status=  ?', true])
      end
    end
end

You can also do something similar for get_gallery on the show, edit, and delete actions.

Samuel
I find it looks cleaner and if you add any other method that needs to have a list of galleries, it's easy to add the filter.But it is more suited to @gallery when you do that in three methods.
Samuel