62c433a96de9da99d09afedcded5de76

Hi,

I have a users users table and a messages table. The users table has a column 'usertype'.

I am trying to count the total messages sent to a recipient(they are logged in) by usertype.

It works but Im sure its a dumb way to be doing it.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
<% myid = self.current_user.id %>


Messages:<br /><br />

<% my_messages = Message.find(:all, :conditions => "user_id =  #{myid}") %>

<% 

mcount = 0
prcount = 0
iecount = 0

%>
<% @my_messages.each do |i| %>

<% senders_usertype = User.find(i.sender_id).usertype %>

<%
case senders_usertype  
  when 'Industry Expert'
     iecount = iecount + 1  
  when 'Media'  
     mcount = mcount + 1  
when 'PR'  
   prcount = 'prcount + 1'
  else  
    # nada
end
%>
 
<% end %>

<%= mcount %> <br />
<%= prcount %> <br />
<%= iecount %> <br />

Refactorings

No refactoring yet !

4767889c5e0baba44af9bbb867c1c1d3

Derrick

January 10, 2008, January 10, 2008 21:52, permalink

No rating. Login to rate!

I'm not an expert on this but to start with I think you want to link the tables, so you'll have a column in in your messages table that is something like user_id or recipient_id, and then you'll just reference them like follows. I'm new to ruby, but I'm pretty sure thats how it would be done. If I can find the code in my app before anyone else answers i'll post it.

1
2
3
4
5
6
<% for user in @users -%>

 <p><%= user.name %> got <%= user.messages.size %> messages</p>

<% end -%>
62c433a96de9da99d09afedcded5de76

nootopian

January 10, 2008, January 10, 2008 23:07, permalink

No rating. Login to rate!

Thanks Derrick,

Apologies I probably should have provided more info.

I do have a user_id column in the messages table along with a sender_id.

In the user model I have
has_many :messages

I can do things like
User.find(12).messages
Message.count(:conditions => "user_id=12")

It just gets tricky when I want to count the total of messages sent to a recipient by usertype

5170ca260dbd2cdfd5a887a4dba7636f

Jeremy Weiskotten

January 11, 2008, January 11, 2008 00:19, permalink

1 rating. Login to rate!

You should let ActiveRecord wire your objects together instead of using finders to fetch them. You should also typically avoid calling finders in your view templates.

Instead of:

myid = self.current_user.id
my_messages = Message.find(:all, :conditions => "user_id = #{myid}")

you can use:

my_messages = current_user.messages

If the page is only going to display these counts, you should use the current_user.messages.count method. This will be more efficient than fetching the messages and each sender from the database. You really should move this count logic into the controller to clean up the view. Set the counts to variables like @count_messages_from_media, and use those variables in your view. 

1
2
3
4
5
6
7
<p>Messages:</p>

<p>
<%= current_user.messages.count(:conditions => ['sender_id in (select id from users where usertype=?)', 'Industry Expert']) %><br />
<%= current_user.messages.count(:conditions => ['sender_id in (select id from users where usertype=?)', 'Media']) %><br />
<%= current_user.messages.count(:conditions => ['sender_id in (select id from users where usertype=?)', 'PR']) %>
</p>
62c433a96de9da99d09afedcded5de76

nootopian

January 11, 2008, January 11, 2008 08:52, permalink

No rating. Login to rate!

Thats awesome. Thanks so much. I learnt a heap from that.

I had to change :condition to :conditions BTW

B849433e0e1a3f4ca0cf7cc55b8acd53

Casper

January 11, 2008, January 11, 2008 08:59, permalink

1 rating. Login to rate!

Not going to comment on the Rails parts (you should move more code into the Model, and have less in your view). Just the "rubyism" part.

Since you have all the cross references in your models you should be able to access all messages for a user by saying user.messages, and access senders of messages by doing message.sender.

Then we group the usertype counters into a hash, so we don't have to hard-code all that stuff and the code stays flexible and "adapts" when you add new user types. 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
# Get all messages for the current user
messages_for_user = current_user.messages

# Initialize our hash where we store the counters
message_type_counters = Hash.new

# Loop through the messages
messages_for_user.each do |msg|
  
  # Grab the usertype of the sender
  usertype = msg.sender.usertype

  # Make sure the hash element is initialized
  message_type_counters[usertype] ||= 0

  # Add to the counter
  message_type_counters[usertype] += 1

end

# Display the results
message_type_counters.each do |usertype, count|
  puts "#{usertype}: #{count}"
end
B849433e0e1a3f4ca0cf7cc55b8acd53

Casper

January 11, 2008, January 11, 2008 09:51, permalink

1 rating. Login to rate!

