2
\$\begingroup\$

I have a polymorphic notification model. When user gets a notification he can get on the page of the notifiable to check out what happened exactly. For instance invited you to join product or commented on a post. On the top of this I also have a polymorphic comment model. So user can get a notification not only for commented on a post but also for commented on a product customeror commented on a product, which of course just makes the whole situation more complex.

So the flow is the following:

If the user clicks on the notification, the link redirects to the custom checking_decreasing action to check the notification and decrease the notification number if necessary, and then it redirects to the desired page with anchor.

As you see my notification_redirection_path action looks terrible and I'm not even sure if it should be in application controller. Any ideas how I could refactor this code?

notifications index page (there is also a json version for dropdown notifications)

<%= link_to checking_decreasing_user_notifications_path(
      current_user,
      notifiable_type: notification.notifiable_type, 
      notifiable_id: notification.notifiable_id, 
      notification_action: notification.action
     ) do %>

notifications_controller

def checking_decreasing
  current_user.decreasing_comment_notification_number(
    params[:notifiable_type], 
    params[:notifiable_id]
  )
  redirect_to notification_redirection_path(
    params[:notifiable_type], 
    params[:notifiable_id], 
    params[:notification_action]
  )
end

application controller

def notification_redirection_path(notifiable_type, notifiable_id, action)
  if action == "commented"
    if notifiable_type == "ProductCustomer"
      product_customer = ProductCustomer.find(notifiable_id)
      product_id = product_customer.product_id
    elsif notifiable_type == "ProductLead"
      product_lead = ProductLead.find(notifiable_id)
      product_id = product_lead.product_id
    end
    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"
            when "ProductCustomer"
              product_product_customer_path(product_id, notifiable_id, anchor: "comment-panel") #/products/#{product_id}/#{notifiable_type}/#{notifiable_id}#comment-panel"
            end
  elsif action == "invited"
    product_path(notifiable_id, anchor: "product-invitation-well")
  elsif action == "accepted"
    product_product_users_path(notifiable_id)
  end
end
\$\endgroup\$
1
\$\begingroup\$

Here's a really basic refactor of notification_redirection_path:

def notification_redirection_path(notifiable_type:, notifiable_id:, notification_action:, **)
  case notification_action
  when "commented"
    case notifiable_type
    when "Post"
      posts_path(anchor: "post_#{notifiable_id}")
    when "Product"
      product_path(notifiable_id, anchor: "comment-panel")
    when "ProductLead"
      product_lead = ProductLead.find(notifiable_id)
      product_product_lead_path(product_lead.product_id, notifiable_id, anchor: "comment-panel")
    when "ProductCustomer"
      product_customer = ProductCustomer.find(notifiable_id)
      product_product_customer_path(product_customer.product_id, notifiable_id, anchor: "comment-panel")
    end
  when "invited"
    product_path(notifiable_id, anchor: "product-invitation-well")
  when "accepted"
    product_product_users_path(notifiable_id)
  end
end

A few things to notice:

  1. I changed the method arguments to keyword arguments. This way instead of doing this:

    redirect_to notification_redirection_path(
      params[:notifiable_type], 
      params[:notifiable_id], 
      params[:notification_action]
    )
    

    ...we can do this:

    redirect_to notification_redirection_path(params)
    
  2. I turned the outer if expression into a case expression. I also changed the indentation so the whens line up with their case. Usually I prefer the other style (like you wrote it) but I find with nested cases this is a bit easier to read.

  3. I removed your initial if notifier_type == ... expression and moved that logic into the when "ProductLead" and when "ProductCustomer" blocks below. No need to have those in two separate places.

This is still pretty messy, though. A quick win for readability, maintainability, and testability would be to split it into separate methods:

def notification_redirection_path(notifiable_type:, notifiable_id:, notification_action: nil, **)
  case notifiable_type
  when "commented"
    commented_notification_redirection_path(notifiable_type, notifiable_id)
  when "invited"
    product_path(notifiable_id, anchor: "product-invitation-well")
  when "accepted"
    product_product_users_path(notifiable_id)
  end
end

private
def commented_notification_redirection_path(type, id)
  case type
  when "Post"
    posts_path(anchor: "post_#{id}")
  when "Product"
    product_path(id, anchor: "comment-panel")
  when "ProductLead"
    product_lead = ProductLead.find(id)
    product_product_lead_path(product_lead.product_id, id, anchor: "comment-panel")
  when "ProductCustomer"
    product_customer = ProductCustomer.find(id)
    product_product_customer_path(product_customer.product_id, id, anchor: "comment-panel")
  end
end

You could employ polymorphic_path to take this to its extreme conclusion, which might look something like this:

ACTION_ANCHORS = {
  "commented" => "comment-panel",
  "invited" => "product-invitation-well"
}

def notification_redirection_path(notifiable_type:, notifiable_id:, notification_action:, **)
  if notification_action == "commented" && notifiable_type == "Product"
    return posts_path(anchor: "post_#{notifiable_id}")
  end

  notifiable = notifiable_type.constantize.find(notifiable_id)

  path =
    if notification_action == "commented" &&
        (ProductLead === notifiable || ProductCustomer === notifiable)
      [ notifiable.product, notifiable ]
    elsif notification_action == "accepted"
      [ notifiable, :product_user ]
    else
      [ notifiable ]
    end

  polymorphic_path(path,
    anchor: ACTION_ANCHORS[notification_action])
end

...but it's probably not worth it, since you lose a lot in readability and maintainability.

\$\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.