2
\$\begingroup\$

I have a content management system for an iOS app built with Rails. Right now there are three different types of content. The first two are just strings uploaded into a textfield by the user. One is Twitter handles and hashtags, the other is URLs. The second type is photos.

The user can upload as many individual instances of these types content as he/she wants to given a feed which will is the then passed to the iOS app via an API call to display.

The current approach I am using is to have Feed, Picture, Twitter, and Webpage models. When the user creates a new Feed, they then associate the appropriate content with that feed via a one to many relationship between each Feed instance it's content instances.

When it comes time to display to content the user has uploaded, a GET request for a given stream is generated and the Feed model then does the following awful thing determine what type of content should be shown:

 def Feed.get_content_for_feed(feed_id)
    return self.map_type_to_class(feed_id).get_content_for_feed(feed_id)
  end

  private

  def Feed.map_type_to_class(feed_id)
    feed_type_id = Feed.find(feed_id).feed_type_id
    not_yet_implemented = nil
    case stream_type_id
      when 1
        return Webpages
      when 2
        return Pictures
      when 3
        return not_yet_implemented
      when 4
        return Twitter
      when 5
        return not_yet_implemented
      when 6
        return not_yet_implemented
      when 7
        return not_yet_implemented
      when 8
        return not_yet_implemented
      else
        return nil

    end
  end

Then each content class implements a get_content_for_feed method to return the content:

def Twitter.get_content_for_feed(feed_id)
    tweet_urls = self.where(:feed_id => feed_id.to_i)
    if tweet_urls.empty?
      return []
    else
      return tweet_urls
    end
  end

It just returns the appropriate class. This works, but I know that it is wrong, but I am not sure what I would be doing.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

First a light review:

You've got a bug, I think. Your map_type_to_class sets a feed_type_id variable, but your case statement is looking at a stream_type_id variable that I don't see anywhere.

Second, don't use so many return statements. It's Ruby; returns are implicit. So this you don't need returns in if...else or case branches you have in your question. In idomatic Ruby, you only see return if someone specifically wants to exit a method early (and even that's rare with well-factored code).

Lastly, this bit is redundant

tweet_urls = self.where(:feed_id => feed_id.to_i)
if tweet_urls.empty?
  return []
else
  return tweet_urls
end

Which, to all intents and purposes is the same as simply:

self.where(:feed_id => feed_id.to_i)

It's implicitly returned, and it's array-like, so there's no need to do anything special. It's either empty or it's not. If you want something that's truly an Array instance (and not just duck-typed), you can append a .to_a.

Anyway, what you're looking for is the constantize method for strings. What you've got is a fairly simple has_many relationship, you just want something like a string item_type column on your Feed model that contains the class name (as a string) instead of your current numeric ID.

From there it's a matter of implementing a polymorphic has_many (Rails already has a polymorphic option for belongs_to but that's a to-one association).

You could simply do

def Feed
  def items
    klass = item_type.constantize
    klass.where(feed_id: id)
  end
end

(yes that could just be one line).

You won't have all the fancy collection methods, caching, and other nice stuff Rails provides for regular associations, but you can add that if you want or simply make do without it (I recommend the latter unless you find it to be bigger hassle).

So in your FeedsController (or wherever), you're looking at something like

def show
  feed = Feed.find(params[:id])
  render json: feed.items
end

That should work as a super-minimal implementation. The point is really just that you store the class name, which you can then turn from a string into a class name constant with constantize.


Alternatively, you could use Rails' built-in associations, but there's more setup involved and it's not as clean to look at. Again, you'll want an item_type string column.

def Feed
  has_many :webpages # the class name Webpage is implicit
  has_many :pictures
  has_many :twitters # yuck, not a good name, but it'll imply the Twitter class

  def items
    association = item_type.underscore.pluralize # convert a class name to an association name
    send(associtation) # call the association method (e.g. `webpages`)
  end
end

The main advantage is that the items method is just a shortcut that provides the correct association object, so you have all the usual convenience methods. So you can do something like this for a webpage-type feed

Feed.find(some_id).items.create(params[:url])

Of course, the major issue is that it's not clear what's going on. Is items a collection of Picture? Of Webpage? If you just blindly try to create a Picture record from params meant for a webpage, things get messy.

So I'd stick to the first version, where items isn't an opaque alias for an association, and instead work a bit harder when adding new records and such.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you for such a clear and informative answer. The 'stream_id' was my mistake when trying to make the question more clear, as 'feeds' in my app are called 'streams'. As to why I am doing the 'return an empty array' thing, I am very paranoid about having nil returned by a method because it will break my do loop. I tired doing constantize and it worked. My other thought was to serialize the classes in the database. Would this work as well, is it preferred? Basically it comes down to how should I let there be a 1-1 or 1-many relationship between a resource and a group of classes? \$\endgroup\$ Commented Sep 10, 2014 at 13:39
  • \$\begingroup\$ @RyanCollins 1. If you've got a do-loop there's probably a much more Ruby-like way to the same thing. "Plain" loop constructs are fairly rare in Ruby :) 2. Don't serialize stuff in the DB unless you really, really need to. It's harder to migrate later and quickly becomes just an opaque blob. Plus, it couples your data to Ruby & Rails. Stick to Rails' conventions as much as possible, and if you do have to go off the beaten track, look to Rails for inspiration. Its solutions are usually simple and they works (e.g. the constantize trick is what it uses for its polymorphic belongs_to). \$\endgroup\$
    – Flambino
    Commented Sep 10, 2014 at 16:15
  • \$\begingroup\$ @RyanCollins Addendum: I said "don't couple to Rails" but also "stick to Rails' conventions" which may sound odd. The point, though, is that Rails' conventions aren't conventions within Rails; they're conventions applied to Rails. So Rails' ways of doing thing are almost always ways that make sense in and of themselves - they're not dependent on Rails. There are just a lot of good patterns there. Which, in turn, means that the less work you do in Rails, the cleaner the result. So don't try to over think it :) \$\endgroup\$
    – Flambino
    Commented Sep 10, 2014 at 17:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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