Podcast #128: We chat with Kent C Dodds about why he loves React and discuss what life was like in the dark days before Git. Listen now.
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 use the singular method of ActiveModel::Naming: ActiveModel::Naming.singular(my_active_record_instance) Alternatively, you can use the model_name method that's mixed in to ActiveRecord by the same ActiveModel::Naming module. However, it's not available on a record instance, so you still have to go through its class: my_active_record_instance.class....


10

I introduced a bug into our codebase by calling #titleize on a string that could possibly come in as nil When a problem is so pervasive, it's usually a sign that there is a conceptual problem with the language itself, some call it void safety, also dubbed "the billion-dollar mistake". Some languages, specially the functional ones, use an Option type. Let's ...


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

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

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

This could should not be in either the model or the view. Rather is should be contained in the submission_helper, or if you really wanted to look into making it less procedural and more OO, then it should be handled by a presenter, a pattern in rails that steps in between the model and the view. Jeff Casimir has developed a presenter for RoR called Draper (...


7

You could create another helper to create your navigation links: def navigation_link_to(text, path) link_to text, path, class: "btn #{active_page(path)}" end Then in your view: = navigation_link_to 'Home', root_path = navigation_link_to 'Sign in', sign_in_path = navigation_link_to 'Account', account_path


7

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


7

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


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

If you work with database you can do this more efficiently with sql request: LineItem.where(:card_id => @card.id).sum('quantity') This is equivalent sql query SELECT SUM(items.identity) FROM items WHERE items.card_id = ?;


6

Notes: Any reason not to set the attributes in a single create step? DRY by using procs for short-lived functions. Methods should return meaningful values. Here it makes sense to return the newly created object. (As Mark pointed out) The mismatch names between attributes makes the code more verbose that it should be. However, I guess part of this mismatch ...


6

Roughly: twitter = Twitter::Client.new img, max = @post.image ? [@post.image.path.to_s, 120] : ["", 140] twitter.update("#{@post.content.truncate(max)}#{img}") redirect_to root_path All the Twitter stuff shouldn't be in the controller, rather in a utility class. This makes testing easier, makes plugging in implementations easier, makes extension easier, ...


6

Yes, use namespace and resources: namespace :oauth do resources :apps, controller: "applications", as: :applications end This is not 100% identical (the names of some of your routes have changed) but it is the way you should be building your routes, and you should change the rest of your app to reflect the change. The primary difference is your names ...


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


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