51224bdd17878b3b19e8987e9bb336a2

Following is the big(easy to understand) snippet of the model validations.
- As my model doesn't extend any ActiveRecord because I don't have table in database.
- I am simply validating the params and adding errors, and returning the errors Array.
- params['a'] is an group of 3 radio button
- Following is the sample of params generated.
{"carrier_name"=>"Metro PCS", "a"=>{"level"=>"1"}, "mobile_no"=>"687-687-6546",
"authenticity_token"=>"1ef5877874726bf44372b7e4eae79986626a7b05", "action"=>"save_alert", "controller"=>"newalert", "email_chbox"=>"1", "email_box"=>"deepak.gole8@gmail.com", "link"=>"http://losangeles.craigslist.org/apa/", "keywords"=>"Rental,House"}
- Also I don't want to use any gems or plugin 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
class Newalert 

def new_alert_validations(params)
     errors = []
     
      
    if params['keywords'].blank? 
      errors  << "keywords can't be blank."
    end
    
    if params['link'].blank? 
      errors  << "Website can't be blank."
    end
    
    unless params['link'].strip =~ /^(http|https):\/\/[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(([0-9]{1,5})?\/.*)?$/ix
      errors  << "Invalid format of website."
    end
     
     
    unless params['email_box'] =~  /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i
      errors  << "Invalid format of email."
    end
     
    if params['email_chbox'].blank? and params['text_chbox'].blank?
      errors  << "Please select either Email or Text message checkbox ."
    end
     
     
    
     return errors 
  end

end

Refactorings

No refactoring yet !

F288a8afe5302a16a366d5e9d34f2fec

Joe Grossberg

June 29, 2008, June 29, 2008 01:03, permalink

No rating. Login to rate!

Warning -- this is pedantic! :)

Instead of "if !params...", do "unless params...". Also, you use inconsistent capitalization. Only the first word in those sentences should be capitalized.

4fa63505dd4dd4691b87d30508b1bfd3

Xavier Shay

June 30, 2008, June 30, 2008 09:42, permalink

No rating. Login to rate!

- Move regexes into constants so it's clear what they do (OT: email regex probably isn't correct for more esoteric ones allowed by the RFC)
- Create a mini DSL to encapsulate validation logic, and make the intent clearer
- Use collect for composing arrays

This is just academic though - why aren't you using the Validatable gem?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
require 'rubygems'
require 'expectations'
require 'activesupport'

class Newalert 
  VALID_EMAIL_REGEX =   /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i 
  VALID_WEBSITE_REGEX = /^(http|https):\/\/[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(([0-9]{1,5})?\/.*)?$/ix 

  def new_alert_validations(params)
    [
      require_non_blank('keywords'),
      require_non_blank('link', :display_name => 'Website'),

      require_matches('link',      VALID_WEBSITE_REGEX, :display_name => 'website'),
      require_matches('email_box', VALID_EMAIL_REGEX,   :display_name => 'email'),

      lambda {|params| "Please select either Email or Text message checkbox" if params['email_chbox'].blank? && params['text_chbox'].blank? }
    ].collect {|validation|
      validation[params]
    }.compact
  end

  private

  def require_non_blank(attribute, options = {})
    lambda {|params| "%s can't be blank." % [options[:display_name] || attribute] if params[attribute].blank? }
  end

  def require_matches(attribute, regex, options = {})
    lambda {|params| "Invalid format of %s" % [options[:display_name] || attribute] unless params[attribute].to_s.strip =~ regex }
  end
end

def valid_params
  {
    "carrier_name"       => "Metro PCS",
    "a"                  => {"level" => "1"},
    "mobile_no"          => "687-687-6546",
    "authenticity_token" => "1ef5877874726bf44372b7e4eae79986626a7b05",
    "action"             => "save_alert",
    "controller"         => "newalert",
    "email_chbox"        => "1",
    "email_box"          => "deepak.gole8@gmail.com",
    "link"               => "http://losangeles.craigslist.org/apa/",
    "keywords"           => "Rental,House"
  }
end

Expectations do
  expect [] do
    Newalert.new.new_alert_validations(valid_params)
  end

  expect ["keywords can't be blank."] do
    Newalert.new.new_alert_validations(valid_params.merge('keywords' => ''))
  end

  expect ["Invalid format of website"] do
    Newalert.new.new_alert_validations(valid_params.merge('link' => 'bogus'))
  end

  expect ["Please select either Email or Text message checkbox"] do
    Newalert.new.new_alert_validations(valid_params.merge('email_chbox' => nil, 'text_chbox' => nil))
  end
end
4fa63505dd4dd4691b87d30508b1bfd3

Xavier Shay

June 30, 2008, June 30, 2008 09:48, permalink

1 rating. Login to rate!

Here another less overkill version - which one you use depends on how much you intend to be using this code - if it's a once off this code is probably better, but if you want to use it a lot the first one has some advantages.

- Extracted regexes into constants
- White space padding so repeated code is clear
- Added .to_s to regexes so they don't error on nil, added .strip so they are consistent (if this is not desired, a comment/test as to why not is recommended)
- Condensed 3 line conditional into one liner for better flow

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
class Newalert 
  VALID_EMAIL_REGEX =   /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i 
  VALID_WEBSITE_REGEX = /^(http|https):\/\/[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(([0-9]{1,5})?\/.*)?$/ix 

  def new_alert_validations(params)
    errors = []
    errors  << "Keywords can't be blank." if params['keywords'].blank? 
    errors  << "Website can't be blank."  if params['link'].blank? 

    errors  << "Invalid format of website." unless params['link'     ].to_s.strip =~ VALID_WEBSITE_REGEX
    errors  << "Invalid format of email."   unless params['email_box'].to_s.strip =~ VALID_EMAIL_REGEX

    errors  << "Please select either Email or Text message checkbox ." if params['email_chbox'].blank? && params['text_chbox'].blank?
    return errors 
  end
end
51224bdd17878b3b19e8987e9bb336a2

DG

July 4, 2008, July 04, 2008 05:16, permalink

No rating. Login to rate!

Thanks Xavier Shay
I would be using 2nd refactoring in my application :)

Your refactoring





Format Copy from initial code

or Cancel