Ruby block returning inappropriate value

Question

Following up this screencast, and its someway returning 2..3 instead if Product records

def save
    puts "--- imported_products: #{imported_products.inspect}"
    // --- imported_products: 2..3
    if imported_products.map(&:valid?).all?
      imported_products.each(&:save!)
      true
    else
      imported_products.each_with_index do |product, index|
        product.errors.full_messages.each do |message|
          errors.add :base, "Row #{index+2}: #{message}"
        end
      end
      false
    end
 end

 def imported_products
   @imported_products ||= load_imported_products
 end

 def load_imported_products
   spreadsheet = open_spreadsheet
   header = spreadsheet.row(1)
   (2..spreadsheet.last_row).each do |i|
     row = Hash[[header, spreadsheet.row(i)].transpose]
     product = Product.find_by_id(row['id']) || Product.new
     product.attributes = row.to_hash.slice(*accessible_attributes)
     product
   end
 end

score 2 · Accepted Answer · answered Mar 25 '14 at 20:14

2

Your load_imported_products method includes an each block. That block is the last 'line' of your method, so the return value of the block becomes the return value of the method.

Try the following:

def load_imported_products
  spreadsheet = open_spreadsheet
  header = spreadsheet.row(1)
  products = []
  (2..spreadsheet.last_row).each do |i|
    row = Hash[[header, spreadsheet.row(i)].transpose]
    product = Product.find_by_id(row['id']) || Product.new
    product.attributes = row.to_hash.slice(*accessible_attributes)
    products << product
  end
  products
end

answered Mar 25 '14 at 20:14

mattwise

1,464
1
10
20

1

Or use #collect (http://www.ruby-doc.org/core-2.1.1/Array.html#method-i-collect) instead of #each in the poster's original method. – probablykabari Mar 25 '14 at 20:15
thanks @mattwise, thought ruby or rails version issue since Ryan is using the same code for `rails 3.2.9` with `ruby 1.9.2` – Mar 25 '14 at 20:17

score 1 · Answer 2 · answered Mar 25 '14 at 20:22

Or use map

def load_imported_products
  spreadsheet = open_spreadsheet
  header = spreadsheet.row(1)
  products = (2..spreadsheet.last_row).map do |i|
    row = Hash[[header, spreadsheet.row(i)].transpose]
    product = Product.find(row['id'].to_i) || Product.new
    product.attributes = row.to_hash.slice(*accessible_attributes)
    product
  end
end

Also find_by_id is not needed the find method uses id although I forced it to an integer in case it was nil or stored as a string.

score 0 · Answer 3 · answered Mar 25 '14 at 20:14

0

If you're talking about the load_imported_products method, then try adding each product to an array, than then return the array.

I'm not sure exactly what that method is returning, but you probably need to explicitly return a collection of products.

So

def load_imported_products
   products = []
   spreadsheet = open_spreadsheet
   header = spreadsheet.row(1)
   (2..spreadsheet.last_row).each do |i|
     row = Hash[[header, spreadsheet.row(i)].transpose]
     product = Product.find_by_id(row['id']) || Product.new
     product.attributes = row.to_hash.slice(*accessible_attributes)
     products << product
   end
   return products
 end

answered Mar 25 '14 at 20:14

user3446496

361
1
8

explicit return is unnecessary in ruby as ruby always returns the last statement.(so just products would suffice) Except in the case of setter methods in which case it returns the value set. – engineersmnky Mar 25 '14 at 20:19
Thanks, and you're right. I know that it's unnecessary, but I thought that it would help with read-ability, especially since the question was asking why the method wasn't returning what it was supposed to. Out of curiosity, why WOULDN'T you type 'return' when you want to return a value from the method? It might save a couple keystrokes, but isn't it worth is to make it clearer that you want that value to return? – user3446496 Mar 25 '14 at 20:25
1

It just is not considered proper convention. There is really nothing wrong with it some people use it inside loops or conditionals but in general explicit returns create vernacular code smell and indicates that there is probably a better way to write your code to avoid this. – engineersmnky Mar 25 '14 at 20:28

Ruby block returning inappropriate value

3 Answers3