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