3
\$\begingroup\$

This code works fine, but I'm still learning, and feel like there is a more Railsy way to write this. Is there a good rails ActiveRecord method I should learn more about? Please let me know if you know a shorter, more direct way to run this same code.

Scenario: A user can have many languages, and level is an attribute of languages. I want to iterate through languages_users and find all the languages of a user, and then find where another user has 2 of the same languages, but at opposite levels.

user.rb

class User < ApplicationRecord
  has_many :languages_users
  has_many :languages, :through => :languages_users
end

controller

#Fluent languages and nonfluent languages can change depending on this situation, but here is good example:
@fluent_languages = LanguagesUser.where(user: current_user.id).where("level > 4")
@nonfluent_language = LanguagesUser.where(user: current_user.id).where("level < 5")

email_matches(@fluent_languages, @nonfluent_languages)


def email_matches(fluent_languages, nonfluent_languages)

    @fluent_languages = fluent_languages
    @nonfluent_languages = nonfluent_languages

    #array of user's fluent language_ids
    @fluent_langs = []
    @fluent_langs << @fluent_languages

    #array of user's nonfluent language_ids
    @nonfluent_langs = []
    @nonfluent_languages.each do |lang|
        @nonfluent_langs << lang.language_id
    end

    @fluent_les = LanguagesUser.select(:user_id).where(language_id: @fluent_langs, level: 1..4)
    @fluent_users_ids = []
    @fluent_les.each do |le|
        @fluent_users_ids << le.user_id
    end

    @nonfluent_les = LanguagesUser.select(:user_id).where(language_id: @nonfluent_langs, level: 5)
    @nonfluent_user_ids = []
    @nonfluent_les.each do |le|
        @nonfluent_user_ids << le.user_id
    end

    @matches = @fluent_users_ids & @nonfluent_user_ids

    @new_matches = User.where(id: @matches, status: "Active").order('last_seen DESC').limit(10)
    @new_matches.each do |user|
        UserMailer.new_match(user, current_user).deliver_later
    end
end

Let me know if I can provide anything else.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I have edited your post, especially the title, to better conform to the How to Ask guidelines. \$\endgroup\$ – 200_success May 14 '17 at 12:52
2
\$\begingroup\$

This is a somewhat shorter more concise version of your code. Taking advantages of some or Rails's magic and conventions.

def email_matches(current_user)
  fluent_ids = current_user.languages.where("level > 4").pluck(:language_id)
  nonfluent_ids = current_user.languages.where("level < 5").pluck(:language_id)

  fluent_user_ids = LanguagesUser.where(language_id: fluent_ids, level: 1..4)
    .pluck(:user_id)
  nonfluent_user_ids = LanguagesUser.where(language_id: nonfluent_ids, level: 5)
    .pluck(:user_id)

  matches = fluent_user_ids & nonfluent_user_ids

  new_matches = User.where(id: matches, status: "Active").order(last_seen: :desc).limit(10)
  new_matches.each do |user|
      UserMailer.new_match(user, current_user).deliver_later
  end
end

This takes advantage of the has_many :languages in your User model. Also uses the pluck method which returns an array of language_ids

current_user.languages.where.where("level > 4").pluck(:language_id)

There is however a way to do this all in maybe 1-2 lines of SQL, maybe someone else will provide that magic here.

\$\endgroup\$
2
\$\begingroup\$

I think you should be closer to MVC here. All this code should be in model, so from controller you just need to command user email matches like @user.email_matches. Also I would use standard Rails tools for creating conditioned relations between models.

    class User < ApplicationRecord
      has_many :languages_users
      has_many :languages, :through => :languages_users
      has_many :fluent_languages_users, -> { :fluent }, class_name: 'LanguagesUser'
      has_many :fluent_languages, through: :fluent_languages_users, source: :language
      has_many :non_fluent_languages_users, -> { :non_fluent }, class_name: 'LanguagesUser'
      has_many :non_fluent_languages, through: :non_fluent_languages_users, source: :language

      def email_matches
        fluent_users = non_fluent_languages.map {|language| language.fluent_users}.flatten
        non_fluent_users = fluent_languages.map {|language| language.non_fluent_users}.flatten

        matching_user_ids = (fluent_users & non_fluent_users).map(&:id)

        User.where(id: matching_user_ids, status: 'Active').order(last_seen: :desc).limit(10).each do |user|
          UserMailer.new_match(user, self).deliver_later
        end
      end
    end

    class LanguagesUser < ApplicationRecord
      belongs_to :user
      belongs_to :language
      FLUENT_LEVELS = [5]
      NON_FLUENT_LEVELS = [1,2,3,4]

      scope :fluent, -> { where(level: FLUENT_LEVELS) }
      scope :non_fluent, -> { where(level: NON_FLUENT_LEVELS) }
    end

    class Language < ApplicationRecord
      has_many :languages_users
      has_many :users, :through => :languages_users
      has_many :fluent_languages_users, -> { :fluent }, class_name: 'LanguagesUser'
      has_many :fluent_users, through: :fluent_languages_users, source: :user
      has_many :non_fluent_languages_users, -> { :non_fluent }, class_name: 'LanguagesUser'
      has_many :non_fluent_users, through: :non_fluent_languages_users, source: :user
    end

    @user = User.find(1)
    @user.email_matches

Or if it would be too consuming for SQL querying you can remove those conditional and through relations and just do like this:

class User < ApplicationRecord
  has_many :languages_users
  has_many :languages, :through => :languages_users

  scope :last_seen_active, -> { where(status: 'Active').order(last_seen: :desc)) }

  def email_matches
    fluent_users = users_with_oposite_languages_level(:non_fluent)
    non_fluent_users = users_with_oposite_languages_level(:fluent)
    fluent_users.merge(non_fluent_users).last_seen_active.limit(10).each do |user|
      UserMailer.new_match(user, self).deliver_later
    end
  end

  private

  def users_with_oposite_languages_level(level_type)
    User.where(
      id: languages_users.send(level_type).map(&:language).compact.uniq do |language|
            language.languages_users.send(inverted_level_type(level_type)).map(&:user_id)
          end.flatten.compact.uniq
    )
  end

  def inverted_level_type(level_type)
    :fluent ? :non_fluent : :fluent
  end
end
\$\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.