Or..come to think of it. This is a perfect place for an SQL JOIN. Databases love to do these kinds of things. And they do it fast. Here's a good place to read up on joins:

http://www.w3schools.com/sql/sql_join.asp

Solution now becomes: 

1
2
3
4
5
6
7
8
9
10
11
12
13
# Untested since I don't have your database, but it should be 
# something like this:
grouped_count = Messages.count(:conditions => "user_id = #{current_user.id}",
                               :joins      => "join users on sender_id = users.id",
                               :group      => "usertype")


# You will get back an ActiveSupport::OrderedHash
# which you can loop like this:

grouped_count.each do |usertype, count|
  puts "#{userype}: #{count}"
end
Dd9b278833b2db936814ac37a546a55f

fifoo

January 11, 2008, January 11, 2008 12:57, permalink

1 rating. Login to rate!

Not sure if you still need it but I thought I would post it anyway.
That's how I would do it using ActiveRecord features but I agree with Casper that if you don't need the messages, you probably want to rely on SQL for efficiency.
And I would definitely put most of the stuff in the Models.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
# User model
class User
  has_many :messages
  has_one  :type

  def number_of_messages_from_usertype(type)
    messages.select { |m| m.sender.type == type }.size
  end
end

# Message model
class Message
  belongs_to :recipient,
             :class_name => "User",
             :foreign_key => "user_id"
  belongs_to :sender,
             :class => "User",
             :foreign_key => "sender_id"
end
E635ccff7389d9070f5e7e9fe8b36beb

rpheath

January 11, 2008, January 11, 2008 19:25, permalink

2 ratings. Login to rate!

This is the perfect opportunity for an association method. Usually, if you say aloud what you are wanting to do, you can pretty much express that exact thing in Ruby/Rails. So, you are "wanting a count of the number of messages per user, per (dynamic) type".

Here's how I'd do it... Note: you can flop the options={} so that the default is to return a count (so you wouldn't have to pass :count_only => true each time), but by doing this you can also return the messages themselves as a set in case you need that separately, too. I just don't know which has higher precedence: count or message set. In the code below, the association method defaults to return the objects, and the helper defaults to return the count, so you can see how it might work from both perspectives.

user.rb

1
2
3
4
5
6
7
8
9
class User
  has_many :messages do
    def by_type(type, options={})
      options[:count_only] ||= false
      messages = find(:all, :conditions => [ 'type = ?', type ])
      options[:count_only] ? messages.size : messages
    end
  end
end

OPTION 1: without helper in view

1
2
3
4
5
6
7
<h1>Messages:</h1>

<ul>
  <li>Industry Expert: <%= current_user.messages.by_type('Industry Expert', :count_only => true) -%></li>
  <li>Media:           <%= current_user.messages.by_type('Media',           :count_only => true) -%></li>
  <li>PR:              <%= current_user.messages.by_type('PR',              :count_only => true) -%></li>
</ul>

OPTION 2: with helper in view

1
2
3
4
5
6
7
<h1>Messages:</h1>

<ul>
  <li><%= message_count_for 'Industry Expert' -%></li>
  <li><%= message_count_for 'Media'           -%></li>
  <li><%= message_count_for 'PR'              -%></li>
</ul>

application_helper.rb (or users_helper.rb / messages_helper.rb, depending)

1
2
3
4
def message_count_for(type, options={})
  options[:count_only] ||= true
  "#{type}: #{current_user.messages.by_type(type, options[:count_only])}"
end
62c433a96de9da99d09afedcded5de76

nootopian

January 12, 2008, January 12, 2008 16:22, permalink

No rating. Login to rate!

Thanks so much everyone.

I came across this recently
http://weblog.jamisbuck.org/2006/10/18/skinny-controller-fat-model

which is helpful to me in determining where in the MVC to put code.

62c433a96de9da99d09afedcded5de76

nootopian

January 12, 2008, January 12, 2008 16:26, permalink

No rating. Login to rate!

@Ryan, thanks for solving my next problem too! (displaying messages by usertype of sender)

5170ca260dbd2cdfd5a887a4dba7636f

Jeremy Weiskotten

January 12, 2008, January 12, 2008 21:18, permalink

No rating. Login to rate!

To improve on Ryan's refactoring, you can use the following for better performance when you only want counts.

1
2
3
4
5
6
7
8
9
10
11
12
class User
  has_many :messages do
    def by_type(type, options={})
      conditions = [ 'type = ?', type ]
      if options[:count_only]
        count(:conditions => conditions)
      else
        find(:all, :conditions => conditions)
      end 
    end
  end
end

Your refactoring





Format Copy from initial code

or Cancel