D07cb7e4c1749e027447bd3b250c302f

This is a model.
my method "image_path" here seems like it is long winded.
I've been using ruby for not very long.
Interested to see what you can come up with

has_img_* & filename are db fields 

class Car < ActiveRecord::Base
  belongs_to :model
  has_many :photos
  #
  # Figure out the path of the car image requested.
  #
  def image_path(which)
    img_url = "http://s3.carfinancechief.com.au"
    case which
    when "search"
      dimension = "200x100"
      photo = find_photo("F")
    when "module"
      dimension = "100x50"
      photo = find_photo("F")
    when "main"
      dimension = "418x155"
      photo = find_photo("F")
    end
    
    if(!photo.nil?)
      case which
      when "search"
        if(photo.has_img_search)
          url = "#{img_url}/car/#{photo.filename.downcase}_#{dimension}.jpg"
        else 
          url = "#{img_url}/not-yet_#{dimension}.jpg"
        end  
      when "module"
        if(photo.has_img_module)
          url = "#{img_url}/car/#{photo.filename.downcase}_#{dimension}.jpg"
        else 
          url = "#{img_url}/not-yet_#{dimension}.jpg"
        end        
      when "main"
        if(photo.has_img_main)
          url = "#{img_url}/car/#{photo.filename.downcase}_#{dimension}.jpg"
        else 
          url = "#{img_url}/not-yet_#{dimension}.jpg"
        end
      end
    else
      url = "#{img_url}/not-yet_#{dimension}.jpg"
    end
    url
  end
  
  private  # ---------------------------------------------------------
  
  def find_photo(view)
    photo = photos.find_by_view(view)
    photo
  end
end

Refactorings

No refactoring yet !

55502f40dc8b7c769880b10874abc9d0

keeb

October 26, 2008, October 26, 2008 09:36, permalink

No rating. Login to rate!

Agree, it is too long. This first way to tackle a problem is by decomposition ( http://en.wikipedia.org/wiki/Decomposition_(computer_science) .) That is to say, to break up a problem in to small, manageable pieces to achieve complexity through a means of combination.

Too much responsibility is being given to the image_path() method. I'd implement it as a Strategy Pattern ( http://en.wikipedia.org/wiki/Strategy_Pattern ) where each case is a separate Strategy. This would allow you to expand 'which' without impacting the image_path() function.

Unfortunately, I don't know any ruby so take my advice with that in mind ;)

D07cb7e4c1749e027447bd3b250c302f

SoreGums

October 26, 2008, October 26, 2008 11:04, permalink

No rating. Login to rate!

Cheers.

The original was three methods with a simple use the filename arrangement.
However I've had to smart it up a bit to check to see if there is an actual image.

I guess I should split up into 3 again and then put the smart part - is the image here or not in a method on its own.

Thanks :)

880cbab435f00197613c9cc2065b4f5a

danielharan

October 26, 2008, October 26, 2008 14:25, permalink

No rating. Login to rate!

There is some repetition that can be removed before applying design patterns. You may find the method becomes sufficiently readable to not worry about too much.

def image_path(type)
  img_url = "http://s3.carfinancechief.com.au"
  photo = find_photo("F")
  dimension = {"search" => "200x100", "module" => "100x50", "main" => "418x155"}[type]
  
  return "#{img_url}/not-yet_#{dimension}.jpg" if photo.nil? || !photo.has_img_main

  if(photo.has_img_search)
    case which
    ...
    end
  end
  
  url
end
A8d3f35baafdaea851914b17dae9e1fc

Adam

October 27, 2008, October 27, 2008 04:48, permalink

2 ratings. Login to rate!

Personally, I would probably rethink the model completely. But based on what you have:

class Photo < ActiveRecord::Basse
  URL_PREFIX = 'http://s3.carfinancechief.com.au'
  DIMENSIONS = { 'search' => '200x100', 'module' => '100x50', 'main' => '418x155' }
  
  belongs_to :car
  
  def self.url_for(path)
    [ URL_PREFIX, *path ].join('/')
  end
  
  def self.dimensions_for(view)
    DIMENSIONS[view] || DIMENSIONS.values.first
  end
  
  def self.image_path(view)
    photo = find_by_view('F')
    photo ? photo.image_path(view) : not_yet_path(view)
  end
  
  def self.not_yet_path(view)
    url_for("not-yet_#{dimensions_for(view)}.jpg")
  end
  
  def has_view?(view)
    DIMENSIONS[view] && send("has_img_#{view}")
  end
  
  def dimensions_for(view)
    self.class.dimensions_for(view)
  end
  
  def image_path(view)
    if has_view?(view)
      self.class.url_for("car/#{filename.downcase}_#{dimensions_for(view)}.jpg")
    else
      self.class.not_yet_path(view)
    end
  end
end

class Car < ActiveRecord::Base
  belongs_to :model
  has_many :photos
end

# @car.photos.image_path('search')
D07cb7e4c1749e027447bd3b250c302f

SoreGums

November 3, 2008, November 03, 2008 03:04, permalink

No rating. Login to rate!

Adam that is a vast improvement on my efforts thanks - last thing do is how do I do

photo.update_attribute(:has_img_(place), false)

obviously that isn't going to work

