1
\$\begingroup\$

I have a Notification model which inherits from the ActiveRecord class. Each notification is associated with a specific type of action, which is determined by an integer field (action) whose value is between 0 and 6. A notification is also associated with a model (e.g. User or Project) whose ID is stored as the object_id field.

Inside a view, I am trying to prepare the notification text and url, which looks something like this:

# notif_string prepares the text of notification.
# For example: "username commented on"
= link_to(notif_string(notification) + notification.objectname,
            notification.url)

The following methods in the Notification class where flagged as complex by Code Climate:

# Finds the object on which action is performed.
# Action can be of following types:
# 0: Commented on Project,
# 1: Followed Project,
# 2: Forked,
# 3: Followed User,
# 4: Created Project,
# 5: Commented on Issue
# 6: Created Issue 
def objectname
  case action
  when 0
    return Project.find(Comment.find(object_id).polycomment_id.to_i).name
  when 5
    return Issue.find(Comment.find(object_id).polycomment_id.to_i)
                .friendly_text
  when 3
    return User.find(object_id).username
  when 1, 2, 4
    return Project.find(object_id).name
  when 6
    return Issue.find(object_id).friendly_text
  end
end

# Find the url to which user which be directed when they click on the
# notification item
def url
  case action
  when 0
    return Project.find(Comment.find(object_id).polycomment_id.to_i).urlbase
  when 5
    return Issue.find(Comment.find(object_id).polycomment_id.to_i).show_url
  when 1, 2, 4
    return Project.find(object_id).urlbase
  when 6
    return Issue.find(object_id).show_url
  else
    return "/#{actor.username}"
  end
end

Yes, my cases are almost repeated but I am not sure how can reduce them. Most of the blogs suggest that I use polymorphism but again I don't know how I can apply it here.

\$\endgroup\$
  • \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. For examples of good titles, check out Best of Code Review 2014 - Best Question Title Category You may also want to read How to get the best value out of Code Review - Asking Questions. \$\endgroup\$ – BCdotWEB Nov 9 '15 at 10:07
  • 1
    \$\begingroup\$ Please explain what a Notification is in your app. Is it an ActiveRecord model stored in the database? Is action a field of Notification? What does each of its possible values mean? What's objectname for? \$\endgroup\$ – Spike Nov 9 '15 at 10:11
  • \$\begingroup\$ @Spike, BCdotWEB. Thanks for your suggestions and questions. I have updated my question. \$\endgroup\$ – sonalkr132 Nov 9 '15 at 10:57
2
\$\begingroup\$

First, object_id is a built-in method of Object in Ruby, so I recommend renaming this field to model_id to avoid any clashes.


You want polymorphic classes for the views of different notification types. Since these classes will only be used by views, we'll make them all inherit from a common base class named NotificationView which will provide all the information the view needs:

class NotificationView
  attr_reader :action_description, :object_name, :url
end

Every subclass of NotificationView will initialize by setting these fields according to a given Notification model. For example:

class IssueCreatedNotificationView < NotificationView
  def initialize(notification)
    issue = Issue.find(notification.model_id)
    @action_description = "created issue"
    @object_name = issue.friendly_text
    @url = issue.show_url
  end
end

(I would put these classes in /app/models/notification_views/)

Now you need a method that constructs the appropriate NotificationView given a Notification. Since the type of a notification is determined by an integer field, you will need a case statement. But you'll only need a single case statement in your code because you can take advantage of polymorphism once you have a NotificationView instance.

I would put this method in the Notification class:

def view
  case action
  when 2
    ProjectForkedNotificationView.new(self)
  when 6
    IssueCreatedNotificationView.new(self)
  # etc...
  end
end

Then in your controller, you can get a NotificationView by calling Notification#view. For example:

@view = Notification.find(params[:id]).view

And you can use this object in your view:

= link_to(@view.action_description + @view.object_name, @view.url)

You may want to add some utility methods to the NotificationView base class, if you need them in your views, for example:

def message
  "#{action_description} #{object_name}"
end
\$\endgroup\$
  • \$\begingroup\$ @sonalkr132 you know that in addition to accept, you can also upvote an answer if you liked it so much \$\endgroup\$ – janos Feb 14 '16 at 10:03
1
\$\begingroup\$

This doesn't directly answer your question, but I also recommend switching from integer actions to enums: http://edgeapi.rubyonrails.org/classes/ActiveRecord/Enum.html

Interestingly, this should not require any changes to your database at all, but will provide a lot of convenience methods and make your intent more clear. I do recommend hard coding a hash of the integer to the symbol in your enum definition to avoid any ambiguity: http://www.justinweiss.com/articles/creating-easy-readable-attributes-with-activerecord-enums/

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