CODEWITHSUNDEEP
ruby-on-railsruby-on-rails-3
Dipak Panchal
Dipak Panchal

Reputation: 6036

How to optimize my code

I want to optimize my code How can i ?

If I added new role then one more condition add but that i don't want. I want any no. of roles but i need only one conditions is it possible?

after_save :announcement_send

def announcement_send
  if self.send_now == true && self.group_id.to_s == "Artists"
    User.having_role("artist").each do |user|
      ArtistMailer.announcement_user(self, user).deliver
    end
  elsif self.send_now == true && self.group_id.to_s == "Fans"
    User.having_role("fan").each do |user|
      ArtistMailer.announcement_user(self, user).deliver
    end
  elsif self.send_now == true && self.group_id.to_s == "Both"
     User.not_having_role("admin").each do |user|
       ArtistMailer.announcement_user(self, user).deliver
     end
  end        
end

Upvotes: 0

Views: 102

Answers (4)

Andreas Lyngstad
Andreas Lyngstad

Reputation: 4927

Pass the role in as an argument

def announcement_send(role)
  if self.send_now == true 
    User.having_role(role).each do |user|
      ArtistMailer.announcement_user(self, user).deliver
    end  
end

def after_save(role)
   announcement_send(role)
end

This will work in the two first conditions

In the last I am guessing that the users have both "fans" and "artists" roles. Then pass user.role.first

Upvotes: 0

apneadiving
apneadiving

Reputation: 115511

Good code should tell stories.

WHITELIST = %w(artist fan)

def announcement_send
  return unless send_now
  users_list_for_announcement.each { |user| notify(user) }
end

def users_list_for_announcement
  in_whitelist? = ->(group) { WHITELIST.include?(group) }
  case formatted_group_name
  when "both"        then User.not_having_role("admin")
  when in_whitelist? then User.having_role(formatted_group_name)
  else []
  end
end

def formatted_group_name
  group_id.to_s.singularize.downcase
end

def notify(user)
  ArtistMailer.announcement_user(self, user).deliver
end

Actually, I'd even create a dedicated scope in User handling the logic to get the proper users depending on the group.

Upvotes: 2

Gowri Naidu R
Gowri Naidu R

Reputation: 2034

I think the following is your required answer:


if self.send_now
if self.group_id.to_s == "Both" 
    User.not_having_role("admin").each do |user|
        ArtistMailer.announcement_user(self, user).deliver
    end
elsif self.group_id.to_s
    User.having_role(self.group_id.to_s.singularize.downcase).each do |user|
        ArtistMailer.announcement_user(self, user).deliver
    end     
end
end

Upvotes: 0

Tigraine
Tigraine

Reputation: 23648

Not really tested this out but I would break this down like this:

def role(s)                                                                                                                                                                               
  if s == "Both"                                                                                                                                                                          
    User.not_having_role("admin")                                                                                                                                                         
  else                                                                                                                                                                                    
    User.having_role(s.singularize.downcase)                                                                                                                                              
  end                                                                                                                                                                                     
end         

if self.send_now                                                                                                                                                                          
  role(self.group_id.to_s).each do |user|                                                                                                                                                 
    ArtistMailer.announcement_user(self, user).deliver                                                                                                                                    
  end                                                                                                                                                                                     
end

This code assumes the group_id always comes along as the plural form of the role the users are in. So Artists means the "artist" role.

Of course this assumes all roles are valid group_ids. If that's not the case you can check the group_id against a whitelist of possible values inside def role:

white_list = ['artist', 'fan']
role = s.singularize.downcase
if white_list.include? role
  User.having_role(role)
else
  # possibly throw an exception
end

Upvotes: 3

Related Questions