class Photo < ActiveRecord::Base
  URL_PREFIX = 'http://s3.carfinancechief.com.au'
  DIMENSIONS = { 'search' => '200x100', 'module' => '100x50', 'main' => '418x155', 'original' => 'orig' }
  S3_ACCESS = ''
  S3_SECRET = ''
  
  belongs_to :car
  
  def self.url_for(path)
    [ URL_PREFIX, *path ].join('/')
  end
  
  def self.dimensions_for(place)
    DIMENSIONS[place] || DIMENSIONS.values.first
  end
  
  def self.image_path(place)
    photo = find_by_view('F')
    photo ? photo.image_path(place) : not_yet_path(place)
  end
  
  def self.not_yet_path(place)
    url_for("not-yet_#{dimensions_for(place)}.jpg")
  end
  
  def has_place?(place)
    DIMENSIONS[place] && send("has_img_#{place}")
  end
  
  def dimensions_for(place)
    self.class.dimensions_for(place)
  end
  
  def image_path(place)
    if has_place?(place)
      self.class.url_for("car/#{filename.downcase}_#{dimensions_for(place)}.jpg")
    else
      self.class.not_yet_path(place)
    end
  end
  
  # Checks if image is in S3 bucket, publicly accessible then sets the 
  # column (has_img_*) to reflect status
  def verify_s3_existence(place)
    bucket = "s3.carfinancechief.com.au"
    file   = "car/#{filename.downcase}_#{dimensions_for(place)}.jpg"
    AWS::S3::Base.establish_connection!(
      :access_key_id     => S3_ACCESS,
      :secret_access_key => S3_SECRET
    )
    if(AWS::S3::S3Object.exists? file, bucket) then
      policy = AWS::S3::S3Object.acl(file, bucket)
      policy.grants << AWS::S3::ACL::Grant.grant(:public_read)
      photo.update_attribute(:has_img_(place), true)
    else
      photo.update_attribute(:has_img_(place), false)
    end
    AWS::S3::Base.disconnect!()
  end
end

class Car < ActiveRecord::Base
  belongs_to :model
  has_many :photos
end

# @car.photos.image_path('search')
D07cb7e4c1749e027447bd3b250c302f

SoreGums

November 3, 2008, November 03, 2008 04:53, permalink

No rating. Login to rate!

I've just done this

class Photo < ActiveRecord::Base
  URL_PREFIX = 'http://s3.carfinancechief.com.au'
  DIMENSIONS = { 'search' => '200x100', 'module' => '100x50', 'main' => '418x155', 'original' => 'orig' }
  S3_ACCESS = ''
  S3_SECRET = ''
  
  belongs_to :car
  
  def self.url_for(path)
    [ URL_PREFIX, *path ].join('/')
  end
  
  def self.dimensions_for(place)
    DIMENSIONS[place] || DIMENSIONS.values.first
  end
  
  def self.image_path(place)
    photo = find_by_view('F')
    photo ? photo.image_path(place) : not_yet_path(place)
  end
  
  def self.not_yet_path(place)
    url_for("not-yet_#{dimensions_for(place)}.jpg")
  end
  
  def has_place?(place)
    DIMENSIONS[place] && send("has_img_#{place}")
  end
  
  def dimensions_for(place)
    self.class.dimensions_for(place)
  end
  
  def image_path(place)
    if has_place?(place)
      self.class.url_for("car/#{filename.downcase}_#{dimensions_for(place)}.jpg")
    else
      self.class.not_yet_path(place)
    end
  end
  
  # Checks if image is in S3 bucket, publicly accessible then sets the column (has_img_*) to reflect status
  def has_s3_img(place)
    result = false
    bucket = "s3.carfinancechief.com.au"
    file   = "car/#{filename.downcase}_#{dimensions_for(place)}.jpg"
    AWS::S3::Base.establish_connection!(
      :access_key_id     => S3_ACCESS,
      :secret_access_key => S3_SECRET
    )
    if(AWS::S3::S3Object.exists? file, bucket) then
      policy = AWS::S3::S3Object.acl(file, bucket)
      policy.grants << AWS::S3::ACL::Grant.grant(:public_read)
      #photo.update_attribute(:has_img_(place), true)
      result = true
    end
    AWS::S3::Base.disconnect!()
    result
  end
end
#!/usr/bin/env ruby

# #####################################################################
#
# Check if a file has been uploaded to S3 yet
#
# #####################################################################

require 'config/environment'
require 'lib/importers'

announce 'Checking for photos' do |out|
  total = Photo.count*4
  i = 0

  Make.find(:all, :include => :models).each do |make|
    make.models.each do |model|
      model.cars.each do |car|
        car.photos.each do |photo|
          photo.update_attribute(:has_img_search, photo.has_s3_img("search"))
          out.progress "#{i+=1}/#{total}"
          photo.update_attribute(:has_img_module, photo.has_s3_img("module"))
          out.progress "#{i+=1}/#{total}"
          photo.update_attribute(:has_img_main, photo.has_s3_img("main"))
          out.progress "#{i+=1}/#{total}"
          photo.update_attribute(:has_img_original, photo.has_s3_img("original"))
          out.progress "#{i+=1}/#{total}"
        end
      end
    end
  end
end

Your refactoring





Format Copy from initial code

or Cancel