9

Equivalent, but functional and idiomatic: def total_leg_count active_flights_within_timeframe.includes(:legs).map { |fl| fl.legs.size }.sum end This pure SQL query should be equivalent and more performant: def total_leg_count active_flights_within_timeframe.includes(:legs).count(:legs) end


8

Use joined tables to make more performant database calculations and avoid unnecessary models instantiations. def liked_by return [] unless object.likes.present? User.distinct.joins(:likes).where(likes: { bonus_id: bonus.id }).pluck(:email) end


6

I will stick to reviewing the code you've posted, although it seems tied to other code. First of all: Use descriptive variable names. x, y, q, and so forth aren't great for legibility. Use query, position and similar full words. Especially since you're dealing with complex nested arrays. There also seems to be some logical issues/dangerous assumptions. I ...


5

Statement, formatted readably: WITH sel AS ( SELECT id FROM categories WHERE name=$1 AND source=$2 ), ins AS ( INSERT INTO categories (name, source, created_at, updated_at ) SELECT $1, $2, 'now', 'now' WHERE NOT EXISTS (SELECT * FROM sel) RETURNING id ) SELECT id FROM ins UNION ALL SELECT id FROM sel; Dummy table: CREATE TABLE categories ( ...


4

There are some bugs related to parameter handling: Splitting by ", ": In #build_where, you split the skill parameter by ", ". However, you example has just "skills"=>"3,8" — with no space after the comma. Splitting will therefore fail. SQL injection vulnerability: Parts of the value of the skills parameter end up in the SQL query's WHERE clause, but ...


4

Some notes on your code: User.all.where -> User.where. The moment you write group_by you are not using SQL anymore, performance will suffer. The line starting with sum is very confusing, the indentation make you think it's the continuation of map but in fact it's part of its inner expression. Indentation should reflect the structure of an expression. map + ...


3

You can memorize result using ||= def referred_by_id @reffered ||= User.where(referral_code: referer_referral_code).first.id end def validates_presence_of_referer_referral_code u = referred_by_id if u.nil? errors.add :referer_referral_code, "Not a valid Referral Code" else if @user.referral_code == referer_referral_code ...


3

Whenever my controllers become too large and the logic too complex, I push that logic down into one of a couple layers: The model A Command or Service object Pushing Behavior Into The Model The first thing I see that could be pushed into the model is this code: @purchases.each do |hc| hc.stripe_charge_id = nil hc.stripe_transaction_id = nil end The ...


3

I think the purchases conditions can be simplified vastly if you just prepare the hash before. Something like this (off the top of my head): where_hash = {} { :uid => :customer_id, :pvid => :provider_id, :pat => :patient_id }.each do |the_var, hash_key| the_value = binding.local_variable_get(the_var) # Ruby >= 2.1 # binding.eval("...


3

These are just some ideas based on some assumptions, so I may be way off. Looking for a first_name in the last_name column sounds kind of silly hackery. Unless I'm looking at this the wrong way. The "hacky" feeling is maybe not so much because of the code itself, but elsewhere in the design. It sounds like a "hacky" user-facing UI or client-server API, ...


3

Couple of things to improve here: Firstly - do not use fixtures, use factories. There is a neat library allowing you to do this with ease called FactoryGirl (for rails you will need FactoryGirlRails). Having this in place you can define factories like: FactoryGirl.define do factory :thumbnail do name 'Thumbnail name if you have such a field' ...


3

The simplest refactor would be: # this is the base query requests = PurchaseRequest.includes(:purchase_order) .where(:created_at => @date_start..@date_end) .where(:total_cost => @cost_beginning..@cost_end) # then we add additional conditions if required requests = unless @vendor == "0" # if vendor, get specific purchase ...


3

[Improved with flat_map thanks to @tokland.] How about: def index? user.roles.flat_map { |role| role.activities['sheet'] }.include?('index') end In Ruby, it is almost never necessary to set a temporary variable to empty and iterate over an object, given the power of methods like map, reduce, find, all?, etc. I don't see the need to use uniq! since it ...


3

Some notes: That's too much code for a controller method, it should be moved to the model. The same name (@measurements) holds up to 4 different values. As a general rule, avoid variable rebinding: different values, different names. More about how the filtering method could be written keeping it DRY: https://stackoverflow.com/a/4480285/188031 For example: ...


3

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: :...


3

Turns out the last code snippet in my question was close to what I ended up needing: def self.special_animals_only SPECIAL_ANIMAL_TYPES.collect { |types| where(types) }.reduce(:or) end


3

Don't think your question belongs to this code review section. You would have gotten help much faster if had posted in stackoverflow instead. Anyway there are couple points you need to check. The CreateComments migration must have been updated (or at least there was another migration that changed the table) because the migration does not say anything about ...


2

# TODO: refactor this bullshit While amusing to devs, swear words probably don't belong in production code. In your copypasta of schema.rb, I do believe you forgot to paste the end keywords. As a side note, you can also write null: false instead of :null => false; as far as I can tell, this is the accepted syntax. You can do the same thing with ...


2

You should use a named Scope in your model class User < ... scope :by_ab_without_user, lambda do |ab_id, id| where(:ab_id => ab_id, :is_admin => null). where("id != :user_id AND role != 'head'", { :user_id => id }) end ... end You can then use it from your controller with User.by_ab_without_user(current_user.ab_id, ...


2

Using inheritance, first you can clean up your structure as following: class DepartmentNameSerializer < Serializer self.root = false attributes :id, :name end class DepartmentSerializer < DepartmentNameSerializer has_many :employees, serializer: EmployeeSerializer has_one :location, serializer: LocationNameSerializer end class ...


2

Wow... yeah, that's a lot. My immediate suggestion would be to simply make the dashboard do less. I doubt all of those things are of extreme importance all the time. A lot seems like historical data (that spans years), but it's shown along side current stuff (that spans hours, it seems). I imagine most users only use a fraction of all that (and some users ...


2

Some notes: if boolean then true else false end -> boolean. UserRelation.where(user_id: self.id).where(with_user_id: user.id) -> UserRelation.where(user_id: self.id, with_user_id: user.id) active_record_collection.count > 0 -> active_record_collection.exists? I don't understand why you have to check both ways, a "coach - student" relationship looks ...


2

A few tips that come into my mind: Extract Sunspot search logic to a separate class. It's not really a user's concern. Inline some scopes, i.e.: scope :with_dependents, -> { includes(:common_app, :video, :applications, :jobs) } (This is actually more readable in my opinion.) Extract password management and encryption to separate class (again, not ...


2

I'd say you're on the right track with you idea to extract a method. While I can't fully replicate your code and its context, I think something like this should work: task countries: setup do |task_name| rows = rows_from_file("system/#{task_name.to_s}", 2) process_rows(task_name, rows) do |row| { alpha2: row[0], name: row[1] } end end # ... def ...


2

TheJSON schema and data class attributes are unlikely to change. Consider implementing the class to encapsulate the JSON hash, adapting it to the ruby idioms and whatever other custom operations you've added. This should greatly decrease the cost and complexity of deserialization.


2

You can use includes on any activerecord relation. Judging by your message about having N+1 queries I suspect you are calling city_names somewhere in a loop. then the correct placement of the includes is the place where you select the elements to loop past. I'm guessing it is for a view or something so i write some example code for .html.erb. In ...


2

Rails already has a built-in mechanism for doing this with the first_or_initialize method within ActiveRecord::Relation. As an example, you can perform the following Service.where(protocol: params[:protocol], port_from: params[:port_from], port_to: params[:port_to]).first_or_initialize This will try and find a service with the given attributes OR ...


2

You can also implement your code using single where with a hash. @vendor will be an optional hash param: options = { created_at: @date_start..@date_end, total_cost: @cost_beginning..@cost_end } options['purchaseorder.VendorRef_ListID'] = @vendor unless @vendor == "0" @purchase_requests = PurchaseRequest.includes(:purchase_order).where(options)


2

Some notes: Conditionals are expressions in Ruby, so you can write: @purchase_requests = if ... Multiple conditions can be used in the hash argument of where. Ruby is a good language to DRY code, don't repeat yourself. To add something new to the other answers, a "declarative-abstraction-for-everything" approach might look like this (implementation of ...


2

First of all you should look at your controller and see what common functionality you can extract to a private methods and callbacks. Another fairly large concern with you controller is that you are just assuming that a record is created or deleted without checking the return value. Create / Update / Destroy actions can and will fail. class ...


Only top voted, non community-wiki answers of a minimum length are eligible