Ruby: Method not returning expected output

Question

I am trying to create a program which simulates a check out at a shop. However my AddItem method returns the class variable @item rather than just the singular item that it has found. So that once all the items have been added it will be able to display the total amount of those items.

class Action
  def initialize(customerMoney)
    @money = customerMoney
    @item = [{ name: :milk, price: 2.99 }, { name: :eggs, price: 1.50 }, { name: :bread, price: 2.00 }]
  end

  def CheckPrice(item)
    @item.each do |x|
      return x[:price] if x[:name] == item
    end
  end

  def AddItem(item)
    i = 0
    @item.each do |x|
      if x[:name] == item
        x
      end
    end
  end

  def CheckTotal(basket)
    total = 0
    basket.each do |x|
      total += x[:price]
    end
    puts total
  end
end

myBasket = []
customer = Action.new(20)
myBasket.append(customer.AddItem(:bread))

p myBasket

demir · Accepted Answer · 2019-10-11 10:54:55Z

2

It is better to use the find method:

def AddItem(item)
  @item.find{ |x| x[:name] == item }
end

UPDATE

In ruby, snake_case is preferred. I've edited the code with the ruby methods.

class Action
  def initialize(customer_money)
    @money = customer_money
    @items = [{ name: :milk, price: 2.99 }, { name: :eggs, price: 1.50 }, { name: :bread, price: 2.00 }]
  end

  def check_price(item)
    @items.find{ |x| x[:name] == :bread }[:price]
  end

  def add_item(item)
    @items.find{ |x| x[:name] == item }
  end

  def check_total(basket)
    puts basket.sum{ |x| x[:price] }
  end
end

edited Oct 11, 2019 at 10:54

answered Oct 11, 2019 at 10:37

demir

4,7272 gold badges25 silver badges34 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

mrzasa · Accepted Answer · 2019-10-11 10:52:06Z

1

Ruby always returns result of the expression evaluated in a method. In case of AddItem it's each. Enumerable#each returns the whole collection that was under enumeration. You need to add a found element as a last line of the method:

   def AddItem(item)
       i = 0 
       found = nil
       @item.each do |x|
           if x[:name] == item
               found = x
           end
       end
       found # <<<<<<<<< returning found
   end

Btw. in ruby methods are named with snake_case not CamelCase. Also @item is an instance (not class) variable.

Btw you could do the same with find:

   def AddItem(item)
     @item.find do |x|
       x[:name] == item
     end  
   end

as find returns the array element for what the block returns a truthy value

edited Oct 11, 2019 at 10:52

answered Oct 11, 2019 at 10:32

mrzasa

23.4k11 gold badges60 silver badges96 bronze badges

Comments

Viktor · Accepted Answer · 2019-10-11 10:43:01Z

0

In Ruby, the last variable defined in a method is automatically returned. Since you are defining @item last that's what you are getting back. The problem is you are looping through @item but not doing anything with the result so @item remains unchanged.

Assuming the @item should actually be plural @items?, I imagine what you actually want to do is use select rather than each.

def AddItem(item)
  @items.select {|x| x[:name] === item}
end

This will return an array of items that returned true from x[:name] === item

Then again the name of the method AddItem doesn't truly represent what the method is actually doing, but thats another story :)

edited Oct 11, 2019 at 10:43

Viktor

2,7933 gold badges22 silver badges31 bronze badges

answered Oct 11, 2019 at 10:40

Brad

8,80811 gold badges48 silver badges54 bronze badges

Comments

Jörg W Mittag · Accepted Answer · 2019-10-12 19:14:14Z

If you do not explicitly return something from a method (or next something from a block), then the last expression evaluated becomes the return value of the method (block, lambda, module definition body, class definition body).

Since you don't explicitly return from AddItem, the last expression that is evaluated will be the return value. The last expression that is evaluated, is:

@item.each do |x|
  if x[:name] == item
    x
  end
end

In other words, the return value of AddItem will be the return value of @item.each. According to the documentation of Array#each, the return value is simply the Array that each was called on. (Note that this is actually not specific to Array#each, it is the general contract of each, so this is true for all implementations of each.)

This makes sense: the purpose of each is to execute a side-effect for each element of the collection. It doesn't really have a useful return value. So, the two sensible return values would be nil and self, and the library designers chose self. (Presumably, to allow for method chaining, I don't know.)

The next problem is that your block doesn't actually do anything. Remember, the purpose of each is to execute a side-effect for every element, but your block doesn't have any side-effects! If you wanted to do anything useful, you would need a side-effect (such as modifying an outer variable binding):

def AddItem(item)
  return_value = nil

  @item.each do |el|
    return_value = el if el[:name] == item
  end

  return_value
end

Alternatively, you could return the found item directly:

def AddItem(item)
  @item.each do |el|
    return el if el[:name] == item
  end
end

But really, what you want to do is find the first matching item:

def AddItem(item)
  @item.find {|el| el[:name] == item }
end

Note that there a lot of confusing things about your code:

You have two methods that are both named CheckSomething, but the two methods do completely different things.
Also, neither of the two methods actually checks something, one method prints something and the other finds something.
Your method AddItem does exactly the same thing as CheckPrice, but it is named completely different. Also, it does things in a completely different way. (Which is why it doesn't work.)
Again, naming: AddItem doesn't actually add anything.
Also, what is that i = 0 doing in AddItem?
The class is named Action, which is a very generic name and doesn't tell much about what it's doing.
It doesn't seem to do much of an action, though.
When you instantiate Action, you assign it to a variable named customer. However, it doesn't appear that it doesn't do much "customery" things either.
In the end, you put the customer into a basket. Just imagine, this were a real-world supermarket. Is that how it works in the real world?

And one last remark: the standard Ruby Community Coding Style is to use snake_case for methods, local variables, instance variables, class variables, and global variables. PascalCase is for constants that point to classes or modules, SCREAMING_SNAKE_CASE for other constants. We don't use camelCase.

Collectives™ on Stack Overflow

Ruby: Method not returning expected output

4 Answers 4

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related