Can this be more succinct?

Ruby
1

How can this be more succinct? Can I somehow put the nil check inline
with the each?

unless(params[:categories].nil?)
params[:categories].each do |id|
@subscriber.subscriptions.create :category_id =>
id, :timestamp_start => Time.now.to_s, :is_subscribed => true
end
end

2

eggie5 wrote:

How can this be more succinct? Can I somehow put the nil check inline
with the each?

unless(params[:categories].nil?)
params[:categories].each do |id|
@subscriber.subscriptions.create :category_id =>
id, :timestamp_start => Time.now.to_s, :is_subscribed => true
end
end

(params[:categories]||[]).each do |id|
@subscriber.subscriptions.create :category_id => id,
:timestamp_start => Time.now.to_s, :is_subscribed => true
end

best,
Dan

3

Hi,

Am Dienstag, 28. Aug 2007, 23:00:08 +0900 schrieb eggie5:

How can this be more succinct? Can I somehow put the nil check inline
with the each?

unless(params[:categories].nil?)
params[:categories].each do |id|
@subscriber.subscriptions.create :category_id =>
id, :timestamp_start => Time.now.to_s, :is_subscribed => true
end
end

Maybe:

pc = params[:categories]
pc and pc.each do |id|
…
end

Bertram

4 
        params[:categories].each do |id|
            @subscriber.subscriptions.create :category_id =>

id, :timestamp_start => Time.now.to_s, :is_subscribed => true
end
end

Yes, you can:

params[:categories].each do |id|
@subscriber.subscriptions.create :category_id => id, :timestamp_start
=>
Time.now.to_s, :is_subscribed => true
end unless(params[:categories].nil?)

Though I personally find the longer form more readable. I read code top
to
bottom, so having the condition for a block at the top of it helps.

HTH,

Felix

5

eggie5 wrote:

How can this be more succinct? Can I somehow put the nil check inline
with the each?

unless(params[:categories].nil?)
params[:categories].each do |id|
@subscriber.subscriptions.create :category_id =>
id, :timestamp_start => Time.now.to_s, :is_subscribed => true
end
end

params[:categories].to_a.each do …

lopex

6

On 28.08.2007 16:09, Bertram S. wrote:

        end
    end

Maybe:

pc = params[:categories]
pc and pc.each do |id|
…
end

+1

robert

7

Hi eggie5,

How can this be more succinct? Can I somehow put the nil check inline
with the each?

unless(params[:categories].nil?)
params[:categories].each do |id|
@subscriber.subscriptions.create :category_id =>
id, :timestamp_start => Time.now.to_s, :is_subscribed => true
end
end

Just a minor thingy, rename timestamp_start column to created_at and
Rails does its magic. So you can write (categories replaced with
category_ids!):

(params[:category_ids] || []).each do |category_id|
@subscriber.subscriptions.create(:category_id => category_id,
:is_subscribed => true)
end

You can also put a verify in the class definition of your controller:

class SubscriptionsController < ApplicationController
verify :params => :category_ids, :method => :post,
:only => :subscribe,
:redirect_to => :back

def subscribe
# …
# both String and Array do have the each method…
params[:category_ids].each do |category_id|
@subscriber.subscriptions.create(:category_id => category_id,
:is_subscribed => true)
end
# …
end

…

end

You can even move this whole bunch of code to the subscribers model
itself:

class Subscriber < ActiveRecord::Base

This methods accepts multiple category ids in form of:

instance.subscribe 1, 2, 4, …

def subscribe(*category_ids)
# wrap insertion of multiple records in a transactions is generally
# a good idea…
ActiveRecord::Base.transaction do
category_ids.each do |category_id|
subscriptions.create(:category_id => category_id,
:is_subscribed => true)
end
end
end

end

class SubscriptionsController < ApplicationController
verify :params => :category_ids, :method => :post,
:only => :subscribe,
:redirect_to => :back

def subscribe
# …
@subscriber.subscribe(*params[:category_ids])
# …
end

…

end

Or extend the association with a module, see AssociationExtensions in
ActiveRecord::Associations::ClassMethods. in this case you could write
something like:

instance.subscriptions.category_ids = category_ids

This encapsulate all methods in a module that are only important for the
association itself - it’s clean!

If you don’t use ActionPack and ActiveRecord at all don’t mind this
post…

Regards
Florian

P.S.
Beware, this code was not run through any functional or unit tests…