2
\$\begingroup\$

I have a polymorphic notification model with action attribute. When user clicks on a notification he/she should be redirected to given page the notification refers to through notifiable.

What is the best way to refactor the following method?

_notification.html.erb

<%= link_to checking_decreasing_user_notifications_path(current_user, notification: notification) do %>

notifications controller

def checking_decreasing
  notification = Notification.find(params[:notification])
  current_user.decreasing_comment_notification_number(notification)
  redirect_to notification_redirection_path(notification)
end

def notification_redirection_path(notification)
  notifiable_type = notification.notifiable_type
  notifiable_id = notification.notifiable_id
  action = notification.action
  if action == "commented"
    route = case notifiable_type
            when "Post"
              posts_path(anchor: "post_#{notifiable_id}")#{}"/posts#post_#{notifiable_id}"
            when "Product"
              product_path(notifiable_id, anchor: "comment-panel")#/products/#{notifiable_id}#comment-panel"
            when "ProductLead"
              product_product_lead_path(product_id, notifiable_id, anchor: "comment-panel")#{}"/products/#{product_id}/#{notifiable_type}/#{notifiable_id}#comment-panel"
              product_lead = ProductLead.find(notifiable_id)
              product_id = product_lead.product_id
            when "ProductCustomer"
              product_customer = ProductCustomer.find(notifiable_id)
              product_id = product_customer.product_id
              product_product_customer_path(product_id, notifiable_id, anchor: "comment-panel") #/products/#{product_id}/#{notifiable_type}/#{notifiable_id}#comment-panel"
            end
  elsif action == "invited"
    if notifiable_type == "Product"
      product_path(notifiable_id, anchor: "product-invitation-well")
    elsif notifiable_type == "ProductCustomer"
      product_customer = ProductCustomer.find(notifiable_id)
      product_id = product_customer.product_id
      product_product_customer_path(product_id, notifiable_id)
    end
  elsif action == "accepted"
    if notifiable_type == "Product" #team member invitation
      product_product_owner_panels_path(notifiable_id)
    elsif notifiable_type == "ProductCustomer" #referencer invitation
      product_customer = ProductCustomer.find(notifiable_id)
      product_id = product_customer.product_id
      product_product_owner_panels_path(product_id)
    end
  elsif action == "wrote"
    product_customer = ProductCustomer.find(notifiable_id)
    product_id = product_customer.product_id
    product_product_customer_path(product_id, notifiable_id)
  end
end
\$\endgroup\$
  • \$\begingroup\$ you should really, really try to find more succint names. think about how you'd describe what you're talking about if you were face to face with someone. eg, a product_customer should probably just be a customer. the context makes it clear you're talking about notifications, so type should suffice, etc. \$\endgroup\$ – Jonah Jun 16 '16 at 3:18
  • \$\begingroup\$ I think, that there is error in notification_redirection_path's case statement just under when "ProductLead". \$\endgroup\$ – Sergii K Jun 16 '16 at 12:44
  • \$\begingroup\$ I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Jun 23 '16 at 21:35
1
\$\begingroup\$

Suggestions:

  • Break long methods into simple parts;
  • Avoid unnecessary local variables;
  • Use single quotes for strings without interpolation;
  • Move repeated parts of code into separate methods.

Code:

def notification_redirection_path(notification)
  notifiable_type = notification.notifiable_type
  notifiable_id = notification.notifiable_id

  case notification.action
  when 'commented'
    case notifiable_type
    when 'Post'
      posts_path(anchor: "post_#{notifiable_id}")#{}"/posts#post_#{notifiable_id}"
    when 'Product'
      product_path(notifiable_id, anchor: 'comment-panel')#/products/#{notifiable_id}#comment-panel"
    when 'ProductLead'
      product_product_lead_path(*lead_params(notifiable_id), anchor: 'comment-panel')#{}"/products/#{product_id}/#{notifiable_type}/#{notifiable_id}#comment-panel"
    when 'ProductCustomer'
      product_product_customer_path(*customer_params(notifiable_id), anchor: 'comment-panel') #/products/#{product_id}/#{notifiable_type}/#{notifiable_id}#comment-panel"
    end
  when 'invited'
    if notifiable_type == 'Product'
      product_path(notifiable_id, anchor: 'product-invitation-well')
    elsif notifiable_type == 'ProductCustomer'
      product_product_customer_path(*customer_params(notifiable_id))
    end
  when 'accepted'
    if notifiable_type == 'Product' #team member invitation
      product_product_owner_panels_path(notifiable_id)
    elsif notifiable_type == 'ProductCustomer' #referencer invitation
      product_product_owner_panels_path(*customer_params(notifiable_id).first)
    end
  when 'wrote'
    product_product_customer_path(*customer_params(notifiable_id))
  end
end

private

def customer_params(notifiable_id)
  [ProductCustomer.find(notifiable_id).product_id, notifiable_id]
end

def lead_params(notifiable_id)
  [ProductLead.find(notifiable_id).product_id, notifiable_id]
end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.