views:

50

answers:

2

Hi all-

I have an application wherein the application will need to check whether one user is waiting for another user whenever a page is loaded. I have entries that look like this:

def self.up
    create_table :calls do |t|
      t.string   "user1_id"
      t.string   "user2_id"
      t.boolean  "active", :default=>false
      t.string   "meeting_url"
      t.string "embed_url"
      t.timestamps
    end

Currently the application checks the calls table for any calls that match the user id's and if active == true. If there are results, they are displayed to the user. The problem is that this necessitates a db call for every page load. So my questions are as follows:

1) Is this the most efficient way to do it? (Clearly I'm skeptical) 2) If it is, how is it best to accomplish this in the DRYest way?

Thanks much

Dave

A: 

This model feels a little muddled to me. It's that user1_id and user2_id in particular that gives me a squicky feeling in my guts -- it tells me that you don't really care who's waiting for whom, that there is no difference between the two users, and that you're happy checking two columns with an "OR" every time you want to look up a user. Which is a mess for both Rails and SQL.

It's also not the business problem you're describing. The relationship between these two users is not perfectly symmetrical: one person is waiting for another. And you clearly care which is which, but you're not modeling it. Or if you are, you're naming them poorly.

I don't have an exact sense of your application from this, but I would probably do two things to clean this up:

  1. Refine those user fields into an asymmetrical relationship: an initiator and a recipient, perhaps. You might call them caller_id and callee_id. Whatever. Then each session only has to check the callee_id field (I think) to see if someone's waiting for that user. That might be enough. If so, don't bother with Step Two.

  2. Push that "waiting for" relationship into a separate, simpler model that's optimized for fast writes and deletes. When someone tries to initiate a call with someone else, you write into that Waiting store which is keyed on the recipient. I'd probably use memcached directly for this instead of a database -- then you have virtually no delay, and frequent reads and updates won't bog down other operations. (Or, depending on the database technology you're using, this could be one of those rare cases where a memory-based table is called for.)

It seems to me like you're thinking data first and trying to make your code fit around the table structure you have. That's not optimal for Rails. Think about your behavior first, and make your data models adapt to what you want users to be able to do.

SFEley
SFEley- Thanks for the answer. I actually changed some of the naming to protect to innocent, but I see what you're getting at. I will check out the memcached option as that seems like a good solution to avoiding really frequent db reads
A: 

If your application needs to know if a user has some calls waiting at each page load, and that information is kept only in the database, then yes, I'd expect that you'd have to query the database for that information at each page load.

As far as DRYness goes, Rails provides a number of mechanisms throughout the MVC stack that ensure that keep things DRY and maintainable:

In the model, you could add a class method to your Call model to encapsulate the logic for the querying awaiting calls. It could be something like:

class Call < ActiveRecord::Base

def self.awaiting_for_user user
    self.all(:conditions => ['user_id = ? AND active = true', user])
end

By encapsulating the query in a method like this, you ensure that if the underlying logic to query that information changes, your invocations of that method will not.

Instead of adding this method call to each controller actions (index, show, etc.), you could use the controller before_filter/around_filter/after_filters methods to run this code on sets of controllers/actions.

class ApplicationController
    before_filter :load_current_user
    before_filter :load_current_users_calls

....
protected

    def load_current_user
        # You could encapsulate this method in your User model
        @user = User.find(session[:user_id])
    end

    def load_current_users_calls
         @calls = Call.awaiting_for_user(@user)
    end

Use the before_filter options like :except and :only to pinpoint the use of the filter in particular controller actions. Also, while declaring the actual load_current_users_calls methods in the ApplicationController ensures that each sub-classed controller has that method, you don't have to put the before_filter declaration in ApplicationController -- you could just put it any controller you want to run the before_filter.

And in the view layer, you could make partial to easily display the @calls in your standard layout. You could add something like this to your layout:

<%- if @calls && [email protected]? -%>
<%= render :partial => 'awaiting_calls_display', :locals => {:calls => @class} =%>
<%- end -%>

Or if that doesn't fit in your layout, you could call it from any applicable views.

greenagain
Greenagain-Thanks, this was helpful.