2
\$\begingroup\$

I know I have seen this before and I can't remember how to fix it or find where I saw how to fix it:

def product_matcher(item)
  product = []
  if !item.blank?
    product = self.products.where(:id => item.id).first
  else
    product = self.products.where(:id => token.id).first
  end
  product
end

It seems wrong to me to first set the variable to an array, then inside the scope of the each to set product to the result of the query and then at the end of the method have the variable return the product. Since Ruby doesn't have explicit types my belief is that you don't even need the product = [] and since the result of a Ruby method is always to return you more than likely don't even need product at the end. However, due to scoping the output would be encapsulated inside the each block and the method would ultimately return nothing when called. How do I improve this?

\$\endgroup\$
1
\$\begingroup\$

It is redundant to use self here.

def product_matcher(item)
  attribute_id = item.blank? ? token.id : item.id
  products.where(id: attribute_id).first
end
\$\endgroup\$
1
\$\begingroup\$

Some notes:

  • product = [] is doing nothing.
  • Note that if in Ruby are expressions, not statements, you can write product = if ....
  • if !item.blank? -> if item

I'd write:

def product_matcher(item)
  products.where(id: (item || token).id).first
end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.