1
\$\begingroup\$

I did this project and I'm going back over it and trying to optimize some stuff. This is the movies_controller.rb file, which is basically a table that I need to sort based on user parameters. What I really want to know here, is there a better way to save and assign the @sort and @ratings_filter? I just need to save them between sessions.

This is my first "revised" attempt:

def index
  @movies = Movie.all
  @all_ratings = Movie.all_ratings
  @ratings_filter = @all_ratings

  (params[:sort]) ? (@sort = params[:sort]) : (@sort = session[:sort])

  (params[:ratings] and params[:commit] == 'Refresh') ? 
  (@ratings_filter = params[:ratings].keys) : (@ratings_filter= session[:ratings] || @all_ratings)

  #save cookies
  if params[:sort] != session[:sort] or params[:ratings] != session[:ratings]
    session[:ratings] = @ratings_filter
    session[:sort] = @sort
    redirect_to movies_path :sort => @sort, :ratings => @ratings_filter# and return
  end

  @movies = Movie.sort_by_and_rating(@sort, @ratings_filter)

end

Here's the original for comparison:

    #params[:commit] has the value of the last button pressed
if (params[:ratings] and params[:commit] == 'Refresh')
    @ratings_filter = params[:ratings].keys
elsif params[:ratings]
    @ratings_filter = params[:ratings]
else
    @ratings_filter = session[:ratings] || @all_ratings
end

#@ratings_filter = params[:ratings].keys || session[:ratings] || @all_ratings

@sort = params[:sort] || session[:sort]

#save cookies
if params[:sort] != session[:sort] or params[:ratings] != session[:ratings]
    session[:ratings] = @ratings_filter
    session[:sort] = @sort
    redirect_to movies_path :sort => @sort, :ratings => @ratings_filter# and return
end

@movies = Movie.sort_by_and_rating(@sort, @ratings_filter)

Here's my GitHub if you need other files for context.

\$\endgroup\$
1
\$\begingroup\$

You could improve your code by doing the following:

  • Do not use and/or but &&/|| since what you need here is boolean operators, not flow control operators. Take a look at this article for a great example on how and when to use each operator.

  • You are constantly doing instance variable assignment at the one-liners conditionals which makes the code difficult to read (conditional ? @i = x ? @i = y). It's not wrong, but you could assign the result of the operation directly to the instance variable.

  • You could extract some conditional checks to private methods to state in a more clear way what your code does.

  • I'm not sure for this one, but I'll go for it. I think that you need the redirect when you set the session. You could just pass it to Movie.sort_by_and_rating(...).

Here's a refactored version of your code:

class MoviesController < ApplicationController
  def index
    @movies = Movie.all
    @all_ratings = Movie.all_ratings    
    @sort = params[:sort] || session[:sort]
    @ratings_filter = params_rating? ? params[:ratings].keys : (session[:ratings] || @all_ratings)

    if set_cookies?
      session[:ratings] = @ratings_filter
      session[:sort] = @sort
    end

    @movies = Movie.sort_by_and_rating(@sort, @ratings_filter)
  end

  private

  def params_rating?
    params[:ratings] && params[:commit] == 'Refresh'
  end

  def set_cookies?
    params[:sort] != session[:sort] || params[:ratings] != session[:ratings]
  end
end
| improve this answer | |
\$\endgroup\$

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.