Refactoring and Improving my Rake Task

Here's my rake task, I'm quite proud of it it's all working, but it's going to be really important to my rails app. Especially the fastercsv bit where I read in all the records and create activerecord objects - it's not very flexible at the moment and I'm not certain the best way to make it more flexible, for example if I want ot use different fields.

If anyone has time I'd really value a general critique and any tips. especially for improving the resorts:load task.

best

BB.

namespace :peaklocation do

  desc "Count the resorts in the database"   task :database_report => :environment do     puts     records = Resort.find(:all)     puts "Resorts : " + records.length.to_s     records = Advert.find(:all)     puts "Adverts : " + records.length.to_s     records = User.find(:all)     puts "Users : " + records.length.to_s     records = User.find(:all)     count = 0     for user in records       if user.admin         count += 1       end     end     puts "Admins : " + count.to_s   end

  # Resort Tasks   namespace :resorts do

    # note that the following command might be needed on the data file     # tr -d '\n' < peaklocation/db/resorts.csv > peaklocation/db/resorts1.csv

    desc "Load up all the resorts from resorts1.csv into #{Rails.env} database"     task :load => :environment do       require 'fastercsv'       count=0         FasterCSV.foreach("#{RAILS_ROOT}/db/resorts1.csv", :row_sep => "\r") do |row|           # name,serial_no,region = row           Resort.create(:name => row[0],:serial_number => row[1],:country => row[7],:resort_height => row[16],:overview_review => row[141],:region => row[6],:continent => row[8],:coordinates_east => row[10],:coordinates_north => row[11],:travel_to_review => row[146])           count += 1           # puts count.to_s + " Resort name: " + row[0]           puts "Resort created : " + row[0]         end       puts "-------------------------"       puts "Resorts created : " + count.to_s       puts "-------------------------"

      Rake::Task["peaklocation:database_report"].invoke

    end

    desc "Delete all the resorts from the #{Rails.env} database"     task :unload => :environment do

      Resort.delete_all

      Rake::Task["peaklocation:database_report"].invoke

    end

    desc "List all the resorts from the database"     task :list => :environment do       resorts = Resort.find(:all)       for resort in resorts         puts resort.id.to_s + ' ' + resort.name       end

      Rake::Task["peaklocation:database_report"].invoke

    end

  end

  # Advert Tasks   namespace :adverts do     desc "Load up all the advert_types into #{Rails.env} database"     task :load_advert_types => :environment do       AdvertType.create!(:name => "Accommodation", :description => "Accom")       AdvertType.create!(:name => "Food", :description => "Food")       AdvertType.create!(:name => "Ski Hire", :description => "Ski Hire")     end   end

  # User Tasks   namespace :users do     desc "Make a user admin in #{Rails.env} database"     task :make_admin => :environment do       username = ENV['USERNAME'] || ENV['username']       raise "Must specify USERNAME=" unless username       u = User.find_by_username(username)       if u         puts "#{u.username} found"         u.admin = 1         puts "#{u.username} set to admin"         u.save         puts "#{u.username} saved"       else         puts "#{username} not found"       end     end   end

end

bingo bob wrote:

Here's my rake task, I'm quite proud of it it's all working, but it's going to be really important to my rails app. Especially the fastercsv bit where I read in all the records and create activerecord objects - it's not very flexible at the moment and I'm not certain the best way to make it more flexible, for example if I want ot use different fields.

If anyone has time I'd really value a general critique and any tips. especially for improving the resorts:load task.

best

BB.

namespace :peaklocation do

  desc "Count the resorts in the database"   task :database_report => :environment do     puts     records = Resort.find(:all)     puts "Resorts : " + records.length.to_s     records = Advert.find(:all)     puts "Adverts : " + records.length.to_s     records = User.find(:all)     puts "Users : " + records.length.to_s     records = User.find(:all)     count = 0     for user in records       if user.admin         count += 1       end     end     puts "Admins : " + count.to_s   end

  # Resort Tasks   namespace :resorts do

    # note that the following command might be needed on the data file     # tr -d '\n' < peaklocation/db/resorts.csv > peaklocation/db/resorts1.csv

    desc "Load up all the resorts from resorts1.csv into #{Rails.env} database"     task :load => :environment do       require 'fastercsv'       count=0         FasterCSV.foreach("#{RAILS_ROOT}/db/resorts1.csv", :row_sep => "\r") do |row|           # name,serial_no,region = row           Resort.create(:name => row[0],:serial_number => row[1],:country => row[7],:resort_height => row[16],:overview_review => row[141],:region => row[6],:continent => row[8],:coordinates_east => row[10],:coordinates_north => row[11],:travel_to_review => row[146])           count += 1           # puts count.to_s + " Resort name: " + row[0]           puts "Resort created : " + row[0]         end       puts "-------------------------"       puts "Resorts created : " + count.to_s       puts "-------------------------"

      Rake::Task["peaklocation:database_report"].invoke

    end

    desc "Delete all the resorts from the #{Rails.env} database"     task :unload => :environment do

      Resort.delete_all

      Rake::Task["peaklocation:database_report"].invoke

    end

    desc "List all the resorts from the database"     task :list => :environment do       resorts = Resort.find(:all)       for resort in resorts         puts resort.id.to_s + ' ' + resort.name       end

      Rake::Task["peaklocation:database_report"].invoke

    end

  end

  # Advert Tasks   namespace :adverts do     desc "Load up all the advert_types into #{Rails.env} database"     task :load_advert_types => :environment do       AdvertType.create!(:name => "Accommodation", :description => "Accom")       AdvertType.create!(:name => "Food", :description => "Food")       AdvertType.create!(:name => "Ski Hire", :description => "Ski Hire")     end   end

  # User Tasks   namespace :users do     desc "Make a user admin in #{Rails.env} database"     task :make_admin => :environment do       username = ENV['USERNAME'] || ENV['username']       raise "Must specify USERNAME=" unless username       u = User.find_by_username(username)       if u         puts "#{u.username} found"         u.admin = 1         puts "#{u.username} set to admin"         u.save         puts "#{u.username} saved"       else         puts "#{username} not found"       end     end   end

