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.

The method must search Products found by criteria and store them in @products.

The criteria are as follows:

  • If the user enters something in text_field (:search_keywords) then the product's name needs to be matched with that word
  • If the user specifies product_type (:product[:type]) then all the found products must be of that type.

def search
  if params[:search_keywords] && params[:search_keywords] != ""
    @products = Product.where("name LIKE '%" + params[:search_keywords] + "%'")
    if params[:product] && params[:product][:type] && params[:product][:type] != ""
      @products = Product.where("name LIKE '%" + params[:search_keywords] + "%' AND product_type_id = " + params[:product][:type])
    end
  else
    if params[:product] && params[:product][:type] && params[:product][:type] != ""
      @products = Product.find_all_by_product_type_id(params[:product][:type])
    else
      @products = Product.all
    end
  end
end
share|improve this question
7  
Note that the way you build your queries is extremely vulnerable to SQL injection. Please see api.rubyonrails.org/classes/ActiveRecord/Base.html under the conditions section on how to safely build queries. –  Bert Goethals Jun 3 '11 at 14:13
add comment

5 Answers 5

Try to use scopes, its a lot nicer

def search
  @products = Products.scoped
  if !params[:search_keywords].blank?
    @products = @products.where("name LIKE ?", "%#{params[:search_keywords]}%")
  end
  if !params[:product].try(:[], :type).blank?
    @products = @products.where(:product_type_id => params[:product][:type])
  end
  @products # maybe @products.all if realy needed
end
share|improve this answer
add comment

I've seen many Railers' codes tempt to shove many logic in View and Controller. Allow me to add these important emphases:

  1. Skinny Controller Fat Model which is one of The Elegant Rails Way principles.

  2. And Testing.

Here I have products_controller.rb file inside app/controllers directory that contains this:

class ProductsController < ApplicationController
  def search
    @products = Product.search(params)
  end
end

I put the logic inside the Model. Here is my product.rb file inside app/models directory:

class Product < ActiveRecord::Base
  scope :nm, lambda { |n| where "name LIKE ?", '%' + n + '%' }
  scope :criteria, lambda { |n, type| nm(n).where type_id: type }

  def self.search(opts={})
    return all if opts.empty?
    return nm(opts[:search_keywords]) unless opts[:search_keywords].nil?
    return criteria(opts[:name], opts[:type]) if opts[:product].try(:[], :type).empty?
  end
end

You could also try to experiment Dynamic Scope Construction to improve Product Model code above. I haven't tried it successfully AND efficiently here, let me know if anyone can make it better, anyway.

And Testing. You can also test your app using RSpec, Cucumber, Ruby Selenium, Watir, or any other Test for Ruby Environment, but here I'll only show you how to do it using Ruby Unit Test. Here is my product_test.rb inside test/unit directory:

require 'test_helper'
require 'product'

class ProductTest < ActiveSupport::TestCase
  fixtures :products

  def test_nm
    assert_equal Product.nm("Pick").first.name, products(:pickaxe).name
  end

  def test_criteria
    assert_equal Product.criteria("Bach", 2).first.name, products(:bach).name
  end

  def test_search
    assert_equal Product.search(:search_keywords => "Pick").first.name, 
                 products(:pickaxe).name
    assert_equal Product.search(:search_keywords => "Pick", :type => 1).first.name, 
                 products(:pickaxe).name
    assert_equal Product.search.size, Product.count
  end
end

And its related fixture. Here is my products.yml file inside test/fixtures directory:

pickaxe:
  name: Pickaxe 3
  type_id: 1

bach:
  name: Bach
  type_id: 2

lotr:
  name: LOTR
  type_id: 3

And for controller testing, here is my products_controller_test.rb file inside test/functional directory:

require 'test_helper'
require "products_controller"

class ProductsControllerTest < ActionController::TestCase
  fixtures :products

  test "should get search" do
    get :search, :search_keywords => 'Pick', :product => { :type => '1' }
    assert_response :success
    assert_not_nil assigns(:products)
  end
end

Let's go test them. I use:

. ruby -v
ruby 1.9.2p0 (2010-08-18) [i386-darwin9.8.0]
. rails -v
Rails 3.0.9

Here is my test's result:

. rake test
(in /Users/arie/se/tester)
NOTICE:  CREATE TABLE will create implicit sequence "products_id_seq" for serial column "products.id"
NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "products_pkey" for table "products"
Loaded suite /opt/experiment/ruby/lib/ruby/1.9.1/rake/rake_test_loader
Started
...
Finished in 0.237899 seconds.

3 tests, 5 assertions, 0 failures, 0 errors, 0 skips

Test run options: --seed 54809
Loaded suite /opt/experiment/ruby/lib/ruby/1.9.1/rake/rake_test_loader
Started
.
Finished in 0.330431 seconds.

1 tests, 2 assertions, 0 failures, 0 errors, 0 skips

Test run options: --seed 25681

Alright! Worked like a charm! 0 errors for both unit (for model) and functional (for controller) tests. I also tested it from my browser and it worked.

You can also download my copy files here.

Cheers!

RTFM (I'll update to put more links here if I can in the future / if I got more reputation):

  1. Testing Reference
  2. Beginning with Cucumber RailsCasts
  3. Active Record Query Interface 3.0
  4. The Skinny on Scopes (Formerly named_scope)
share|improve this answer
add comment
  1. You can use blank?

  2. You need user parameters in SQL query.

  3. Maybe you need escaping character % in search_keywords.

def search
    keywords_blank = params[:search_keywords].blank?
    type_blank = params[:product] && !params[:product][:type].blank?

    @products = if keywords_blank && type_blank
      Product.all
    elsif keywords_blank
      Product.find_all_by_product_type_id(params[:product][:type])
    elsif type_blank
      Product.find(:all, :conditions => ["name LIKE :name", :name => "%#{params[:search_keywords]}%"])
    else
      Product.find(:conditions => ["name LIKE :name AND product_type_id = :type", {:name => "%#{params[:search_keywords]}%", :type => params[:product][:type]}])
    end
end
share|improve this answer
    
Thanks a lot for reviewing my code –  user600115 May 31 '11 at 19:59
add comment

You can use a combination of scopes to simplify the code. Also, you can take advantage of some handy Ruby/ActiveSupport methods and conventions to avoid if conditions.

def search
  @products = Product.scoped
  @products = @products.search_by_name(params[:search_keywords])
  @products = @products.search_by_type(params[:product][:type] rescue nil)
  # in your view
  # @products.all
end 

class Product

  def self.search_by_name(value)
    if value.present?
      where("name LIKE ?", "%#{value}%")
    else
      self
    end
  end

  def self.search_by_type(value)
    if value.present?
      where(:product_type_id => value)
    else
      self
    end
  end

end
share|improve this answer
add comment

I like Bert Goethals, Arie and Simone Carletti's answers - my solution would have a touch of each:

in your controller:

class ProductsController < ApplicationController

  def search
    keywords = params[:search_keywords]
    type_id = params[:product].try(:[],:type)
    @products = Product.search({:type_id => type_id, :keywords => keywords})
  end

end

in your model

  def self.search(options={})
    keywords = options[:keywords]
    type_id = options[:type_id]

    products = Product.scoped
    products = products.where("name like ?","%#{keywords}%") if keywords.present? 
    products = products.where(:type_id => type_id) if type_id.present?
    products
  end

I could go with scopes but only if the scope will be used from more than one spot, otherwise the code is small enough to be understandable and organized without them.

share|improve this answer
add comment

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.