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.

http://gist.github.com/160336

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....