12

The method is doing three things: Determining whether or not to set display_name. Determining the default display name. Setting display_name to the default value. Each of those adds to the Abc metric. The biggest contributor to the Abc metric is determining the default display name. To me, that also seems like the most logically separate. We can lower the ...


11

The ?: is called a ternary operator. It's not specific to Ruby; a lot of languages have it (in fact, I believe I linked you to that wikipedia page in a comment very recently). Go easy on the newlines; there's a lot of whitespace there (update: question was edited; now it's gone) Use string interpolation We've talked about that is_ prefix on ? methods already ...


10

You can reduce the amount of queries by putting it into a transaction: ActiveRecord::Base.transaction do ... end This wont reduce the amount of queries but will do them all at once which will save it doing the commit step for each time it has to do the query. Note, that if one of them fails, the transaction will normally be rolled back. Bulk importing ...


10

Notes: Conditionals are expressions in Ruby, you can (and, idiomatically, should) move the assignment outside. active_record_relation.count == 0 is ok, but active_record_relation.empty? is more declarative. I'd write: def set_default_display_name credentials_by_type = user.credentials.where(type: type) credentials_by_name = user.credentials.where(...


10

About your code: As a general rule, the imperative pattern acc = 0; xs.each { |x| acc += f(x) }; total = acc should be replaced by the functional expression total = xs.reduce(0) { |acc, x| acc + f(x) } or total = xs.map { |x| f(x) }.reduce(0, :+). Given that Rails provides an Enumerable#sum, it can be simplified even more: total = xs.map { |x| f(x) }.sum. ...


9

Creating a model just to check the parameters is overkill and not idiomatic in Rails. If you need some extra logic, use the existing model. In this case, I'd make Post#tagged_with accept a nil value which wouldn't filter anything: def Post scope :tagged_with, ->(tag) { tag ? <code to filter> : all } end class PostsController < ...


9

Well first your indention is a bit strange, it'd be better like this: class Add_dash def dashesHere(word) attachTheDashes = "--" + word + "--" return attachTheDashes end end And you typically don't need return in Ruby, it'll return the last value evaluated in your function. So having that in mind you could write it like this: class Add_dash ...


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

Another view on @tokland solution: def authenticate! return fail! unless account = Account.find_by(subdomain: subdomain) return fail! unless user = account.users.find_by(email: params["user"]["email"]) return fail! unless user.authenticate(params["user"]["password"]) success! user end This intention is to be as close to "original task explained in ...


8

You don't need to define empty actions in your Controller. Your code should work even if you delete all the empty actions, leaving you only with the views and routes: page_controller.rb: class PageController < ApplicationController end views: adventure.html.erb cooking.html.erb dancing.html.erb programming.html.erb reading.html.erb running.html.erb ...


8

days_name? Shouldn't that rather be day_names? Actually, given its function it should probably be weekdays Shouldn't "sar" be "sat" for Saturday? Never seen it abbreviated "sar" The opposite of "all" is "none" - not "no" Day names are capitalized in English, so "Mon", "Tue", "Wed", etc. You rarely need to create an array and then push items to it. It's ...


8

Seems OK to me. However, it might be nice to add a "macro" to let you skip the I18n. and just use t like you do in views. And you can add some extra checks while you're at it, since you'll i18n want it to make noise when testing. For instance, you could add a file like spec/support/macros/i18n_macros.rb with the following: module I18nMacros def t(*args)...


8

The simplest thing to do is probably to use FactoryGirl's sequence: FactoryGirl.define do factory :company do sequence(:name) { |n| "#{Faker::Company.name} #{n}" } end end sequence basically gives you an auto-incrementing integer, so you can avoid uniqueness issues. Sure, it'll generate some slightly odd company names, but for testing that shouldn'...


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


7

You are right, you code is too verbose. This is a pretty common pattern and you have some alternatives. For example, active_support has the abstraction Object#try: <%= @person.team.try(:name) %> Another alternative is the Object#maybe proxy: https://github.com/bhb/maybe <%= @person.team.maybe.name %>


7

Personally, I only write feature tests for critical and very common cases, and outsource more specific tests (i.e is a certain field on a model validated) to model specs. I wouldn't consider it a feature that a certain validation error is shown to the user, but I consider it a feature that the user gets informed that something went wrong and his post ...


6

Some notes: g: use meaningful names. Don't mix code with parentheses and without. Check the pattern Object#presence. # if no address found, ...: declarative code makes this kind of comments unnecessary. I'd write: results = Geocoder.search(address).presence || Geocoder.search(latitude, longitude) if results.present? # save else log.info("Geocode ...


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


6

I would suggest using a service object. I would like to see what others think about this. It looks like there's too much going on for the controller. The controller should be responsible for execution and value return. The model for processing the object. The service for wiring up the object and giving it prepared params to do its job. This is not tested, ...


6

Rails already has environment-specific configuration via the files in config/environments. As explained in the Rails Guides, you can simply do # in config/application.rb (the default config) config.authentication_providers = %w(weibo open_wechat wechat) and # in config/environments/development.rb (overrides the default) config.authentication_providers = %...


6

You very rarely need to modify a variable from inside an each block. Instead, check out all the methods available to you in Array and Enumerable. Among them, there's #each_with_objects, which you can use like so: a.each_with_object({}) do |(name, id), hash| hash[name] ||= [] hash[name] << id end which'll produce the same hash as your current ...


6

The "default to empty string" bit should probably be done in the model. I.e. in a before_validation callback or similar. You can also use Hash#slice to pluck out certain keys: user = find_or_create_by(auth_hash.slice(%w(uid provider))) user.assign_attributes(auth_hash['info'].slice(%w(name email nickname)) user.avatar = auth_hash['info']['image'] user....


6

DRY - don't repeat yourself - basically means that you should extract repeated logic into methods. In this case you're repeating checking validity creating Date instances (if they're valid, you should just create them, and then compare them; it's madness to create them for the comparison only to re-create them a few lines later) ... and for each of those, ...


6

I'd just write the call with one line per key/value: Method.create( domain_id: domain.id, type_notification: type, message: title, url: url, owner_id: user.id ) Note that the explicit reference to self in method calls is not idiomatic (check the unofficial Ruby style guide for this and other useful tips). Also, since Method is a class name ...


6

If that really is all you need to calculate, then it's not too bad. However, I suspect that you have other date calculations happening in your application. In that case, why not take advantage of Ruby's Date class? The >> operator adds months. require 'date' next_period = (Date.today >> tax_period_months).month


6

Look into regular expressions, which Ruby is very good at: def vowel_censor(string) string.gsub(/[aeiou]/i, "X") # case-insensitive end Or, even easier, more low-level and more efficient, use String#tr: def vowel_censor(string) string.tr("aeiou", "X") # lowercase only; use "aeiouAEIOU" to handle both upper- and lowercase end For your current code: ...


6

You can make use of the Array.new, passing in the maximum number of stars you want to show, and defaulting all the stars to empty. Then, you can fill in the number of full stars you need. Then, finally, thanks to Numeric's divmod returning either 0 or 1 for the number of half stars you need, you make one more pass and fill in the number of half stars you ...


5

I can't speak for that particular Railscast episode, but rails provides an idiomatic way to deal with validation errors. Taking your code as an example, I'd change it to look like this (using Rails 4.2): app/controllers/profiles_controller.rb: class ProfilesController < ActionController::Base def show @profile = Profile.find(...) end def new ...


5

Many things here. Style considerations First, in ruby conditionals are expressions, so instead of : self.lat.nil? ? self.lat = zip.latitude.to_f : self.lat = self.lat.to_f you can do : self.lat = self.lat.nil? ? zip.latitude.to_f : self.lat.to_f you can also get rid of self when not assigning : self.lat = lat.nil? ? zip.latitude.to_f : lat.to_f you ...


5

Quite a few things sprang to mind immediately, but as I delved deeper I realized that, well, there are a lot of issues. So here goes. Outright bugs, I think User.apikeys - what? Shouldn't that be @user.api_keys? But I notice you don't test the index action, which would have caught that. Security Issues You're not really using params filtering correctly. ...


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