4
\$\begingroup\$

A little background:

My app has two models called mapgroups and usergroups. A map belongs to a group and a user belongs to group. You can go in a group and add maps which is how maps are "assigned" to a user. These will show up on their dashboard.

class User < ApplicationRecord
  def fetch_maps
    @groups = self.groups
    @maps = Array.new

    @groups.each do |g|
      g.maps.each do |m|
        @maps << m
      end
    end

    return @maps
  end
end

In order to display the maps the user should have shown in their dashboard I created a fetch_maps method to call on the current user.

Im wondering if there is any way to clean this up or really just any feedback. This is the first method I have written with current Ruby knowledge.

Edit:

Mapgroup Model

class Mapgroup < ApplicationRecord
  belongs_to :map, optional: true
  belongs_to :group, optional: true
end

Usergroup Model

class Usergroup < ApplicationRecord
  belongs_to :user, optional: true
  belongs_to :group, optional: true
end
\$\endgroup\$
0
3
\$\begingroup\$

This would potentially be a very expensive way to do this as you have to query the dtabase once for every group the user is in, i.e. an n+1 query. You should be able to do it with a single query by creating a relation:

class User < ApplicationRecord
  belongs_to :usergroup
  has_many   :groups,     through: :usergroup
  has_many   :maps,       through: :groups

then you can just query the map relation. An alternative would be to use a join. There is more than one way to do this but you could do:

def fetch_maps
  Map.joins(:mapgroups => {:groups => :users} ).where(user: self)
end

Rails doesn't always run the queries the way you expect so I suggest you run your code in the console or watch your log files to make sure that you are only running one query.

\$\endgroup\$
7
  • \$\begingroup\$ Compared to the answer above which is less expensive? Is one or the other better practice based on the number of queries it has to run in order to retrieve those results? \$\endgroup\$ – Trenton Tyler Mar 6 '18 at 14:55
  • \$\begingroup\$ Compared to 200_successs answer? I would run a test if this matters to you (obviously I don't know the size of your data set and this may not matter at all) but his solution is just a simplification of yours and will query the database multiple times. \$\endgroup\$ – Marc Rohloff Mar 6 '18 at 18:41
  • \$\begingroup\$ I think that with those associations in place, fetch_maps can be replaced by just maps. That will issue a single query through the usergroups, groups, and mapgroups tables to maps. \$\endgroup\$ – David Aldridge Mar 9 '18 at 23:21
  • 1
    \$\begingroup\$ Sorry if that wasn't clear. I did mean that as an either/or answer. I would do it the first way if it works correctly. \$\endgroup\$ – Marc Rohloff Mar 10 '18 at 21:35
  • \$\begingroup\$ My bad: didn't read it properly. This should be the accepted answer. \$\endgroup\$ – David Aldridge Mar 10 '18 at 23:27
1
\$\begingroup\$

I don't see any reason to use instance variables @groups and @maps instead of local variables. (You could use instance variables to cache the result of a lookup, but that's not what you did here.)

In general, there are more elegant ways to express array concatenation in a loop. I recommend Enumerable#flat_map in this case.

class User < ApplicationRecord
  def fetch_maps
    self.groups.flat_map { |g| g.maps }
  end
end

The return is customarily implicit in Ruby.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ This looks like a correct Ruby answer, but isn't really adapted to Rails/ActiveRecord. \$\endgroup\$ – Eric Duminil Mar 9 '18 at 8:28
  • 1
    \$\begingroup\$ In the context of a Rails app, adding the association through groups directly to maps, as in Marc Rohloff's answer, is more efficient and succinct. \$\endgroup\$ – David Aldridge Mar 9 '18 at 23:23

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.