Move logic into controller in a Rails app

Question

I have defined this variable within my view

<% value_one.each do |s| %> 
  <% document = current_user.documents.where(skill_id: s.id ) %>  
<% end %>

So I'm finding the users documents based on the skill_id (s).

But I am wondering how i can move this logic into the controller and assign it to say @documents. I am unsure on how to pass the params s.id to the controller though?

Am I thinking about this wrong, or is there a simple way to do this?

Update

Based on the answer below my controller looks like so

def index
  @skills = Skill.all
  @documents = current_user.documents.where(skill_id: @skills.map(&:id)).all
end

I now have 1 single query (thankfully)

Document Load (0.5ms)  SELECT "documents".* FROM "documents" WHERE "documents"."user_id" = $1 AND "documents"."skill_id" IN (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, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70)  [["user_id", 1]]

It might be more appropriate to put this logic into a model instead of the controller. — Iron Savior
– Iron Savior, Commented Jan 30, 2015 at 18:04

user229044 · Accepted Answer · 2015-01-30 14:24:27Z

2

You're querying your documents in the least efficient way. If your value_one has 100 items, you're performing 100 queries.

Instead, you should do a single query for all the items in question:

# Assuming 'value_one' is available....
@documents = current_user.documents.where(skill_id: value_one.map(&:id)).all

You will have to actually store/map the objects to whatever values are in value_one. A simple solution would be to retain your current loop and find the object from the already loaded list of objects:

<% value_one.each do |s| %> 
  <% document = @documents.select { |d| d.skill_id == s.id } %>  
<% end %>

answered Jan 30, 2015 at 14:24

user229044♦

241k41 gold badges348 silver badges350 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Richlewis Over a year ago

yes i noticed my query for document is very poor, hence why I am looking to improve it and find out the best practice in this situation. Thank you for your help..

Richlewis Over a year ago

ok im getting undefined local variable or method value_one but i know you mentioned assuming value_one is available..how would i go about that?

Richlewis Over a year ago

oh wait a sec i think i understand what you mean now.. Ive updated my question to show my current controller, if you could confirm this is right (though by looking at the query now I think its right)

cristi_razvi Over a year ago

the idea is where does 'value_one' come from?

Richlewis Over a year ago

@meagar would you be able to assist with a way of accessing s.id in a partial or maybe refactoring my code to enable what i am trying to achieve <a href='stackoverflow.com/questions/28256604/…'> here </a> please

Sebastian Wramba · Accepted Answer · 2015-01-30 14:25:27Z

0

I think you need a function with one parameter. Assigning it to a global variable will not work, since you need that parameter for the query. So I'd say you could do

def userDocumentsForSkill(sid)
  return @current_user.documents.where(skill_id: sid)
end

... or similar and in your template:

<% value_one.each do |s| %> 
  <% document = userDocumentsForSkill(s.id) %>  
<% end %>

Not tested this, but I hope you get the idea.

answered Jan 30, 2015 at 14:25

Sebastian Wramba

10.2k8 gold badges43 silver badges59 bronze badges

Comments

Miknash · Accepted Answer · 2015-01-30 14:26:17Z

0

You shouldn't have ANY logic in the view.

Assuming that you have connection between document and skill (skill has_many documents) you can do document.includes(:skill) or joins(:skill). You can add additional conditions here.

This will generate only one query to the database, and your view will be cleared from any logic.

If you want me to provide full code please update question with controller for that particular action.

answered Jan 30, 2015 at 14:26

Miknash

7,9483 gold badges38 silver badges50 bronze badges

Collectives™ on Stack Overflow

Move logic into controller in a Rails app

Update

3 Answers 3

5 Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

Update

3 Answers 3

5 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related