1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21
# OPTIMIZE: I think we can do a LOT for optimize this. def create added_counter = 0 invalid_counter = 0 params[:recipients].each_line { |l| entry = l.split(',') email = entry[0] email.strip! unless email.nil? full_name = entry[1] full_name.strip! unless full_name.nil? subscriber = Subscriber.new :list_id => @list.id, :full_name => full_name, :email => email if subscriber.valid? subscriber.save added_counter += 1 else invalid_counter += 1 end } flash[:notice] = "Added #{added_counter} new subscribers, and a #{invalid_counter} records has been skipped or invalid." redirect_to(list_path(@list)) end
Refactorings
No refactoring yet !
mister
March 22, 2008, March 22, 2008 19:23, permalink
No rating.
Login to rate!
This code is of course full-functional and satisfy me, but is ugly as hell!
Dan Kubb
March 22, 2008, March 22, 2008 21:31, permalink
1 rating.
Login to rate!
Here is how I would write this. I wouldn't bother with the counters, just accumulate all the results into an array and loop over them to find out how many are valid or not, as needed. The only optimization I would make over what I have below, is if @list has a subscribers association, I'd use @list.subscribers.create instead of Subscribers.create :list_id => @list.id. Since I don't know what your data model is I chose the safest route below.
1 2 3 4 5 6 7 8 9
def create subscribers = [] params[:recipients].to_s.each_line do |line| email, full_name = line.split(',').map(&:strip) subscribers << Subscriber.create :list_id => @list.id, :email => email, :full_name => full_name end flash[:notice] = "Added #{subscribers.select(&:valid?).size} new subscribers, and a #{subscribers.size - subscribers.select(&:valid?).size} records has been skipped or invalid." redirect_to list_path(@list) end
This scripts get textarea with subscribers list entries separated by comma ','. We add