31e2b397e15442a2d3ba42db7bd4584e

User has_many :orders, Order belongs_to :store. There's got to be a cleaner way to write this.

def has_store_url?(user)
    user and \
    user.respond_to?(:orders) and \
    user.orders.last and \
    user.orders.last.respond_to?(:store) and \
    user.orders.last.store.present? and \
    user.orders.last.store.respond_to?(:url) and \
    user.orders.last.store.url.present?
  end

Refactorings

No refactoring yet !

92772ff5353c89d9bd10f8e334161e16

Ben Hughes

June 16, 2011, June 16, 2011 17:55, permalink

No rating. Login to rate! 
# If you only care about nil's and not whether an object responds to the method:
def has_store_url?(user)
  user.try(:orders).try(:last).try(:store).try(:url).present?
end

# If you really care about whether an object responds to the method (and that sounds suspect to me... but that's another issue to be discussed):
def has_store_url?(user)
  user.orders.last.store.url.present?
rescue NoMethodError
  false
end

# Note: The above is a little heavy-handed and I'm not sure how I feel about it. For example a NoMethod error could be coming from the "store" method's implementation
# and you're rescuing it without that intent here. But, it is concise.
92772ff5353c89d9bd10f8e334161e16

Ben Hughes

June 16, 2011, June 16, 2011 17:55, permalink

No rating. Login to rate! 
# If you only care about nil's and not whether an object responds to the method:
def has_store_url?(user)
  user.try(:orders).try(:last).try(:store).try(:url).present?
end

# If you really care about whether an object responds to the method (and that sounds suspect to me... but that's another issue to be discussed):
def has_store_url?(user)
  user.orders.last.store.url.present?
rescue NoMethodError
  false
end

# Note: The above is a little heavy-handed and I'm not sure how I feel about it. For example a NoMethod error could be coming from the "store" method's implementation
# and you're rescuing it without that intent here. But, it is concise.
D41d8cd98f00b204e9800998ecf8427e

Marc

June 17, 2011, June 17, 2011 00:48, permalink

No rating. Login to rate! 
def has_store_url?(user)
  user.rtry(:orders, :last, :store, :url).present?
end

# Perhaps extend Object with this one
def rtry(*methods)
  methods.inject(self) { |obj, method| obj = obj.try(method) }
end
A8d3f35baafdaea851914b17dae9e1fc

Adam

June 19, 2011, June 19, 2011 17:28, permalink

No rating. Login to rate!

When you're chaining that many objects together, you're probably approaching it from the wrong point of view. I believe you're trying to do something like this:

class User < ActiveRecord::Base
  has_many :orders
  has_many :stores, :through => :orders
  
  def has_store_url?
    store = stores.order('orders.created_at DESC').first
    store.url.present? if store
  end
end

# For backwards compatibility with your method
def has_store_url?(user)
  user.has_store_url? if user
end

Your refactoring





Format Copy from initial code

or Cancel