end

I refactored your tasks a bit. I moved the guts of most of the tasks to class methods on the seemingly appropriate class. When generating the report I removed the use of String#+ and replaced it with interpolation #{}. I also used ActiveRecord::Base.count instead of .find and then .length on that. I tried to make some comments and suggestions.

class Database::Report   def self.to_s     puts     resort_count = Resort.count     puts "Resorts : #{resort_count}"     advert_count = Advert.count     puts "Adverts : #{advert_count}"     user_count = User.count     puts "Users : #{user_count}"     users = User.find(:all)     admin_count = 0     users.each do |user|       if user.admin         count += 1       end     end     # couldn't you use raw sql here?     # admin_count = User.count(:conditions => ["admin = ?", true])

    puts "Admins : #{admin_count}"   end end

require 'fastercsv'

class Resort   def self.import_from_csv(filename)     count=0

    # wrap in transaction in the event one of the resort's fails     # validation and raises an exception     Resort.transaction do       # I see you were removing the newline characters with tr, I       # changed your row_sep to "\r\n" thinking that might help.       # However if that does not work I would load the string up in       # ruby and perform the transformation with a regexp, then pass       # it off the the appropriate FasterCSV method for a string.       FasterCSV.foreach(filename, :row_sep => "\r\n") do |row|         # name,serial_no,region = row         # use ! (bang) version of create to throw exception if         # validation fails, helpful with transactions         Resort.create!(:name => row[0],                        :serial_number => row[1],                        :country => row[7],                        :resort_height => row[16],                        :overview_review => row[141],                        :region => row[6],                        :continent => row[8],                        :coordinates_east => row[10],                        :coordinates_north => row[11],                        :travel_to_review => row[146])         count += 1         puts count.to_s + "#{count} Resort name: #{row[0]}"         puts "Resort created : " + row[0]       end     end     puts "-------------------------"     puts "Resorts created : #{count}"     puts "-------------------------"   end end

namespace :peaklocation do

  desc "Count the resorts in the database"   task :database_report => :environment do     puts Database::Report   end

  namespace :resorts do

    desc "Load up all the resorts from resorts1.csv into #{Rails.env} database"     task :load => :environment do       filename = ENV['FILENAME'] || "#{RAILS_ROOT}/db/resorts1.csv"       Resort.import_from_csv(filename)       Rake::Task["peaklocation:database_report"].invoke     end

    desc "Delete all the resorts from the #{Rails.env} database"     task :unload => :environment do       Resort.delete_all       Rake::Task["peaklocation:database_report"].invoke     end

    desc "List all the resorts from the database"     task :list => :environment do       Resort.find(:all).each do |resort|         puts "#{resort.id} #{resort.name}"       end

      Rake::Task["peaklocation:database_report"].invoke     end   end

  namespace :adverts do     desc "Load up all the advert_types into #{Rails.env} database"     task :load_advert_types => :environment do       adverts = [["Accomodation", "Accom"],                  ["Food", "Food"],                  ["Ski Hire", "Ski Hire"],                 ].each do |name, description|         AdvertType.create!(:name => name, :description => description)       end     end   end

  namespace :users do     desc "Make a user admin in #{Rails.env} database"     task :make_admin => :environment do       username = ENV['USERNAME'] || ENV['username']       raise "Must specify USERNAME=" unless username       user = User.find_by_username(username)       if user         puts "#{user.username} found"         u.admin = 1         puts "#{user.username} set to admin"         u.save         puts "#{user.username} saved"       else         puts "#{username} not found"       end     end   end end

Best, Michael Guterl

Michael,

That's superb, like Christmas has come early! I'm going to review all your improvements to understand them and then implement, learned a LOT here. Thanks, will feedback.

Follow up question. How would you handle the migration file in my case (for the CSV import I have to create the fields in advance of course via the migration). Currently I rather boringly hand type them. Am wondering if it's possible to read the first row of the CSV (headers) and maybe create all the fields in one go ! I suppose I would have to have them as all the same data type e.g. string. Maybe not a smart move.

current migration like this....