CODEWITHSUNDEEP
ruby-on-railsruby-on-rails-3validationoptimization
kobaltz
kobaltz

Reputation: 7070

Optimize suggestions on check to see if a current user is a friend already or not

I have a user model where the user has a friendships of another user. The friendship model uses friend with a class_name of User. Everything seems to be working properly. However, I think that I'm just trying to patch together the functionality and not following best procedures.

In my controller, friendship controller, I have it where the current_user can add a friend. However, I do not want them to add the same friend twice.

user = current_user.id
friend = params[:friend_id]
temp_friendship = Friendship.where('(user_id = ? AND friend_id = ?) OR (user_id = ? AND friend_id = ?)', user,friend,friend,user)
if !temp_friendship.present?
  @friendship = current_user.friendships.build(:friend_id => params[:friend_id])
  if @friendship.save
    redirect_to current_user, :notice => "Added friend."
  else
    redirect_to current_user, :alert => "Unable to add friend."
  end
else
  redirect_to current_user, :alert => "Already a friend."
end

This code all works great. However, it seems like I'm making unnecessary calls back to the database. Is there any way to optimize this controller call, either validation through the model or anything like that?

I have tried doing this, but it only returns a validation error if I already initiated the friend. If someone added me as a friend (where the friend_id would be my user's ID) it would not raise any errors.

validates_uniqueness_of :user_id, :scope => :friend_id
validates_uniqueness_of :friend_id, :scope => :user_id

Upvotes: 0

Views: 480

Answers (3)

kik
kik

Reputation: 7937

You can't optimize for performances

What you do here is basically :

  1. make a request to check if record with same ids exist
  2. write a new record if not

That sounded familiar to you and you decide to try a mean to implement that as a uniqueness validation ; but that's no solution, you're actually doing exactly what #validates_uniquess does : check all ids, then save.

On that point, you can't do better, you've already reduced problem to its smallest steps. So even if you could translate this in symmetric scoped uniqueness rules, it would still fire two database queries (actually, it would fire three using two #validates_uniqueness_of).

You can optimize for legibility

On that point, there are several things you can do. That's no joke : it will save time when you'll have to read the whole controller quickly later, way after you write it.

First, your temp_friendship query could be a scope and a model method. That would be their place and it probably will prove useful.

Second, the redirection if friendship exists could be a before filter, which would leave the action way more legible :

class User < ActiveRecord::Base
  has_many :friends, through: :friendship

  scope :friend_with, ->( other ) do
    other = other.id if other.is_a?( User )
    where( '(friendships.user_id = users.id AND friendships.friend_id = ?) OR (friendships.user_id = ? AND friendships.friend_id = users.id)', other, other ).includes( :frienships )
  end

  def friend_with?( other )
    User.where( id: id ).friend_with( other ).any?
  end
end


class FriendshipsController < ApplicationController
  before_filter :check_friendship, only: :create

  def create
    @friendship = current_user.friendships.build( friend_id: params[:friend_id] )

    if @friendship.save
      redirect_to current_user, notice: 'Added friend.'
    else
      redirect_to current_user, alert: 'Unable to add friend.'
    end
  end

  private

  def check_friendship
    redirect_to( current_user, alert: 'Already a friend' ) if current_user.friend_with?( params[ :friend_id ] )
  end
end

Upvotes: 1

David Aldridge
David Aldridge

Reputation: 52386

Another option here is to remove the validation from the Rails app and enforce uniqueness in the database.

It's unconventional, but removes the need for the extra queries and is completely safe if a way that an intra-row validation enforced by an application outside the database never is. (Hence the comments in the Rails Guides about backing up ActiveRecord validation with database validation.

You would add to the save of the model a rescue step to handle uniqueness errors thrown by the RDBMS, and treat them as validation failures. there's good information on rescuing database erros here: Rails 3 ignore Postgres unique constraint exception.

I just raise this as another option, so just evaluate it to see if the unconventional-code trade-off is worth it for you.

I would really like to see this sort of methodology encapsulated in activerecord. Maybe I should roll my sleeves up ...

Upvotes: 1

MrTheWalrus
MrTheWalrus

Reputation: 9700

A custom validator could be used to check for either 'A is a friend of B' or 'B is a friend of A' as a case of 'friendship already exists'. The total number of database hits probably doesn't go down, though - the validator is just going to perform a similar check to what you already wrote. It's probably still a better way, since it moves the logic out of your controller, but I wouldn't expect any performance improvement.

Upvotes: 0

Related Questions